Skip to content

test: EKS add-on support #601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ThirdEyeSqueegee wants to merge 5 commits into
base: main
Choose a base branch
from
Open

test: EKS add-on support #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8dc3f6
test: EKS add-on support
ThirdEyeSqueegee Apr 13, 2026
337e6d1
fix: flag parsing, use `jq` for clusterrole name
ThirdEyeSqueegee Apr 13, 2026
7c7c822
Merge branch 'main' into addon-test
simonmarty Jul 28, 2026
efc75d6
Merge branch 'main' into addon-test
simonmarty Jul 30, 2026
24c4458
Merge branch 'main' into addon-test
ThirdEyeSqueegee Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,10 @@ aws iam attach-role-policy \
- `./run-tests.sh x64-pod-identity` will run only x64 Pod Identity tests
- `./run-tests.sh arm-irsa` will run only ARM IRSA tests
- `./run-tests.sh arm-pod-identity` will run only ARM Pod Identity tests

### Running tests with the EKS managed addon

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
Comment on lines +51 to +54

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Three doc accuracy issues.

  1. "does not require PRIVREPO" is not true as shipped — integration.bats.template:346-351 (and teardown() at L396-401) still consume $PRIVREPO, $PRIVTAG and ghcr-secret with no addon guard. Someone following these instructions gets a failed run. Accurate once the guards from my comment on L134 are added.

  2. --addon x64-pod-identity still needs POD_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".

  3. Worth stating what --addon does and doesn't validate. aws eks create-addon has 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 --addon run 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.

8 changes: 8 additions & 0 deletions tests/addon_config_values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
secrets-store-csi-driver:
enableSecretRotation: true
rotationPollInterval: "15s"
syncSecret:
enabled: true
tokenRequests:
- audience: "sts.amazonaws.com"
- audience: "pods.eks.amazonaws.com"
Comment on lines +1 to +8

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

Suggested change
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.

7 changes: 6 additions & 1 deletion tests/generate-test-files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,12 @@ def get_auth_setup(arch: str, auth_type: str) -> str:
--approve \\
--region $REGION"""

return f""" log "Creating EKS Pod Identity addon"
return f""" if [[ -z "${{POD_IDENTITY_ROLE_ARN}}" ]]; then
echo "Error: POD_IDENTITY_ROLE_ARN is not set" >&2
return 1
fi
Comment on lines +235 to +238

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
fi

That 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".


log "Creating EKS Pod Identity addon"
eksctl create addon --name eks-pod-identity-agent --cluster $CLUSTER_NAME --region $REGION

log "Creating Pod Identity association"
Expand Down
65 changes: 41 additions & 24 deletions tests/integration.bats.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,25 @@ log() {
echo -e "${{{LOG_COLOR}}}[$TIMESTAMP] [{{ARCH}}-{{AUTH_TYPE}}] $msg${NC}" >&3
}

if [[ -z "${PRIVREPO}" ]]; then
echo "Error: PRIVREPO is not specified" >&2
return 1
fi

# GHCR pull credentials are only required when running inside GitHub Actions,
# where the provider image lives in a private GHCR repo. Local runs pull from
# whatever PRIVREPO points at and skip the GHCR pull-secret entirely.
if [[ "${GITHUB_ACTIONS}" == "true" ]]; then
if [[ -z "${GITHUB_ACTOR}" ]]; then
echo "Error: GITHUB_ACTOR is not specified" >&2
if [[ "${INSTALL_METHOD}" != "addon" ]]; then
if [[ -z "${PRIVREPO}" ]]; then
echo "Error: PRIVREPO is not specified" >&2
return 1
fi

if [[ -z "${GITHUB_TOKEN}" ]]; then
echo "Error: GITHUB_TOKEN is not specified" >&2
return 1
# GHCR pull credentials are only required when running inside GitHub Actions,
# where the provider image lives in a private GHCR repo. Local runs pull from
# whatever PRIVREPO points at and skip the GHCR pull-secret entirely.
if [[ "${GITHUB_ACTIONS}" == "true" ]]; then
if [[ -z "${GITHUB_ACTOR}" ]]; then
echo "Error: GITHUB_ACTOR is not specified" >&2
return 1
fi

if [[ -z "${GITHUB_TOKEN}" ]]; then
echo "Error: GITHUB_TOKEN is not specified" >&2
return 1
fi
fi
fi

Expand Down Expand Up @@ -66,17 +68,25 @@ setup_file() {

{{AUTH_SETUP}}

if [[ "${GITHUB_ACTIONS}" == "true" ]]; then
log "Create GHCR secret"
KUBECONFIG=${{KUBECONFIG_VAR}} kubectl create secret docker-registry ghcr-secret --docker-server=ghcr.io --docker-username=$GITHUB_ACTOR --docker-password=$GITHUB_TOKEN -n $NAMESPACE
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
Comment on lines +71 to +75

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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=120s

3. 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.

else
# GHCR pull-secret is only needed inside GitHub Actions (private GHCR repo).
if [[ "${GITHUB_ACTIONS}" == "true" ]]; then
log "Create GHCR secret"
KUBECONFIG=${{KUBECONFIG_VAR}} kubectl create secret docker-registry ghcr-secret --docker-server=ghcr.io --docker-username=$GITHUB_ACTOR --docker-password=$GITHUB_TOKEN -n $NAMESPACE
fi

log "Adding secrets-store-csi-driver Helm repository"
helm repo add secrets-store-csi-driver https://kubernetes-sigs.github.io/secrets-store-csi-driver/charts

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

@simonmarty simonmarty Apr 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi

log "Adding secrets-store-csi-driver Helm repository"
helm repo add secrets-store-csi-driver https://kubernetes-sigs.github.io/secrets-store-csi-driver/charts

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 "Cluster setup completed for $CLUSTER_NAME"
}

Expand Down Expand Up @@ -121,6 +131,10 @@ validate_jmes_mount() {
}

@test "Install aws provider" {
if [[ "${INSTALL_METHOD}" == "addon" ]]; then
skip "Provider installed as EKS addon"
fi
Comment on lines +134 to +136

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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" — runs helm upgrade -n $NAMESPACE csi-secrets-store-provider-aws ... unconditionally.
  • L391-402, teardown() — repeats the same helm upgrade to restore driverWritesSecrets=false.

In --addon mode all four things they depend on are absent:

  1. No csi-secrets-store-provider-aws Helm release exists (this test is skipped) → Error: UPGRADE FAILED: "csi-secrets-store-provider-aws" has no deployed releases.
  2. helm package --dependency-update at L140 never ran, and charts/secrets-store-csi-driver-provider-aws/charts/ is not vendored (I checked — only Chart.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.
  3. $PRIVREPO/$PRIVTAG are empty — the new guard at L24 stops enforcing PRIVREPO, and tests/README.md:51 says it isn't needed. --set image.tag= renders image: <repo>:, exactly the footgun the comment at L148-152 guards against for the Helm path.
  4. ghcr-secret is only created inside the else at 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 "Installing AWS provider"

KUBECONFIG=${{KUBECONFIG_VAR}} helm package --dependency-update ../charts/secrets-store-csi-driver-provider-aws
Expand Down Expand Up @@ -149,8 +163,11 @@ validate_jmes_mount() {
@test "Verify serviceaccounts/token create permission is not granted to provider" {
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)

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. No match → PROVIDER_CLUSTER_ROLE is empty. L170 degrades to kubectl get clusterrole -o json, which returns a kind: List whose .rules is null, so jq -e '.rules[]' aborts with Cannot iterate over null (exit 5). Fails safe, but the message points at jq rather than at a missing role.
  2. Multiple matches → head -1 silently picks the first in API order. If any other ClusterRole matches provider.*cluster-role (a stale release, a second provider) the serviceaccounts/token check 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.
  3. The assignment is a pipeline ending in head, so exit status is head'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:

Suggested change
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.


# Check the provider's own ClusterRole for serviceaccounts/token create permission
run bash -c "kubectl --kubeconfig=${{KUBECONFIG_VAR}} -n $NAMESPACE get clusterrole csi-secrets-store-provider-aws-secrets-store-csi-driver-provide-cluster-role -o json | jq -e '[.rules[] | select((.resources[]? == \"serviceaccounts/token\") and (.verbs[]? == \"create\"))] | length == 0'"
run bash -c "kubectl --kubeconfig=${{KUBECONFIG_VAR}} -n $NAMESPACE get clusterrole $PROVIDER_CLUSTER_ROLE -o json | jq -e '[.rules[] | select((.resources[]? == \"serviceaccounts/token\") and (.verbs[]? == \"create\"))] | length == 0'"
assert_success

log "Verified: Provider ClusterRole does not grant serviceaccounts/token create permission"
Expand Down
29 changes: 29 additions & 0 deletions tests/run-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,35 @@ delete_cluster() {
eksctl delete cluster --name $1 --parallel 25
}

# Parse --addon and --addon-version flags from any position
REMAINING_ARGS=()
while [[ $# -gt 0 ]]; do
case "$1" in
--addon)
export INSTALL_METHOD=addon
shift
;;
--addon-version)
Comment thread
ThirdEyeSqueegee marked this conversation as resolved.
if [[ -z "$2" || "$2" == --* ]]; then
echo "Error: --addon-version requires a value" >&2
exit 1
fi
export ADDON_VERSION="$2"
shift 2
;;
*)
REMAINING_ARGS+=("$1")
shift
;;
Comment on lines +57 to +60

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 $?
0

In 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:

Suggested change
*)
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.

esac
done
set -- "${REMAINING_ARGS[@]}"

if [[ -n "$ADDON_VERSION" && "$INSTALL_METHOD" != "addon" ]]; then

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

echo "Error: --addon-version requires --addon" >&2
exit 1
fi
Comment on lines +65 to +68

@secrets-manager-ai-code-review secrets-manager-ai-code-review Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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_VERSION exported in the caller's shell, no --addon → this block exit 1s. Because it sits before the clean branch at L70, that kills ./run-tests.sh clean x64-irsa — i.e. .github/workflows/integ.yml:135, the if: 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=addon inherited → the untouched CI invocation ./run-tests.sh x64-irsa silently 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:

Suggested change
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.


if [[ "$1" == "clean" ]]; then
cleanup
echo "Cleaning up secrets and parameters for all configs..."
Expand Down