Repository navigation
Conversation
…stalls Adds hack/ark/test-e2e-jwt.sh (make ark-test-e2e-jwt), mirroring test-e2e.sh but using --set config.cyberark.serviceId/subdomain instead of a Secret. Asserts the agent-credentials Secret does not exist when ARK_DISCOVERY_API is unset, proving the install is genuinely Secret-free. Not wired into .github/workflows/tests.yaml: no CI-side Conjur JWT onboarding automation exists yet (see #839), and a fresh kind cluster's OIDC issuer won't match any pre-onboarded authenticator. Requires targeting an already-onboarded cluster via USE_EXISTING_CLUSTER=true.
wallrj-cyberark
left a comment
There was a problem hiding this comment.
I left nine comments. These three matter most:
- The check on line 143 that no Secret exists cannot fail.
- With the default
USE_EXISTING_CLUSTER=false, the script cannot pass. - On a reused cluster, the log check on line 153 can pass by reading the previous run's pod.
This branch also predates #838, so the script has not yet been run as committed.
| # Prove the point of this script: the agent must run without an | ||
| # agent-credentials Secret at all when ARK_DISCOVERY_API is unset. | ||
| if [[ -z "$ARK_DISCOVERY_API" ]]; then | ||
| if kubectl get secret agent-credentials --namespace "$NAMESPACE" &>/dev/null; then |
There was a problem hiding this comment.
This check cannot fail. Line 76 deletes agent-credentials, line 77 only recreates it when ARK_DISCOVERY_API is set, and the chart never creates that Secret. So the check only tests the script's own setup.
If the chart started requiring the Secret again, the pod would fail with CreateContainerConfigError and helm --wait on line 119 would fail first. The check would never run.
Please either remove it and correct the PR description, or replace it with something that tests the chart. For example, assert that every secretKeyRef in the running pod spec is optional: true.
|
|
||
| # Set to true to use an existing cluster, otherwise a new kind cluster will be created. | ||
| # Note: the cluster will not be deleted after the test completes. | ||
| : ${USE_EXISTING_CLUSTER:=false} |
There was a problem hiding this comment.
The header says a fresh kind cluster will not be trusted, but USE_EXISTING_CLUSTER still defaults to false. So make ark-test-e2e-jwt with no extra variables builds and pushes an image, creates a kind cluster, installs ESO and the agent, then fails at the 60-second log timeout on line 151 with no hint why.
Please fail early instead, and drop the kind create cluster branch on lines 70-72:
if [[ "${USE_EXISTING_CLUSTER:-}" != true ]]; then
echo "Set USE_EXISTING_CLUSTER=true and point kubectl at a cluster already onboarded in Conjur Cloud" >&2
exit 1
fi| # Parse logs as JSON using jq to ensure logs are all JSON formatted. | ||
| timeout 60 jq -n \ | ||
| 'inputs | if .msg | test("Data sent successfully") then . | halt_error(0) else . end' \ | ||
| <(kubectl logs deployments/disco-agent --namespace "${NAMESPACE}" --follow) |
There was a problem hiding this comment.
On a reused cluster this can pass without the new pod ever uploading anything.
kubectl logs deployments/disco-agent picks one pod, and prefers the one that has been ready longest. Straight after rollout status, that can be the previous run's pod, which is still terminating and whose log already contains Data sent successfully. jq then halts on that old line.
test-e2e.sh gets away with this because CI uses a fresh kind cluster. This script needs a reused one, so reruns are the normal case. Please save the ${RANDOM} from line 136 in a variable and follow the pod with that disco-agent.cyberark.cloud/test-id label.
| --namespace $NAMESPACE \ | ||
| --selector app.kubernetes.io/name=disco-agent \ | ||
| --output jsonpath={.items[*].metadata.name} \ | ||
| | xargs -I{} kubectl get --raw /api/v1/namespaces/$NAMESPACE/pods/{}:8081/proxy/metrics \ |
There was a problem hiding this comment.
{.items[*].metadata.name} prints all pod names on one line, and xargs -I{} passes the whole line as one argument. While the previous run's pod is still terminating, the URL becomes pods/disco-agent-old disco-agent-new:8081/proxy/metrics and this step fails. The pprof query on line 168 has the same problem.
Please query only the new pod, selected by the test-id label (see the comment on line 153).
| --set config.clusterDescription="A temporary cluster for E2E testing. Contact @wallrj-cyberark." \ | ||
| --set config.period=60s \ | ||
| --set config.cyberark.serviceId="$ARK_SERVICE_ID" \ | ||
| --set config.cyberark.subdomain="$ARK_SUBDOMAIN" \ |
There was a problem hiding this comment.
This branch was cut from da60d34, before #838 merged. On this branch's head the chart has no config.cyberark.subdomain, and values.schema.json sets additionalProperties: false on config.cyberark, so Helm rejects this flag.
The merge into master will work, but it means nobody has run the script as committed. The test plan only covers bash -n and make -n. Please rebase on master and add one live run against an onboarded cluster to the description.
| # TODO(wallrj): See if there's an API for checking that this secret has been | ||
| # imported by the backend. For now we have to log into the Disco web UI and | ||
| # search for this secret. | ||
| kubectl create secret generic e2e-sample-secret-$(date '+%s') \ |
There was a problem hiding this comment.
With a reused cluster, every run adds another e2e-sample-secret-<timestamp> Secret to default, and nothing removes them.
Line 104 also installs or upgrades External Secrets Operator in the developer's own cluster. If ESO is already installed under a different release name, Helm fails on CRD ownership. On a throwaway kind cluster neither of these mattered.
Please use a fixed name and kubectl apply, or delete the Secret on exit. Please also mention the ESO install in the header so people know what the script changes on their cluster.
| PATH="$(bin_dir)/tools:${PATH}" ./hack/ark/test-e2e.sh | ||
|
|
||
| .PHONY: ark-test-e2e-jwt | ||
| ## Run a basic E2E test on a Kind cluster using Conjur JWT auth, no |
There was a problem hiding this comment.
The help text says "on a Kind cluster", and the target depends on $(NEEDS_KIND). The script's own header says a kind cluster will not be trusted. Please change this to "on an existing cluster", and drop $(NEEDS_KIND) once the script stops creating clusters.
| set -o pipefail | ||
|
|
||
| # The Conjur authn-jwt service ID onboarded for the target cluster. | ||
| : ${ARK_SERVICE_ID?} |
There was a problem hiding this comment.
${ARK_SERVICE_ID?} only rejects an unset variable, not an empty one. With ARK_SERVICE_ID= the chart renders service_id: "", the agent silently falls back to username/password auth, and the run fails at the end with a credentials error. That is the opposite of the auth mode this script exists to test. ARK_SUBDOMAIN on line 34 has the same problem.
| : ${ARK_SERVICE_ID?} | |
| : ${ARK_SERVICE_ID:?} |
| @@ -0,0 +1,169 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
About 20 of these 169 lines differ from test-e2e.sh. A fix to one script will be missed in the other. The problems on lines 153 and 160 already exist in both.
Please consider having test-e2e.sh choose the auth method based on whether ARK_SERVICE_ID is set, instead of adding a second copy. Line 131 also copies the original author's contact handle into clusterDescription.
Summary
Closes #839. Adds
hack/ark/test-e2e-jwt.sh(make ark-test-e2e-jwt), a local dev script mirroringtest-e2e.shbut exercising the Conjur-JWT-only path added in #838:--set config.cyberark.serviceId/config.cyberark.subdomain, noagent-credentialsSecret at all. It asserts the Secret genuinely doesn't exist whenARK_DISCOVERY_APIis unset, so a regression that silently starts requiring one again would fail this check.Not wired into CI
.github/workflows/tests.yamlis untouched. A live run needs the target cluster already onboarded in Conjur Cloud (anauthn-jwtauthenticator trusting that cluster's OIDC issuer/JWKS, a registered workload, upload grants) — no such onboarding automation exists in this repo, and CI has no secrets for it. A freshkind create clusterper run also won't match any pre-onboarded authenticator, since JWT trust is bound to a specific issuer. The script requiresUSE_EXISTING_CLUSTER=truepointed at whichever cluster you've already onboarded by hand.Test plan
bash -n hack/ark/test-e2e-jwt.sh— syntax check.make -n ark-test-e2e-jwt— target resolves correctly.test-e2e.shto confirm nothing outside the auth-method wiring changed.