fix(aws): parse resource policies into canonical JSON (sorted) before comparing#5622
fix(aws): parse resource policies into canonical JSON (sorted) before comparing#5622Skarlso merged 3 commits intoexternal-secrets:mainfrom
Conversation
Signed-off-by: Charles Moscofian <charlesmoscofian@hotmail.com>
22e6708 to
4442914
Compare
runtime/esutils/utils.go
Outdated
| } | ||
|
|
||
| // CanonicalJSONCompare compares two JSON documents for semantic equality. | ||
| func CanonicalJSONCompare(json1, json2 string) (bool, error) { |
There was a problem hiding this comment.
this is not enough for proper canonical comparison I believe. Please check https://pkg.go.dev/encoding/json/jsontext#Value.Canonicalize for details. if you really want canonical representations we should introduce JCS as per RFC 8785
There was a problem hiding this comment.
Yeap, you are absolutely right, it does not do full canonical comparison and it is not a fully RFC8785 compliant implementation. This is more of a paretto implementation in which it should cover 80% of the use cases (including the bug at hand). I see three clear options to continue from here:
- I can write a full compliant implementation of the RFC8785, I don't mind but it can take a little while, and it still might not be 100% compliant specially when evaluating non UTF keys.
- We could rely on using
jsonv2/jsontextbut those are experimental at this point or search for a third party library that handles the full implementation, but that needs to be vetted by maintainers. - There is a quick escape using reflect.DeepEqual , it's a simpler implementation but it adds quite a bit in terms of runtime cost specially on big resource policies.
I'm very much interested in driving this home, but I think I need some input from the projects perspective which direction makes more sense from the maintainers perspective.
| } | ||
|
|
||
| if maps.Equal(currentPolicyMap, policyJSONMaps) { | ||
| if isSame, err := esutils.CanonicalJSONCompare(currentPolicy, policyJSON); err != nil { |
There was a problem hiding this comment.
Rather than using a canonical JSON compare which is an incomplete comparison, how about just type switching? In case of a slice, use a slice, in case of a map use a map?
There was a problem hiding this comment.
I think this is mostly what this implementation does. It parses the representations of slices and maps (using sorted keys). Maybe the main issue with this implementation might be the naming, since this does not fully implements CanonicalJSON (it's based on it) it might feel misleading. I can rename it if that's the way to go.
There was a problem hiding this comment.
I mean that don't do byte compare. Why not just use a structural compare?
There was a problem hiding this comment.
I was looking over other parts of the project and reflect.DeepEqual is used, I think it can fit well in here.
Just cleanup the old implementation and pushed it.
There was a problem hiding this comment.
I think this works. Especially if other parts of the code are already using this. And your test pretty much shows that it works, right?
There was a problem hiding this comment.
Yes it does, I wrote them based replicating the issue.
Signed-off-by: Charles Moscofian <charlesmoscofian@hotmail.com>
WalkthroughReplaced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/v1/aws/secretsmanager/secretsmanager.go (1)
886-905: Handle emptycurrentPolicybefore JSON unmarshaling.When
currentPolicyis an empty string (line 886-889),json.Unmarshal([]byte(""), ¤tPolicyMap)at line 897 will fail with an "unexpected end of JSON input" error. This will cause the reconciliation to error out even when there's no existing policy and a new one should simply be applied.Consider handling the empty policy case explicitly:
currentPolicy := "" if currentPolicyOutput != nil && currentPolicyOutput.ResourcePolicy != nil { currentPolicy = *currentPolicyOutput.ResourcePolicy } + // If there's no current policy, we need to create one + if currentPolicy == "" { + putPolicyInput := &awssm.PutResourcePolicyInput{ + SecretId: secretID, + ResourcePolicy: aws.String(policyJSON), + } + if meta.Spec.ResourcePolicy.BlockPublicPolicy != nil { + putPolicyInput.BlockPublicPolicy = meta.Spec.ResourcePolicy.BlockPublicPolicy + } + + _, err = sm.client.PutResourcePolicy(ctx, putPolicyInput) + metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMPutResourcePolicy, err) + if err != nil { + return fmt.Errorf("failed to put resource policy: %w", err) + } + return nil + } + // convert to maps so we can do a stable comparison. var ( currentPolicyMap map[string]any policyJSONMaps map[string]any )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
providers/v1/aws/secretsmanager/secretsmanager.go(3 hunks)providers/v1/aws/secretsmanager/secretsmanager_test.go(5 hunks)
🔇 Additional comments (5)
providers/v1/aws/secretsmanager/secretsmanager.go (1)
904-906: Good fix:reflect.DeepEqualcorrectly handles nested slices.Using
reflect.DeepEqualresolves the panic caused bymaps.Equalwhen comparing maps containing[]interface{}values (which are not comparable). This properly addresses the linked issue #5620.providers/v1/aws/secretsmanager/secretsmanager_test.go (4)
109-136: LGTM: Helper function provides good test fixture.The
makeValidGetResourcePolicyOutputfunction creates a realistic AWS resource policy for testing the comparison logic. The policy structure matches the format expected from AWS Secrets Manager.
1124-1199: Test validates changed policy scenario correctly.This test verifies that when the resource policy differs (different principal ARN),
PutResourcePolicyis called. The test correctly includesPutResourcePolicyFnin the mock to verify it gets invoked.
1202-1209: LGTM: Test setup properly injects kubeclient.The
kubeclientfield is correctly wired into theSecretsManagerstruct for tests that require Kubernetes client interactions.
1049-1123: Test correctly validates that unchanged resource policies are not synced.This test verifies that when the existing resource policy matches the desired policy (despite different JSON key ordering),
PutResourcePolicyis not called. The test properly:
- Omits
PutResourcePolicyFnfrom the mock (which would fail if called)- Uses different JSON key ordering between the existing policy and ConfigMap to ensure policy comparison works correctly regardless of key order
The ConfigMap is created without an explicit
NamespaceinObjectMeta, which defaults to empty string. Verify that the test setup initializes the SecretsManager client with an empty namespace (or matches the ConfigMap's namespace) to ensure the ConfigMap lookup succeeds.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
providers/v1/aws/secretsmanager/secretsmanager_test.go (1)
1049-1123: LGTM! Well-designed test for canonical JSON comparison.This test correctly verifies that when an existing resource policy and desired policy have identical content but different JSON field ordering (lines 1091-1116 vs lines 111-134), the canonical comparison detects they're equal and skips the PutResourcePolicy call.
The implicit assertion (not setting PutResourcePolicyFn) is consistent with patterns elsewhere in this test file.
Optional: Consider explicit assertion
For clarity, you could add an explicit assertion that PutResourcePolicyFn was not called:
client: fakesm.Client{ // ... existing functions ... PutResourcePolicyFn: func(ctx context.Context, input *awssm.PutResourcePolicyInput, opts ...func(*awssm.Options)) (*awssm.PutResourcePolicyOutput, error) { t.Fatal("PutResourcePolicy should not be called when policy is unchanged") return nil, nil }, },However, the current pattern (omitting the function) is already used consistently in this file and is equally valid.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
providers/v1/aws/secretsmanager/secretsmanager.go(2 hunks)providers/v1/aws/secretsmanager/secretsmanager_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- providers/v1/aws/secretsmanager/secretsmanager.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Analyze project (actions, none)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (4)
providers/v1/aws/secretsmanager/secretsmanager_test.go (4)
41-42: LGTM! Necessary imports for Kubernetes client testing.The controller-runtime client imports enable proper test wiring for ConfigMap-based resource policy sources.
109-136: LGTM! Well-structured test helper.The helper provides realistic AWS IAM resource policy test data that serves as a baseline for the canonical JSON comparison tests.
567-567: LGTM! Proper test wiring for Kubernetes client.The kubeclient field addition and initialization correctly wire the fake Kubernetes client into the SecretsManager for testing ConfigMap-based policy sources.
Also applies to: 1208-1208
1124-1199: LGTM! Proper test for changed resource policy.This test correctly verifies that when the resource policy content actually differs (line 1184 has "role/sudo" vs line 128 has "role/admin"), the PutResourcePolicy call is made. This complements the previous test case and ensures the canonical comparison doesn't incorrectly skip updates when policies genuinely differ.
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets](https://github.com/external-secrets/external-secrets) | minor | `1.1.1` -> `1.2.0` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets)</summary> ### [`v1.2.0`](https://github.com/external-secrets/external-secrets/releases/tag/v1.2.0) [Compare Source](external-secrets/external-secrets@v1.1.1...v1.2.0) Image: `ghcr.io/external-secrets/external-secrets:v1.2.0` Image: `ghcr.io/external-secrets/external-secrets:v1.2.0-ubi` Image: `ghcr.io/external-secrets/external-secrets:v1.2.0-ubi-boringssl` <!-- Release notes generated using configuration in .github/release.yml at main --> #### What's Changed ##### General - chore: bump 1.1.1 by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5687](external-secrets/external-secrets#5687) - chore: fix the argocd e2e test case by [@​Skarlso](https://github.com/Skarlso) in [#​5688](external-secrets/external-secrets#5688) - feat(provider): add Barbican provider support by [@​rkferreira](https://github.com/rkferreira) in [#​5398](external-secrets/external-secrets#5398) - docs(secretserver): promote secretserver provider to beta by [@​DelineaSahilWankhede](https://github.com/DelineaSahilWankhede) in [#​5668](external-secrets/external-secrets#5668) - feat(controller): add flag to enable/disable secretstore reconcile by [@​Ilhan-Personal](https://github.com/Ilhan-Personal) in [#​5653](external-secrets/external-secrets#5653) - fix(aws-secrets-manager): Apply filtering based on both name and tags if provided by [@​iypetrov](https://github.com/iypetrov) in [#​5685](external-secrets/external-secrets#5685) - fix(gcpsm): SecretExists should check for regional secrets when store location is specified by [@​tokiwong](https://github.com/tokiwong) in [#​5708](external-secrets/external-secrets#5708) - feat: introduce store deprecation by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5711](external-secrets/external-secrets#5711) - feat(charts): add global values for common deployment configurations by [@​Gabryel8818](https://github.com/Gabryel8818) in [#​5652](external-secrets/external-secrets#5652) - feat: add Doppler OIDC-based authentication by [@​mikesellitto](https://github.com/mikesellitto) in [#​5475](external-secrets/external-secrets#5475) - fix: make custom configuration available regardless of environment by [@​Skarlso](https://github.com/Skarlso) in [#​5713](external-secrets/external-secrets#5713) - chore(chart): update bitwarden dependency to v0.5.2 by [@​Skarlso](https://github.com/Skarlso) in [#​5719](external-secrets/external-secrets#5719) - docs(templating): update rbac for generic targets by [@​lostick](https://github.com/lostick) in [#​5736](external-secrets/external-secrets#5736) - fix(testing): Breaking changes should not break ci by [@​evrardjp](https://github.com/evrardjp) in [#​5739](external-secrets/external-secrets#5739) - fix(security): Get rid of getSecretKey by [@​evrardjp](https://github.com/evrardjp) in [#​5738](external-secrets/external-secrets#5738) - fix(aws): parse resource policies into canonical JSON (sorted) before comparing by [@​cmoscofian](https://github.com/cmoscofian) in [#​5622](external-secrets/external-secrets#5622) - docs: Fix example in GCP documentation by [@​headcr4sh](https://github.com/headcr4sh) in [#​5745](external-secrets/external-secrets#5745) - chore(secretserver): update dependencies to accept new DelineaXPM/tss-sdk-go by [@​DelineaSahilWankhede](https://github.com/DelineaSahilWankhede) in [#​5742](external-secrets/external-secrets#5742) - fix(gcpsm): Improve SecretExists method in GCP secret manager provider by [@​tosih](https://github.com/tosih) in [#​5610](external-secrets/external-secrets#5610) - chore(docs): add clarification to helm values being disabled by [@​Skarlso](https://github.com/Skarlso) in [#​5746](external-secrets/external-secrets#5746) - fix(release): apply [`64dc681`](external-secrets/external-secrets@64dc681) to release by [@​jakobmoellerdev](https://github.com/jakobmoellerdev) in [#​5749](external-secrets/external-secrets#5749) - docs(release): 1.2 stability-support.md by [@​jakobmoellerdev](https://github.com/jakobmoellerdev) in [#​5750](external-secrets/external-secrets#5750) ##### Dependencies - chore(deps): bump golang from 1.25.4 to 1.25.5 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5693](external-secrets/external-secrets#5693) - chore(deps): bump golang from 1.25.4-bookworm to 1.25.5-bookworm in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5702](external-secrets/external-secrets#5702) - chore(deps): bump ubi9/ubi from `dcd8128` to `75937d9` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5655](external-secrets/external-secrets#5655) - chore(deps): bump peter-evans/slash-command-dispatch from 5.0.0 to 5.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5695](external-secrets/external-secrets#5695) - chore(deps): bump github/codeql-action from 4.31.5 to 4.31.7 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5696](external-secrets/external-secrets#5696) - chore(deps): bump actions/stale from 10.1.0 to 10.1.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5697](external-secrets/external-secrets#5697) - chore(deps): bump actions/create-github-app-token from 2.2.0 to 2.2.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5700](external-secrets/external-secrets#5700) - chore(deps): bump step-security/harden-runner from 2.13.2 to 2.13.3 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5698](external-secrets/external-secrets#5698) - chore(deps): bump actions/checkout from 6.0.0 to 6.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5699](external-secrets/external-secrets#5699) - chore(deps): bump platformdirs from 4.5.0 to 4.5.1 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5705](external-secrets/external-secrets#5705) - chore(deps): bump distroless/static from `87bce11` to `4b2a093` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5692](external-secrets/external-secrets#5692) - chore(deps): bump alpine from 3.22 to 3.23 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5703](external-secrets/external-secrets#5703) - chore(deps): bump urllib3 from 2.5.0 to 2.6.0 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5704](external-secrets/external-secrets#5704) - chore(deps): bump pymdown-extensions from 10.17.2 to 10.18 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5706](external-secrets/external-secrets#5706) - chore(deps): bump alpine from 3.22.2 to 3.23.0 in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5701](external-secrets/external-secrets#5701) - chore(deps): bump golang from `2611181` to `2611181` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5721](external-secrets/external-secrets#5721) - chore(deps): bump codecov/codecov-action from 5.5.1 to 5.5.2 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5725](external-secrets/external-secrets#5725) - chore(deps): bump urllib3 from 2.6.0 to 2.6.2 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5730](external-secrets/external-secrets#5730) - chore(deps): bump github/codeql-action from 4.31.7 to 4.31.8 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5726](external-secrets/external-secrets#5726) - chore(deps): bump anchore/sbom-action from 0.20.10 to 0.20.11 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5724](external-secrets/external-secrets#5724) - chore(deps): bump tornado from 6.5.2 to 6.5.3 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5732](external-secrets/external-secrets#5732) - chore(deps): bump ubi9/ubi from `75937d9` to `d4feb57` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5722](external-secrets/external-secrets#5722) - chore(deps): bump golang from `5117d68` to `09f53de` in /e2e by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5729](external-secrets/external-secrets#5729) - chore(deps): bump alpine from `4b7ce07` to `51183f2` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5694](external-secrets/external-secrets#5694) - chore(deps): bump hashicorp/setup-terraform from [`712b439`](external-secrets/external-secrets@712b439) to [`071811a`](external-secrets/external-secrets@071811a) by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5727](external-secrets/external-secrets#5727) - chore(deps): bump pymdown-extensions from 10.18 to 10.19.1 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5731](external-secrets/external-secrets#5731) - chore(deps): bump step-security/harden-runner from 2.13.3 to 2.14.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5728](external-secrets/external-secrets#5728) - chore(deps): bump actions/cache from 4.3.0 to 5.0.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5723](external-secrets/external-secrets#5723) #### New Contributors - [@​iypetrov](https://github.com/iypetrov) made their first contribution in [#​5685](external-secrets/external-secrets#5685) - [@​tokiwong](https://github.com/tokiwong) made their first contribution in [#​5708](external-secrets/external-secrets#5708) - [@​Gabryel8818](https://github.com/Gabryel8818) made their first contribution in [#​5652](external-secrets/external-secrets#5652) - [@​mikesellitto](https://github.com/mikesellitto) made their first contribution in [#​5475](external-secrets/external-secrets#5475) - [@​lostick](https://github.com/lostick) made their first contribution in [#​5736](external-secrets/external-secrets#5736) - [@​cmoscofian](https://github.com/cmoscofian) made their first contribution in [#​5622](external-secrets/external-secrets#5622) - [@​headcr4sh](https://github.com/headcr4sh) made their first contribution in [#​5745](external-secrets/external-secrets#5745) - [@​tosih](https://github.com/tosih) made their first contribution in [#​5610](external-secrets/external-secrets#5610) **Full Changelog**: <external-secrets/external-secrets@v1.1.1...v1.2.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi4zOS4xIiwidXBkYXRlZEluVmVyIjoiNDIuMzkuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiY2hhcnQiXX0=--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/2737 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
Problem Statement
Related Issue
Fixes: #5620
Proposed
Add a canonical comparison between JSON objects since
maps.Equalrequires all map values to implement comparable but slices can be parsed as []interface{} which is not comparable.This could also be achieved using reflection but it gets even more expensive, specially for bigger JSON objets.
Format
Please ensure that your PR follows the following format for the title:
Where
scopeis optionally one of:Checklist
git commit --signoffmake testmake reviewableSummary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.