test: EKS add-on support - #601
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
=======================================
Coverage 61.62% 61.62%
=======================================
Files 11 11
Lines 740 740
=======================================
Hits 456 456
Misses 265 265
Partials 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ThirdEyeSqueegee
added
the
safe-to-test
| log "Installing secrets-store-csi-driver via Helm" | ||
| KUBECONFIG=${{KUBECONFIG_VAR}} helm --namespace=$NAMESPACE install --wait --wait-for-jobs --timeout=30s csi-secrets-store secrets-store-csi-driver/secrets-store-csi-driver --set enableSecretRotation=true --set rotationPollInterval=15s --set syncSecret.enabled=true --set tokenRequests[0].audience=sts.amazonaws.com --set tokenRequests[1].audience=pods.eks.amazonaws.com | ||
| log "Installing secrets-store-csi-driver via Helm" | ||
| KUBECONFIG=${{KUBECONFIG_VAR}} helm --namespace=$NAMESPACE install --wait --wait-for-jobs --timeout=30s csi-secrets-store secrets-store-csi-driver/secrets-store-csi-driver --set enableSecretRotation=true --set rotationPollInterval=15s --set syncSecret.enabled=true --set tokenRequests[0].audience=sts.amazonaws.com --set tokenRequests[1].audience=pods.eks.amazonaws.com |
There was a problem hiding this comment.
Nit, for a followup, (you don't have to do it in this PR, and you don't have to be the one to do it):
We might as well install the provider here through the helm chart and install the driver as a Helm dependency instead of in two separate steps. We just have to pass the config options we're passing here to the underlying driver chart. That way the addon and the provider flows are more similar.
github-actions
Bot
removed
the
safe-to-test
ThirdEyeSqueegee
added
the
safe-to-test
github-actions
Bot
removed
the
safe-to-test
ThirdEyeSqueegee
added
the
safe-to-test
e443af1 to
337e6d1
Compare
github-actions
Bot
removed
the
safe-to-test
ThirdEyeSqueegee
added
the
safe-to-test
github-actions
Bot
removed
the
safe-to-test
|
Claude finished @simonmarty's task in 2m 16s —— View job Review: EKS add-on test support
I've reviewed the changes and left a couple of inline comments. Overall this is a clean, well-scoped addition and the flag-parsing rework nicely addresses the earlier out-of-order-args feedback. One likely bug worth confirming before merge. 🔴 Likely bug:
|
![secrets-manager-ai-code-review[bot]](https://avatars.githubusercontent.com/in/4081740?s=60&v=4){kind=small}
| done | ||
| set -- "${REMAINING_ARGS[@]}" | ||
|
|
||
| if [[ -n "$ADDON_VERSION" && "$INSTALL_METHOD" != "addon" ]]; then |
There was a problem hiding this comment.
👍 This correctly resolves @simonmarty's earlier note — flags are now parsed from any position and --addon-version ... --addon (out of order) is accepted before the --addon-version requires --addon check runs. Nice.
One small note: INSTALL_METHOD and ADDON_VERSION are exported here so the generated .bats files inherit them. That works because bats is invoked as a child process later in this same script, but it does mean these leak into the caller's environment if the script is ever sourced. Given it's #!/bin/bash and always run as a subprocess, this is fine as-is.
| log "Verifying provider ClusterRole does not grant serviceaccounts/token create permission" | ||
|
|
||
| # Find the provider ClusterRole (name varies between Helm and addon installs) | ||
| PROVIDER_CLUSTER_ROLE=$(kubectl --kubeconfig=${{KUBECONFIG_VAR}} get clusterrole -o json | jq -r '.items[].metadata.name | select(test("provider.*cluster-role"))' | head -1) |
There was a problem hiding this comment.
Minor robustness: if the select(test("provider.*cluster-role")) filter matches nothing, PROVIDER_CLUSTER_ROLE is empty and the next command degrades to kubectl get clusterrole -o json (all roles). That output is {items: [...]} rather than {rules: [...]}, so the jq '.rules[]' errors and the test fails — fail-safe, but with a confusing message. Consider asserting the role was found first:
[[ -n "$PROVIDER_CLUSTER_ROLE" ]] || fail "Could not locate provider ClusterRole"| log "Verifying provider ClusterRole does not grant serviceaccounts/token create permission" | ||
|
|
||
| # Find the provider ClusterRole (name varies between Helm and addon installs) | ||
| PROVIDER_CLUSTER_ROLE=$(kubectl --kubeconfig=${{KUBECONFIG_VAR}} get clusterrole -o json | jq -r '.items[].metadata.name | select(test("provider.*cluster-role"))' | head -1) |
There was a problem hiding this comment.
Minor robustness: if the select(test("provider.*cluster-role")) filter matches nothing, PROVIDER_CLUSTER_ROLE is empty and the next command becomes kubectl get clusterrole -o json (all roles), whose top-level shape is {items: [...]} rather than {rules: [...]} — so the jq '.rules[]' will error and the test fails rather than silently passing. That's fail-safe, but the failure message won't point at the root cause. Consider asserting the role was found first, e.g.:
[[ -n "$PROVIDER_CLUSTER_ROLE" ]] || fail "Could not locate provider ClusterRole"
simonmarty
left a comment
simonmarty
left a comment
There was a problem hiding this comment.
Could you address the AI code review comments?
GitHub Actions
Inactive
|
Claude finished @ThirdEyeSqueegee's task in 45m 17s —— View job Review: EKS add-on test support
@simonmarty asked for the earlier AI review comments to be addressed. Checking the current head ( 🔴 BlockingAddon mode cannot reach a green run. The RBAC security assertion can pass vacuously. 🟡 Worth addressing
Notes, not blockers
🟢 Looks good
Caveat on method: this is a static review. I couldn't run |
| if [[ "${INSTALL_METHOD}" == "addon" ]]; then | ||
| skip "Provider installed as EKS addon" | ||
| fi |
There was a problem hiding this comment.
🔴 Blocking: this is the only Helm-dependent test that got the guard — addon mode can't pass.
Two other Helm-only blocks were missed (both outside the diff, so I can't anchor there):
- L343-352,
@test "CSI inline volume test with driverWritesSecrets enabled"— runshelm upgrade -n $NAMESPACE csi-secrets-store-provider-aws ...unconditionally. - L391-402,
teardown()— repeats the samehelm upgradeto restoredriverWritesSecrets=false.
In --addon mode all four things they depend on are absent:
- No
csi-secrets-store-provider-awsHelm release exists (this test is skipped) →Error: UPGRADE FAILED: "csi-secrets-store-provider-aws" has no deployed releases. helm package --dependency-updateat L140 never ran, andcharts/secrets-store-csi-driver-provider-aws/charts/is not vendored (I checked — onlyChart.lock,Chart.yaml,templates,values.yaml), so helm also fails dependency resolution:found in Chart.yaml, but missing in charts/ directory: secrets-store-csi-driver.$PRIVREPO/$PRIVTAGare empty — the new guard at L24 stops enforcingPRIVREPO, andtests/README.md:51says it isn't needed.--set image.tag=rendersimage: <repo>:, exactly the footgun the comment at L148-152 guards against for the Helm path.ghcr-secretis only created inside theelseat L78-81, but L351 attaches it unconditionally.
Given the PR description says all tests passed under ./run-tests.sh --addon x64, this run very likely didn't reach L343 — worth re-verifying end-to-end.
Rather than a third copy of the inline if, a shared guard in tests/helpers.bash (already loaded at L3) makes the omission visible and gives the next Helm-only test something to copy:
skip_if_addon() {
[[ "${INSTALL_METHOD}" == "addon" ]] && skip "provider installed as EKS addon"
}Alternatively, toggle driverWritesSecrets through addon_config_values.yaml + aws eks update-addon so addon mode keeps that coverage instead of losing it.
Separately: skipping this test also drops the only assertion that the provider actually rolled out (L156-158) — see my comment on L71.
| log "Verifying provider ClusterRole does not grant serviceaccounts/token create permission" | ||
|
|
||
| # Find the provider ClusterRole (name varies between Helm and addon installs) | ||
| PROVIDER_CLUSTER_ROLE=$(kubectl --kubeconfig=${{KUBECONFIG_VAR}} get clusterrole -o json | jq -r '.items[].metadata.name | select(test("provider.*cluster-role"))' | head -1) |
There was a problem hiding this comment.
🔴 The lookup can silently make this security assertion vacuous. (Re-raising @simonmarty's jq note and the earlier bot comment — the guard still isn't there.)
Three failure modes, none of which report the real cause:
- No match →
PROVIDER_CLUSTER_ROLEis empty. L170 degrades tokubectl get clusterrole -o json, which returns akind: Listwhose.rulesis null, sojq -e '.rules[]'aborts withCannot iterate over null(exit 5). Fails safe, but the message points at jq rather than at a missing role. - Multiple matches →
head -1silently picks the first in API order. If any other ClusterRole matchesprovider.*cluster-role(a stale release, a second provider) theserviceaccounts/tokencheck runs against the wrong object and passes green while the real provider role grants the permission. That's a false pass on a privilege-escalation regression test. - The assignment is a pipeline ending in
head, so exit status ishead's — a kubectl failure never trips bats' errexit; the variable just comes back empty and you land in case 1.
The chart already gives you a name-independent handle: provider.labels (_helpers.tpl) stamps app: secrets-store-csi-driver-provider-aws and app.kubernetes.io/name onto the ClusterRole via rbac.yaml:20-22, and this file already uses that selector idiom at L156/L355. That replaces both the regex and head -1:
| PROVIDER_CLUSTER_ROLE=$(kubectl --kubeconfig=${{KUBECONFIG_VAR}} get clusterrole -o json | jq -r '.items[].metadata.name | select(test("provider.*cluster-role"))' | head -1) | |
| # Find the provider ClusterRole (name varies between Helm and addon installs) | |
| PROVIDER_CLUSTER_ROLE=$(kubectl --kubeconfig=${{KUBECONFIG_VAR}} get clusterrole -l app=secrets-store-csi-driver-provider-aws -o jsonpath="{.items[0].metadata.name}") | |
| [[ -n "$PROVIDER_CLUSTER_ROLE" ]] || fail "Could not locate provider ClusterRole" |
If you'd rather keep the regex, the [[ -n ... ]] || fail ... line alone fixes case 1 and 3; case 2 still needs a uniqueness check.
Nit while you're here: the -n $NAMESPACE retained on L170 is a no-op — ClusterRole is cluster-scoped, and the new L167 correctly omits it.
| if [[ "${INSTALL_METHOD}" == "addon" ]]; then | ||
| log "Installing aws-secrets-store-csi-driver-provider as EKS addon" | ||
| aws eks create-addon --cluster-name $CLUSTER_NAME --addon-name aws-secrets-store-csi-driver-provider \ | ||
| --configuration-values file://addon_config_values.yaml ${ADDON_VERSION:+--addon-version $ADDON_VERSION} --region $REGION | ||
| aws eks wait addon-active --cluster-name $CLUSTER_NAME --addon-name aws-secrets-store-csi-driver-provider --region $REGION |
There was a problem hiding this comment.
🟡 Three gaps in the addon install branch relative to the Helm branch it replaces.
1. Nothing installs or verifies the CSI driver. The else branch at L87 is the only thing that installs secrets-store-csi-driver, and it's skipped here. Addon mode relies entirely on the published add-on vendoring the driver as a subchart and leaving secrets-store-csi-driver.install at its default true (Chart.yaml gates the dep on that condition). If the EKS build strips or flips it — plausible, since EKS ships the driver as its own add-on — nothing installs the driver and the first symptom is @test "secretproviderclasses crd is established" (L179) timing out at 60s, followed by every mount/rotation/sync test failing with no pointer at the root cause. Worth setting secrets-store-csi-driver.install: true explicitly in addon_config_values.yaml so the intent is asserted rather than inherited.
2. No readiness gate. The Helm path had helm install --wait --wait-for-jobs plus the explicit provider-pod assertion at L156-158 — and that assertion is inside the test you just skipped. aws eks wait addon-active reflects EKS control-plane add-on status, not that the provider DaemonSet is Ready on all 3 nodes. The next assertions have hard 60s budgets (L179, L201), so a slow rollout on a fresh 3-node cluster surfaces as an intermittent mount timeout. Mirroring what the skipped test provided would fix it:
kubectl --kubeconfig=${{KUBECONFIG_VAR}} -n $NAMESPACE rollout status daemonset -l app=secrets-store-csi-driver-provider-aws --timeout=120s3. create-addon isn't re-entrant. No --resolve-conflicts, so a re-run against a cluster whose teardown_file didn't complete gets ResourceInUseException and aborts setup_file, failing all tests with a confusing error. And if the add-on lands in a stuck CREATING/DEGRADED, the waiter blocks for its full default (60 × 10s = 10 min) on a live billing cluster and then reports a bare Max attempts exceeded — echoing aws eks describe-addon on failure would make that diagnosable.
Also note (not a defect, but worth stating in the README — see my comment there): create-addon has no image override, so --addon validates the published add-on image, not $PRIVREPO:$PRIVTAG from the branch under test.
| *) | ||
| REMAINING_ARGS+=("$1") | ||
| shift | ||
| ;; |
There was a problem hiding this comment.
🟡 The catch-all makes a typo'd flag a silent green run.
Unrecognized flags fall into REMAINING_ARGS and become $1. The dispatch chain at L96-137 has no else, so nothing matches, bats is never invoked, bats_exit_code stays at its initialized 0, and L141 exits 0 having run zero tests:
$ ./run-tests.sh --addon-verison v1 x64-irsa # or --addons, or --addon=true
Generating test files...
Test files generated successfully
Cleaning up...
$ echo $?
0In CI that's a green integration-tests check on a build where the suite never ran. The =-joined form (--addon-version=v1) hits this too, since the case only handles the space-separated form.
This state was unreachable before — adding flags is what makes it easy to hit. Rejecting unknown options here is half the fix:
| *) | |
| REMAINING_ARGS+=("$1") | |
| shift | |
| ;; | |
| -*) | |
| echo "Error: unknown option: $1" >&2 | |
| exit 1 | |
| ;; | |
| *) | |
| REMAINING_ARGS+=("$1") | |
| shift | |
| ;; |
The other half is an else on the dispatch chain (L137) that errors on an unrecognized target, e.g. else echo "Error: unknown target: $1" >&2; exit 1; fi.
| if [[ -n "$ADDON_VERSION" && "$INSTALL_METHOD" != "addon" ]]; then | ||
| echo "Error: --addon-version requires --addon" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🟡 This reads the ambient environment, not just the flags parsed above.
Neither variable is initialized before the loop, so inherited values are indistinguishable from flags:
ADDON_VERSIONexported in the caller's shell, no--addon→ this blockexit 1s. Because it sits before thecleanbranch at L70, that kills./run-tests.sh clean x64-irsa— i.e..github/workflows/integ.yml:135, theif: always()step whose only job is deleting the EKS cluster and the secrets/parameters after a failed run. A stray env var turns an ordinary test failure into a leaked 3-node cluster plus secrets in two regions.INSTALL_METHOD=addoninherited → the untouched CI invocation./run-tests.sh x64-irsasilently takes the addon path with no flag and no log line.
Fix is to parse into locals and export only what the flags actually set:
| if [[ -n "$ADDON_VERSION" && "$INSTALL_METHOD" != "addon" ]]; then | |
| echo "Error: --addon-version requires --addon" >&2 | |
| exit 1 | |
| fi | |
| if [[ -n "$addon_version" && "$install_method" != "addon" ]]; then | |
| echo "Error: --addon-version requires --addon" >&2 | |
| exit 1 | |
| fi | |
| export INSTALL_METHOD="$install_method" | |
| [[ -n "$addon_version" ]] && export ADDON_VERSION="$addon_version" |
(with install_method= / addon_version= initialized before the loop and the case arms assigning those instead of exporting). Moving the check after the clean branch would also keep cleanup reachable regardless.
Minor, optional: --addon-version X has exactly one meaning, so having that arm also set addon mode would let this validation block go away entirely.
| return f""" if [[ -z "${{POD_IDENTITY_ROLE_ARN}}" ]]; then | ||
| echo "Error: POD_IDENTITY_ROLE_ARN is not set" >&2 | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🟡 Good check, but it fires ~15-20 minutes and one EKS cluster too late.
This lands at the {{AUTH_SETUP}} substitution point, which is integration.bats.template:69 — inside setup_file, after create-secrets (L54) and after eksctl create cluster --nodes 3 (L62-67). Every other required-env guard (PRIVREPO, GITHUB_ACTOR, GITHUB_TOKEN, template L24-44) is deliberately at file scope so it aborts before any AWS call.
So ./run-tests.sh x64-pod-identity with the var unset (local run, or a fork where secrets.POD_IDENTITY_ROLE_ARN is empty) first creates the secrets/parameters in two regions, then waits out a full 3-node cluster build, and only then prints the error. Cluster reclamation then depends on teardown_file running after a failed setup_file; in CI the if: always() cleanup step covers it, locally nothing does.
Since auth_type is already known at generation time, this belongs in the template's guard block instead — e.g. a {{REQUIRED_ENV_GUARD}} substitution alongside {{AUTH_SETUP}}, or a plain gate in the template:
if [[ "{{AUTH_TYPE}}" == "pod-identity" && -z "${POD_IDENTITY_ROLE_ARN}" ]]; then
echo "Error: POD_IDENTITY_ROLE_ARN is not specified" >&2
return 1
fiThat also gets the shell out of the f-string (no ${{...}} brace-doubling to reason about) and keeps all env validation in one greppable place. Two nits if it stays here: the new block is tab-indented while the adjacent generated eksctl lines use 4 spaces, and the message says "is not set" where the existing guards say "is not specified".
| secrets-store-csi-driver: | ||
| enableSecretRotation: true | ||
| rotationPollInterval: "15s" | ||
| syncSecret: | ||
| enabled: true | ||
| tokenRequests: | ||
| - audience: "sts.amazonaws.com" | ||
| - audience: "pods.eks.amazonaws.com" |
There was a problem hiding this comment.
🟡 Two things worth pinning down here.
1. Add install: true explicitly. This file is the only thing configuring the driver in addon mode, but it never asserts the driver is installed at all — that depends on the published add-on's baked-in default for the secrets-store-csi-driver.install condition (charts/.../Chart.yaml:11-15). Making it explicit costs nothing and turns an inherited assumption into a stated one:
| secrets-store-csi-driver: | |
| enableSecretRotation: true | |
| rotationPollInterval: "15s" | |
| syncSecret: | |
| enabled: true | |
| tokenRequests: | |
| - audience: "sts.amazonaws.com" | |
| - audience: "pods.eks.amazonaws.com" | |
| secrets-store-csi-driver: | |
| install: true | |
| enableSecretRotation: true | |
| rotationPollInterval: "15s" | |
| syncSecret: | |
| enabled: true | |
| tokenRequests: | |
| - audience: "sts.amazonaws.com" | |
| - audience: "pods.eks.amazonaws.com" |
Related: EKS validates --configuration-values against the schema from DescribeAddonConfiguration and rejects unknown keys, so if the published add-on doesn't expose secrets-store-csi-driver.*, create-addon fails with InvalidParameterException after the cluster is already up. A quick aws eks describe-addon-configuration --addon-name aws-secrets-store-csi-driver-provider --addon-version <v> would confirm this file's shape against the real schema — cheap to check, and it's the assumption the whole addon path rests on.
2. Drift risk (follow-up, not this PR). All five settings now exist in two syntaxes with nothing linking them: this YAML and the --set chain at integration.bats.template:87. The tokenRequests pair is also already declared verbatim in charts/secrets-store-csi-driver-provider-aws/values.yaml. The rotation tests hard-wire wait_for_process 240 5 against the 15s poll interval, so bumping it in one place and not the other makes the two lanes exercise different driver configs — a rotation test that passes in one mode and fails in the other for a config reason that looks like a product regression. This is @simonmarty's earlier suggestion (install the driver as a Helm dependency of the provider chart so both flows share one config source); agreed it's a follow-up.
The tokenRequests shape itself is correct — a list of two single-key maps is exactly what --set tokenRequests[0].audience=... tokenRequests[1].audience=... produces.
| To install the provider as an EKS managed addon instead of via Helm, use the `--addon` flag. This does not require `PRIVREPO`, `GITHUB_ACTOR`, or `GITHUB_TOKEN`. | ||
|
|
||
| - `./run-tests.sh --addon x64-irsa` will install the provider as an EKS addon and run x64 IRSA tests | ||
| - `./run-tests.sh --addon --addon-version v2.2.2-eksbuild.2 x64-irsa` will install a specific addon version |
There was a problem hiding this comment.
🟡 Three doc accuracy issues.
-
"does not require
PRIVREPO" is not true as shipped —integration.bats.template:346-351(andteardown()at L396-401) still consume$PRIVREPO,$PRIVTAGandghcr-secretwith no addon guard. Someone following these instructions gets a failed run. Accurate once the guards from my comment on L134 are added. -
--addon x64-pod-identitystill needsPOD_IDENTITY_ROLE_ARN, and as of this PR that's a hard failure rather than a downstream error (generate-test-files.py:236). Worth saying so here, since the section reads as "addon mode needs fewer env vars". -
Worth stating what
--addondoes and doesn't validate.aws eks create-addonhas no image override, so this path exercises whatever provider image the published add-on ships — not the build from the branch under test. That makes it a release/packaging test, and it can't substitute for the Helm job. Without that caveat a reader could reasonably assume a green--addonrun validated their change.
Also, the example pins v2.2.2-eksbuild.2 while the chart is at 3.1.2; a current version (or a note that --addon-version is optional and defaults to whatever EKS resolves) would age better.
2 participants
Description
Why is this change being made?
--addonand--addon-versionflags torun-tests.shWhat is changing?
--addonflag inrun-tests.shi.
--addon-versioncan be used to specify an add-on version to testaddon_config_values.yamlto pass config options to add-on for integ testsPOD_IDENTITY_ROLE_ARNis undefined when Pod Identity configs are selected for test runsRelated Links
N/A
Testing
How was this tested?
./run-tests --addon x64and./run-tests.sh x64 --addon --addon-version v3.0.0-eksbuild.1i. All tests pass
When testing locally, provide testing artifact(s):
Reviewee Checklist
Update the checklist after submitting the PR
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
Reviewer Checklist
All reviewers please ensure the following are true before reviewing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.