Repository navigation
fix(compute): omit enforceOnKey when enforceOnKeyConfigs is set in Co… - #12775
Conversation
c80433f to
8ba07a9
Compare
|
/cc @acpana can you take a look please? |
580a803 to
f1a4c5e
Compare
maqiuyujoyce
left a comment
There was a problem hiding this comment.
Could you please remove ComputeSecurityPolicy from tests/e2e/ratcheting.go:223 and rerun the real GCP tests?
A primary motivation for adding the custom TypeSet hasher and defaulting logic is to prevent permadiffs and reconciliation loops. Because ComputeSecurityPolicy is currently exempted in ratcheting.go, the integration tests actually skips the re-reconciliation step, which won't help validate if permdiff is gone.
| return computeSecurityPolicyRuleElem | ||
| } | ||
|
|
||
| func effectiveEnforceOnKey(rloMap map[string]interface{}) string { |
There was a problem hiding this comment.
Nit: A more consistent function name would probably be canonicalizeEnforceOnKey or setDefaultEnforceOnKey.
There was a problem hiding this comment.
Done. Renamed to canonicalizeEnforceOnKey.
| rateLimitThreshold: | ||
| count: 100 | ||
| intervalSec: 60 | ||
| enforceOnKey: IP |
There was a problem hiding this comment.
Just to make sure we are on the same page: Because spec.rule is an atomic list treated as the full Source of Truth, omitting enforceOnKeyConfigs here naturally clears it on GCP rather than leaving it unmanaged. This is different from a typical field not under a list.
There was a problem hiding this comment.
Yes, enforceOnKey/enforceOnKeyConfigs cannot be used together anyway, and this behaviour (clearing) is visible in the recorded logs:
- In
_http_old_controller.log(lines 265–293),patchRulesendsupdateMask=rate_limit_options.enforce_on_key,rate_limit_options.enforce_on_key_configswith"enforceOnKey": "IP"and"enforceOnKeyConfigs": [], and the subsequentGET(lines 477–487) confirmsenforceOnKeyConfigsis cleared on GCP. - In
_http.log(lines 248–291 & 414–423), the Direct controller'sPATCHreplacesrulesand similarly clearsenforceOnKeyConfigson GCP.
…mputeSecurityPolicy Fixes GoogleCloudPlatform#1022
f1a4c5e to
34b6eae
Compare
|
Done. Removed Removing the exemption also revealed a similar permadiff on |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maqiuyujoyce The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
fce16bc
BRIEF Change description
Summary
Configuring
rateLimitOptions.enforceOnKeyConfigson aComputeSecurityPolicyrule previously failed with a GCP API error (Only one of enforceOnKey and enforceOnKeyConfigs can be specified) because the Terraform schema forrate_limit_options.enforce_on_keydefinedDefault: "ALL". Even whenenforceOnKeywas omitted in YAML,"ALL"was injected alongsideenforceOnKeyConfigs.Alignment with Upstream & Config Connector Adaptations
This change incorporates the approach from upstream
terraform-provider-google-betaand adapts it for Config Connector's continuous reconciliation model (state-into-spec: absent):Default: "ALL"fromenforce_on_keysoenforce_on_key_configscan be specified without conflict.ForceSendFields: []string{"EnforceOnKey", "EnforceOnKeyName", "EnforceOnKeyConfigs"}inexpandSecurityPolicyRuleRateLimitOptionsso in-place rule updates (patchRule) clear previously configured mutually exclusive fields on GCP.state-into-spec: absentPermadiffs:Default: "ALL"alone causes a permadiff when bothenforceOnKeyandenforceOnKeyConfigsare omitted: KCC's spec remains""while GCP returns"ALL", causingschema.TypeSethash mismatches and infinitepatchRuleloops.Computed: truetoenforce_on_key.enforceOnKey = "ALL"inexpandSecurityPolicyRuleRateLimitOptionsonly when bothenforceOnKeyandenforceOnKeyConfigsare empty.resourceComputeSecurityPolicyRuleHashandeffectiveEnforceOnKeyto normalize omittedenforce_on_key("") to"ALL"duringTypeSethashing andupdateMaskdiffing whenenforce_on_key_configsis empty.Risk Mitigation & Testing
sync.Once-cached helper (getComputeSecurityPolicyRuleElem) to reference theruleschema in the custom hash function without relocating the ~330-line inline schema definition. Zero existing test fixtures were modified.resource_compute_security_policy_internal_test.goverifying payload expansion andTypeSethash equivalence whenenforce_on_keyis omitted vs. explicitly set to"ALL".computesecuritypolicy-ratelimitE2E fixture testingenforceOnKeyConfigs,enforceOnKey, rule transitions, and omitted defaults. Validated against real GCP via./hack/record-gcpand updatedmockgcpso that mock and real GCP HTTP traces match byte-for-byte.Fixes #1022 (b/556657009)
WHY do we need this change?
To allow
ComputeSecurityPolicyrules to configureenforceOnKeyConfigswithout requiring users to supply the awkward workaroundenforceOnKey: "".Additionally, exporting a security policy created via the GCP Console with
config-connector exportomitsenforceOnKey, causing subsequentkubectl applyoperations of exported manifests to fail with HTTP 400 unless manually edited.Special notes for your reviewer:
None.
Does this PR add something which needs to be 'release noted'?
Intended Milestone
1.158.0
Tests you have done
make ready-prto ensure this PR is ready for review.