Merged
Conversation
📝 WalkthroughWalkthroughAdds optional SOPS KMS key and AWS-backed SecretStore support to SecretStack: new CRD fields and status, templates to render KMS Key and SecretStore, observed-resource mocks and expanded tests, CI/Make changes to run example validations with observed-resources steps, updated project dependencies and README docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CLI
participant Render as Render Function
participant Composition as Composition Engine
participant Crossplane as Crossplane Providers
participant ESO as ExternalSecrets Operator
Dev->>Render: submit SecretStack spec (sops, secretStore, provider refs)
Render->>Render: init state (kubernetesProviderConfigRef, sops, secretStore)
Render->>Composition: emit KMS Key (if sops.enabled) and SecretStore objects (if secretStore.enabled and ESO ready)
Composition->>Crossplane: apply KMS Key and Kubernetes Object resources
Crossplane->>Composition: report observed statuses (kms-key ARN, ready conditions)
Composition->>Render: provide observed resource statuses
Render->>Dev: populate status.kmsKeyArn and final composed manifests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Published Crossplane PackageThe following Crossplane package was published as part of this PR: Package: ghcr.io/hops-ops/aws-secret-stack:pr-16-0d1576e2609d724454ffb6df34d5e875a34834fc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/render/000-state-init.yaml.gotmpl`:
- Around line 87-93: The template uses ($secretStoreSpec.enabled | default true)
and ($sopsSpec.enabled | default true), but Sprig's default treats explicit
false as empty; change these to explicit presence checks so false is preserved:
replace each use with an if/else that checks hasKey on the spec (e.g. if hasKey
$secretStoreSpec "enabled" then use $secretStoreSpec.enabled else true end) and
do the same for $sopsSpec.enabled so an explicit false is honored.
In `@README.md`:
- Line 16: The README's "Minimal" example incorrectly states the default scope
includes ClusterSecretStore; update the minimal example description to reflect
that the default scope is Namespaced and the default resource is SecretStore
(i.e., replace any mention of "ClusterSecretStore" with "SecretStore" and, if
present, explicitly note the default scope is Namespaced) so the documentation
matches the actual default behavior.
In `@tests/test-render/main.k`:
- Around line 368-403: The test header/comment for metav1alpha1.CompositionTest
with metadata.name = "secret-store-disabled" is misleading (says “renders
nothing” and mentions `Usage`) while the assertResources actually expect Helm +
PodIdentity; update the title/comment to accurately describe the expected result
(e.g., "SecretStore disabled renders Helm + PodIdentity" or similar) and remove
or correct the stray reference to `Usage` so the comment matches the assertions
in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98d65756-8ec6-45e1-beac-25efb0a12c91
📒 Files selected for processing (19)
.github/workflows/on-pr.yaml.github/workflows/on-push-main.yamlMakefileREADME.mdapis/secretstacks/definition.yamlexamples/test/mocks/observed-resources/standard/steps/1/helm-external-secrets.yamlexamples/test/mocks/observed-resources/standard/steps/1/kms-key.yamlexamples/test/mocks/observed-resources/standard/steps/1/pod-identity.yamlexamples/test/mocks/observed-resources/standard/steps/2/helm-external-secrets.yamlexamples/test/mocks/observed-resources/standard/steps/2/kms-key.yamlexamples/test/mocks/observed-resources/standard/steps/2/pod-identity.yamlexamples/test/mocks/observed-resources/standard/steps/2/secret-store.yamlfunctions/render/000-state-init.yaml.gotmplfunctions/render/010-state-status.yaml.gotmplfunctions/render/220-kms-key.yaml.gotmplfunctions/render/230-secret-store.yaml.gotmplfunctions/render/999-status.yaml.gotmpltests/test-render/main.kupbound.yaml
Comment on lines
+87
to
+93
| "enabled" ($secretStoreSpec.enabled | default true) | ||
| "scope" ($secretStoreSpec.scope | default "Namespaced") | ||
| "name" ($secretStoreSpec.name | default "default") | ||
| ) | ||
| "sops" (dict | ||
| "enabled" ($sopsSpec.enabled | default true) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Sprig template library, does default true .valuereturntruewhen.valueis explicitlyfalse?
💡 Result:
Yes, in the Sprig template library, default true .value returns true when .value is explicitly false.
Citations:
- 1: http://masterminds.github.io/sprig/defaults.html
- 2: default true false triggers the default value Masterminds/sprig#111
- 3: https://masterminds.github.io/sprig/defaults.html
🏁 Script executed:
fd -t f "000-state-init.yaml.gotmpl" -x cat -n {}Repository: hops-ops/aws-secret-stack
Length of output: 4279
🏁 Script executed:
# Check for other uses of | default true or | default false
rg '\| default (true|false)' functions/render/000-state-init.yaml.gotmpl -n
# Also check if there are other boolean defaults that might have the same issue
rg '\| default ' functions/render/000-state-init.yaml.gotmpl -nRepository: hops-ops/aws-secret-stack
Length of output: 1994
🌐 Web query:
Is hasKey a valid Sprig function in Go templates?
💡 Result:
Yes, hasKey is a valid Sprig function in Go templates. It checks if a dictionary (dict) contains a given key and returns true if the key exists, false otherwise.
Citations:
- 1: http://masterminds.github.io/sprig/dicts.html
- 2: https://github.com/Masterminds/sprig/blob/master/functions.go
🏁 Script executed:
# Verify hasKey syntax in Go templates and check if the proposed fix syntax is correct
rg 'hasKey' --type go --type template -A 2 -B 2 | head -30Repository: hops-ops/aws-secret-stack
Length of output: 101
🏁 Script executed:
# Search for hasKey usage in all files
rg 'hasKey' -nRepository: hops-ops/aws-secret-stack
Length of output: 51
🏁 Script executed:
# Check for conditional patterns with if statements in template files
rg 'if hasKey|if \(' functions/render/ -n -A 2Repository: hops-ops/aws-secret-stack
Length of output: 51
🏁 Script executed:
# Look for if statements in the template files to see the pattern used
rg '{{-.*if ' functions/render/ -n -A 2 | head -40Repository: hops-ops/aws-secret-stack
Length of output: 152
🏁 Script executed:
# Look for if statements in the template files
rg 'if ' functions/render/ -n -A 2 | head -40Repository: hops-ops/aws-secret-stack
Length of output: 2880
🏁 Script executed:
# Search for any existing hasKey usage or similar patterns in the repository
find . -name "*.gotmpl" -o -name "*.tpl" | xargs rg -l "hasKey\|has " 2>/dev/null || echo "No matches found"
# Check if there are any assignment patterns like $var :=
rg '\$\w+ :=' functions/render/000-state-init.yaml.gotmpl -nRepository: hops-ops/aws-secret-stack
Length of output: 1138
Fix enabled: false cannot be set for secretStore and sops due to Sprig default filter behavior.
Lines 87 and 92 use | default true, which treats explicit false as empty and replaces it with true per Sprig semantics. This breaks the ability to disable these features.
Proposed fix
{{- $sopsSpec := $spec.sops | default dict }}
{{- $secretStoreSpec := $spec.secretStore | default dict }}
+{{- $secretStoreEnabled := true }}
+{{- if hasKey $secretStoreSpec "enabled" }}
+ {{- $secretStoreEnabled = $secretStoreSpec.enabled }}
+{{- end }}
+{{- $sopsEnabled := true }}
+{{- if hasKey $sopsSpec "enabled" }}
+ {{- $sopsEnabled = $sopsSpec.enabled }}
+{{- end }}
{{- $state := dict
...
"secretStore" (dict
- "enabled" ($secretStoreSpec.enabled | default true)
+ "enabled" $secretStoreEnabled
"scope" ($secretStoreSpec.scope | default "Namespaced")
"name" ($secretStoreSpec.name | default "default")
)
"sops" (dict
- "enabled" ($sopsSpec.enabled | default true)
+ "enabled" $sopsEnabled
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/render/000-state-init.yaml.gotmpl` around lines 87 - 93, The
template uses ($secretStoreSpec.enabled | default true) and ($sopsSpec.enabled |
default true), but Sprig's default treats explicit false as empty; change these
to explicit presence checks so false is preserved: replace each use with an
if/else that checks hasKey on the spec (e.g. if hasKey $secretStoreSpec
"enabled" then use $secretStoreSpec.enabled else true end) and do the same for
$sopsSpec.enabled so an explicit false is honored.
|
|
||
| ## Usage | ||
|
|
||
| Minimal — installs ESO, PodIdentity, ClusterSecretStore, and SOPS KMS key: |
There was a problem hiding this comment.
Default scope is documented incorrectly in the minimal example description.
Line 16 says default includes ClusterSecretStore, but the default scope is Namespaced (so default resource is SecretStore).
📝 Suggested doc fix
-Minimal — installs ESO, PodIdentity, ClusterSecretStore, and SOPS KMS key:
+Minimal — installs ESO, PodIdentity, SecretStore (namespaced), and SOPS KMS key:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Minimal — installs ESO, PodIdentity, ClusterSecretStore, and SOPS KMS key: | |
| Minimal — installs ESO, PodIdentity, SecretStore (namespaced), and SOPS KMS key: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 16, The README's "Minimal" example incorrectly states the
default scope includes ClusterSecretStore; update the minimal example
description to reflect that the default scope is Namespaced and the default
resource is SecretStore (i.e., replace any mention of "ClusterSecretStore" with
"SecretStore" and, if present, explicitly note the default scope is Namespaced)
so the documentation matches the actual default behavior.
Comment on lines
+368
to
+403
| # Test 9: SecretStore disabled renders nothing | ||
| # ========================================================================== | ||
| metav1alpha1.CompositionTest { | ||
| metadata.name = "secret-store-disabled" | ||
| spec = { | ||
| compositionPath = "apis/secretstacks/composition.yaml" | ||
| xrdPath = "apis/secretstacks/definition.yaml" | ||
| timeoutSeconds = 60 | ||
| validate = False | ||
| xr = { | ||
| apiVersion = "aws.hops.ops.com.ai/v1alpha1" | ||
| kind = "SecretStack" | ||
| metadata.name = "no-store" | ||
| spec = { | ||
| clusterName = "my-cluster" | ||
| secretStore.enabled = False | ||
| sops.enabled = False | ||
| aws.region = "us-east-1" | ||
| } | ||
| } | ||
| observedResources = [ | ||
| { | ||
| apiVersion = "helm.hops.ops.com.ai/v1alpha1" | ||
| kind = "ExternalSecrets" | ||
| metadata = { | ||
| name = "no-store" | ||
| annotations = {"crossplane.io/composition-resource-name" = "helm-external-secrets"} | ||
| } | ||
| status.conditions = [ | ||
| {type = "Ready", status = "True"} | ||
| {type = "Synced", status = "True"} | ||
| ] | ||
| } | ||
| ] | ||
| # Only Helm + PodIdentity + Usage, no SecretStore, no KMS Key | ||
| assertResources = [ |
There was a problem hiding this comment.
Align Test 9 title/comment with actual assertions.
The test says “renders nothing” and mentions Usage, but it asserts Helm + PodIdentity. This is only a naming/comment mismatch, but it can confuse future readers.
✏️ Suggested wording fix
- # Test 9: SecretStore disabled renders nothing
+ # Test 9: SecretStore disabled skips SecretStore and KMS resources
...
- # Only Helm + PodIdentity + Usage, no SecretStore, no KMS Key
+ # Only Helm + PodIdentity should render; no SecretStore, Usage, or KMS Key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test-render/main.k` around lines 368 - 403, The test header/comment for
metav1alpha1.CompositionTest with metadata.name = "secret-store-disabled" is
misleading (says “renders nothing” and mentions `Usage`) while the
assertResources actually expect Helm + PodIdentity; update the title/comment to
accurately describe the expected result (e.g., "SecretStore disabled renders
Helm + PodIdentity" or similar) and remove or correct the stray reference to
`Usage` so the comment matches the assertions in the test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test-render/main.k (1)
168-222: Add coverage for theUsagerender path.The new SecretStore cases only assert the
Object. None of them setobserved.secretStore.readyand verify theUsageemitted byfunctions/render/230-secret-store.yaml.gotmpl, so the delete-order branch can regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-render/main.k` around lines 168 - 222, The test misses asserting the Usage render path: update the "secret-store-default-namespaced" CompositionTest (metav1alpha1.CompositionTest) to include an observed resource entry that sets observed.secretStore.ready = True (i.e., simulate the SecretStore being ready) and add an assertResources entry verifying the Usage resource emitted by functions/render/230-secret-store.yaml.gotmpl (the Usage object and its expected fields/owner reference), so the test covers the delete-order branch and the Usage render path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/render/230-secret-store.yaml.gotmpl`:
- Line 12: The template currently gates rendering of the SecretStore on
$ss.enabled and $state.observed.helmExternalSecrets.ready which causes
long‑lived SecretStore resources to be dropped when ESO is transiently NotReady;
change the condition so SecretStore rendering is gated by CRD availability (e.g.
a CRD-ready flag in $state.observed.helmExternalSecrets or by presence of the
SecretStore object) instead of the transient ready condition, and ensure the
Usage resource only waits for CRD availability (not ESO Ready) so the
delete-order guard is present during create; update the conditional around
$ss.enabled/$state.observed.helmExternalSecrets.ready and the Usage gating logic
to reference the CRD-ready/existence check while leaving SecretStore rendering
once the object exists.
---
Nitpick comments:
In `@tests/test-render/main.k`:
- Around line 168-222: The test misses asserting the Usage render path: update
the "secret-store-default-namespaced" CompositionTest
(metav1alpha1.CompositionTest) to include an observed resource entry that sets
observed.secretStore.ready = True (i.e., simulate the SecretStore being ready)
and add an assertResources entry verifying the Usage resource emitted by
functions/render/230-secret-store.yaml.gotmpl (the Usage object and its expected
fields/owner reference), so the test covers the delete-order branch and the
Usage render path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 226227a5-f879-4f50-a265-0e701f174898
📒 Files selected for processing (2)
functions/render/230-secret-store.yaml.gotmpltests/test-render/main.k
|
|
||
| {{- $ss := $state.secretStore }} | ||
|
|
||
| {{- if and $ss.enabled $state.observed.helmExternalSecrets.ready }} |
There was a problem hiding this comment.
Don't key long-lived resources off transient Ready conditions.
Line 12 drops the SecretStore from desired state whenever ESO is temporarily NotReady, so a chart upgrade or sync blip can make Crossplane delete it even though the CRDs still exist. Line 78 also delays the Usage until the store is Ready, which leaves the delete-order guard absent during creation. Gate initial creation on CRD availability, but keep rendering once the SecretStore object exists.
Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/render/230-secret-store.yaml.gotmpl` at line 12, The template
currently gates rendering of the SecretStore on $ss.enabled and
$state.observed.helmExternalSecrets.ready which causes long‑lived SecretStore
resources to be dropped when ESO is transiently NotReady; change the condition
so SecretStore rendering is gated by CRD availability (e.g. a CRD-ready flag in
$state.observed.helmExternalSecrets or by presence of the SecretStore object)
instead of the transient ready condition, and ensure the Usage resource only
waits for CRD availability (not ESO Ready) so the delete-order guard is present
during create; update the conditional around
$ss.enabled/$state.observed.helmExternalSecrets.ready and the Usage gating logic
to reference the CRD-ready/existence check while leaving SecretStore rendering
once the object exists.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2etest-secretstacks/main.k (1)
218-222: Minor syntax inconsistency.Line 221 uses colon (
:) for property assignment while adjacent lines use equals (=). KCL accepts both, but consistent style improves readability.🔧 Suggested fix for consistency
tags = { "e2etest" = "true" "test-run" = _now - "repo": "https://github.com/hops-ops/aws-secret-stack" + "repo" = "https://github.com/hops-ops/aws-secret-stack" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2etest-secretstacks/main.k` around lines 218 - 222, The tags map has a mixed assignment style: two entries use = while the "repo" entry uses :, so update the "tags" mapping to use consistent equals assignment for the "repo" key (i.e., change the "repo": "https://github.com/hops-ops/aws-secret-stack" entry to use =) to match the other keys in the tags map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2etest-secretstacks/main.k`:
- Around line 218-222: The tags map has a mixed assignment style: two entries
use = while the "repo" entry uses :, so update the "tags" mapping to use
consistent equals assignment for the "repo" key (i.e., change the "repo":
"https://github.com/hops-ops/aws-secret-stack" entry to use =) to match the
other keys in the tags map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c17c9681-b647-4276-a1e2-804a81cf1bf9
📒 Files selected for processing (3)
.github/workflows/on-pr.yaml.github/workflows/on-push-main.yamltests/e2etest-secretstacks/main.k
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/on-pr.yaml
- .github/workflows/on-push-main.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores