Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
169 changes: 169 additions & 0 deletions hack/ark/test-e2e-jwt.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
#!/usr/bin/env bash

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.

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.

#
# Build and deploy the disco-agent Helm chart using Conjur JWT auth
# (config.cyberark.serviceId + config.cyberark.subdomain), with no
# agent-credentials Secret at all.
#
# Unlike test-e2e.sh (legacy username/password), this requires the target
# cluster to already be onboarded in Conjur Cloud: an authn-jwt authenticator
# trusting the cluster's OIDC issuer/JWKS, a registered workload for the
# disco-agent ServiceAccount, and upload grants for that workload. See the
# chart README's "Per-cluster Conjur onboarding" section. This script does
# not perform that onboarding, and a fresh `kind create cluster` will not be
# trusted by an authenticator onboarded against a different cluster's issuer
# -- point USE_EXISTING_CLUSTER at whichever cluster you already onboarded.
#
# Prerequisites:
# * kubectl: https://kubernetes.io/docs/tasks/tools/#kubectl
# * kind: https://kind.sigs.k8s.io/docs/user/quick-start/
# * helm: https://helm.sh/docs/intro/install/
# * jq: https://jqlang.github.io/jq/download/
# * make: https://www.gnu.org/software/make/
#
# You can run `make ark-test-e2e-jwt` which will automatically download all
# prerequisites and then run this script.

set -o nounset
set -o errexit
set -o pipefail

# The Conjur authn-jwt service ID onboarded for the target cluster.
: ${ARK_SERVICE_ID?}

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.

${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.

Suggested change
: ${ARK_SERVICE_ID?}
: ${ARK_SERVICE_ID:?}


# CyberArk tenant subdomain. Not a credential.
: ${ARK_SUBDOMAIN?}

# Discovery API URL. Leave empty to use the production default -- only set
# this for a non-production tenant. When set, this script creates a Secret
# holding just this one non-credential value (config.cyberark has no field
# for it yet).
: ${ARK_DISCOVERY_API:=}

# The base URL of the OCI registry used for Docker images and Helm charts
# E.g. ttl.sh/7e6ca67c-96dc-4dea-9437-80b0f3a69fb1
: ${OCI_BASE?}

# The Kubernetes namespace to install into
: ${NAMESPACE:=cyberark}

# 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}

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


script_dir=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)
root_dir=$(cd "${script_dir}/../.." && pwd)
export TERM=dumb

tmp_dir="$(mktemp -d /tmp/jetstack-secure.XXXXX)"
trap 'rm -rf "${tmp_dir}"' EXIT

pushd "${tmp_dir}"
> release.env
make -C "$root_dir" ark-release \
GITHUB_OUTPUT="${tmp_dir}/release.env" \
OCI_SIGN_ON_PUSH=false \
oci_platforms="" \
ARK_OCI_BASE="${OCI_BASE}"
cat release.env
source release.env

if [[ "$USE_EXISTING_CLUSTER" != true ]]; then
kind create cluster || true
fi

kubectl create ns "$NAMESPACE" || true

kubectl delete secret agent-credentials --namespace "$NAMESPACE" --ignore-not-found
if [[ -n "$ARK_DISCOVERY_API" ]]; then
kubectl create secret generic agent-credentials \
--namespace "$NAMESPACE" \
--from-literal=ARK_DISCOVERY_API=$ARK_DISCOVERY_API
fi

# Create a sample secret in the cluster
#
# 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') \

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.

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.

--namespace default \
--from-literal=username=${RANDOM}

# Create a sample ConfigMap in the cluster that will be discovered by the agent
#
# This ConfigMap has the label that matches the default label-selector configured
# in the ark/configmaps data gatherer (conjur.org/name=conjur-connect-configmap).
kubectl apply -f "${root_dir}/hack/ark/conjur-connect-configmap.yaml"

# Install External Secrets Operator CRDs and controller
#
# This is required for the agent to discover ExternalSecret and SecretStore resources.
echo "Installing External Secrets Operator..."
helm repo add external-secrets https://charts.external-secrets.io
helm repo update
helm upgrade --install external-secrets \
external-secrets/external-secrets \
--namespace external-secrets-system \
--create-namespace \
--wait \
--set installCRDs=true

# Create sample External Secrets Operator resources that will be discovered by the agent
kubectl apply -f "${root_dir}/hack/ark/secret-store.yaml"
kubectl apply -f "${root_dir}/hack/ark/external-secret.yaml"
kubectl apply -f "${root_dir}/hack/ark/cluster-secret-store.yaml"
kubectl apply -f "${root_dir}/hack/ark/cluster-external-secret.yaml"

# We use a non-existent tag and omit the `--version` flag, to work around a Helm
# v4 bug. See: https://github.com/helm/helm/issues/31600
helm upgrade agent "oci://${ARK_CHART}:NON_EXISTENT_TAG@${ARK_CHART_DIGEST}" \
--install \
--wait \
--create-namespace \
--namespace "$NAMESPACE" \
--set-json extraArgs='["--log-level=6"]' \
--set pprof.enabled=true \
--set fullnameOverride=disco-agent \
--set "imageRegistry=${OCI_BASE}" \
--set "imageNamespace=" \
--set "image.digest=${ARK_IMAGE_DIGEST}" \
--set config.clusterName="e2e-jwt-test-cluster" \
--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" \

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

--set acceptTerms=true \
--set-json "podLabels={\"disco-agent.cyberark.cloud/test-id\": \"${RANDOM}\"}"

kubectl rollout status deployments/disco-agent --namespace "${NAMESPACE}"

# 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

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

echo "agent-credentials Secret exists, but this run is meant to be Secret-free" >&2
exit 1
fi
fi

# Wait 60s for log message indicating success.
# 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)

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.

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.


# Query the Prometheus metrics endpoint to ensure it's working.
kubectl get pod \
--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 \

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.

{.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).

| grep '^process_'

# Query the pprof endpoint to ensure it's working.
kubectl get pod \
--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/debug/pprof/cmdline \
| xargs -0
9 changes: 9 additions & 0 deletions make/ark/02_mod.mk
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ ark-release:
ark-test-e2e: $(NEEDS_KIND) $(NEEDS_KUBECTL) $(NEEDS_HELM)
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

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

## agent-credentials Secret. Requires the target cluster to already be
## onboarded in Conjur Cloud -- see hack/ark/test-e2e-jwt.sh for details.
## Not wired into CI: no CI-side onboarding automation exists yet.
## @category CyberArk Discovery and Context
ark-test-e2e-jwt: $(NEEDS_KIND) $(NEEDS_KUBECTL) $(NEEDS_HELM)
PATH="$(bin_dir)/tools:${PATH}" ./hack/ark/test-e2e-jwt.sh

.PHONY: ark-verify
## Verify the Helm chart
## @category CyberArk Discovery and Context
Expand Down
Loading