Skip to content

fix: Move sharedSecret to Kubernetes secret and restrict access to admins - #1818

Open
Richard87 wants to merge 23 commits into
masterfrom
606-migrate-sharedSecret-to-k8s-secret
Open

fix: Move sharedSecret to Kubernetes secret and restrict access to admins#1818
Richard87 wants to merge 23 commits into
masterfrom
606-migrate-sharedSecret-to-k8s-secret

Conversation

@Richard87

@Richard87 Richard87 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

The web-console does not look great when its a reader user

Deploy key is "pending
hared secret is MIA

Atleast it doesnt break completley

equinor/radix-web-console#1367

Copilot AI left a comment

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.

Pull request overview

This PR migrates the GitHub webhook shared secret from RadixRegistration.Spec.SharedSecret into a dedicated Kubernetes Secret in the application namespace, with RBAC intended to restrict access to app-admins.

Changes:

  • Introduces radix-webhook-shared-secret secret name/key defaults and adds operator logic to seed/repair the secret during sync.
  • Updates operator reconciliation to grant app-admin access to the shared-secret Kubernetes Secret.
  • Updates api-server endpoints/tests to regenerate and read the shared secret via the Kubernetes Secret (instead of RadixRegistration.Spec.SharedSecret).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.

Show a summary per file
File Description
pkg/apis/defaults/commonsecrets.go Adds shared-secret secret name/key constants.
pkg/apis/application/secrets.go Adds operator seeding/repair logic for webhook shared secret Secret.
pkg/apis/application/secrets_test.go Adds tests for seeding/non-overwrite/repair behavior of shared secret Secret.
pkg/apis/application/application.go Calls shared-secret secret apply + grants admin RBAC access during reconcile.
pkg/apis/application/application_test.go Extends RBAC/secret expectations to include the new shared secret Secret.
api-server/api/applications/applications_handler.go Moves regenerate/read shared secret logic to Kubernetes Secret access.
api-server/api/applications/applications_controller_test.go Updates controller tests to validate secret-based regenerate/read behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/apis/application/secrets.go
Comment thread pkg/apis/application/secrets.go Outdated
Comment thread pkg/apis/application/secrets.go Outdated
Comment thread pkg/apis/application/secrets.go Outdated
Comment thread api-server/api/applications/applications_handler.go
Comment thread pkg/apis/application/secrets_test.go Outdated
Comment thread api-server/api/applications/applications_controller_test.go
Comment thread api-server/api/applications/applications_controller_test.go Outdated
Comment thread api-server/api/applications/applications_controller_test.go
Comment thread pkg/apis/application/secrets_test.go Outdated
@Richard87
Richard87 requested a review from nilsgstrabo July 7, 2026 11:42
Copilot AI review requested due to automatic review settings July 27, 2026 11:57

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Comment on lines +470 to 473
secret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
if err != nil && !k8serrors.IsNotFound(err) {
return nil, err
}
Comment thread pkg/apis/application/secrets.go Outdated
Comment thread pkg/apis/application/secrets_test.go
Comment thread Makefile
Copilot AI review requested due to automatic review settings July 27, 2026 12:03

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

Makefile:10

  • This Makefile change removes the previous branch/environment gating and changes the default VERSION from latest to dev. That affects tagging and deployment behavior, but it is unrelated to the PR’s stated goal (webhook shared-secret migration) and isn’t explained in the PR description. Consider reverting these Makefile changes or moving them to a separate PR with rationale, so the secret-migration change can be reviewed/rolled back independently if needed.
ENVIRONMENT ?= dev
VERSION 	?= dev
CRD_TEMP_DIR := ./.temp-resources/
CRD_CHART_DIR := ./charts/radix-operator/templates/
JSON_SCHEMA_DIR := ./json-schema/
CONTAINER_REPO ?= radix$(ENVIRONMENT)
DOCKER_REGISTRY	?= $(CONTAINER_REPO).azurecr.io
BRANCH := $(shell git rev-parse --abbrev-ref HEAD | sed 's|/|-|g')
HASH := $(shell git rev-parse HEAD)
TAG := $(BRANCH)-$(HASH)

Comment thread api-server/api/githubwebhook/github_controller.go
Comment thread api-server/api/applications/applications_controller_test.go
Copilot AI review requested due to automatic review settings July 30, 2026 08:13

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

api-server/api/applications/applications_handler.go:473

  • GetDeployKeyAndSecret currently returns an error for any Secret GET error other than NotFound. For reader users (intentionally lacking access to the webhook shared secret), this will likely be a Forbidden error and cause the whole endpoint to fail instead of returning the public deploy key with an empty SharedSecret.
	secret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
	if err != nil && !k8serrors.IsNotFound(err) {
		return nil, err
	}

api-server/api/githubwebhook/github_controller.go:329

  • If the Kubernetes Secret does not exist, getWebhookSharedSecret falls back to rr.Spec.SharedSecret but returns nil error even when that field is empty. That leads to a later signature-validation error, which is misleading compared to reporting a missing/empty webhook secret configuration up front.
	if err != nil {
		if k8serrors.IsNotFound(err) {
			// TODO: When all Secrets have been created and seeded, remove the deprecated Spec.SharedSecret field from RadixRegistration and stop seeding from it.
			return []byte(rr.Spec.SharedSecret), nil //nolint:staticcheck
		}

pkg/apis/application/secrets_test.go:205

  • assertWebhookSharedSecretIsGeneratedAndValid asserts the same err twice; the final assert.NoError(t, err) is redundant and can be removed.
func assertWebhookSharedSecretIsGeneratedAndValid(t *testing.T, encodedSecret []byte) {
	decodedSecret, err := base64.StdEncoding.DecodeString(string(encodedSecret))
	assert.NoError(t, err)
	assert.NotEmpty(t, decodedSecret)

	assert.GreaterOrEqual(t, len(decodedSecret), 10)
	assert.NoError(t, err)
}

Makefile:23

  • The Makefile changes (VERSION default, removed DNS/branch gating logic, new echo output) appear unrelated to moving the webhook shared secret into a Kubernetes Secret and restricting access. This increases the scope/risk of the PR and may affect developer workflows and deployment behavior.
ENVIRONMENT ?= dev
VERSION 	?= dev
CRD_TEMP_DIR := ./.temp-resources/
CRD_CHART_DIR := ./charts/radix-operator/templates/
JSON_SCHEMA_DIR := ./json-schema/
CONTAINER_REPO ?= radix$(ENVIRONMENT)
DOCKER_REGISTRY	?= $(CONTAINER_REPO).azurecr.io
BRANCH := $(shell git rev-parse --abbrev-ref HEAD | sed 's|/|-|g')
HASH := $(shell git rev-parse HEAD)
TAG := $(BRANCH)-$(HASH)

echo:
	@echo "ENVIRONMENT : " $(ENVIRONMENT)
	@echo "CONTAINER_REPO : " $(CONTAINER_REPO)
	@echo "DOCKER_REGISTRY : " $(DOCKER_REGISTRY)
	@echo "BRANCH : " $(BRANCH)
	@echo "VERSION : " $(VERSION)
	@echo "TAG : " $(TAG)
	@echo ""
	@echo "pipeline-runner : " $(DOCKER_REGISTRY)/radix-pipeline:$(TAG)
	@echo "radix-operator : " $(DOCKER_REGISTRY)/radix-operator:$(TAG)
	@echo "radix-webhook : " $(DOCKER_REGISTRY)/radix-webhook:$(TAG)
	@echo "radix-api-server : " $(DOCKER_REGISTRY)/radix-api-server:$(TAG)

e2e/radix_job_queueing_test.go:25

  • The new E2E timeouts (15s prereqs / 60s progress) are very aggressive for CI clusters and can make the test flaky (waiting for operator deployment ready, RBAC, job activation/queue drain). Consider increasing these to more forgiving values.
const (
	queueTestPrereqTimeout   = 15 * time.Second
	queueTestProgressTimeout = 60 * time.Second
	queueTestPollInterval    = 200 * time.Millisecond
	queueTestCreationSpacing = 1100 * time.Millisecond
)

e2e/radix_job_queueing_test.go:231

  • waitForSecret currently treats any GET error as "not ready" and will time out without surfacing non-NotFound errors (e.g., RBAC/connection issues). Returning non-NotFound errors improves debuggability and avoids hiding real failures behind a deadline exceeded.
func waitForSecret(ctx context.Context, c client.Client, namespace, name string, timeout time.Duration) error {
	return wait.PollUntilContextTimeout(ctx, queueTestPollInterval, timeout, true, func(ctx context.Context) (bool, error) {
		secret := &corev1.Secret{}
		if err := c.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, secret); err == nil {
			return true, nil
		}
		return false, nil
	})

e2e/internal/gatewayapi_installer.go:13

  • This updates the installed Gateway API CRDs to v1.5.1, but go.mod still depends on sigs.k8s.io/gateway-api v1.4.0. Installing newer CRDs than the compiled types can cause subtle schema/field drift in E2E. Consider aligning the CRD URL and the Go module version (either bump the dependency or pin the CRDs to the same version).
// InstallGatewayApiCRDs installs the Gateway API CRDs from GitHub
func InstallGatewayApiCRDs(ctx context.Context, KubeConfigPath string) error {
	crdURL := "https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml"

Copilot AI review requested due to automatic review settings July 30, 2026 08:22
Copilot AI review requested due to automatic review settings July 30, 2026 12:07

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

api-server/api/applications/applications_handler.go:474

  • GetDeployKeyAndSecret currently returns an error if the caller is forbidden from reading the webhook shared-secret Secret. This makes the whole endpoint fail for reader users, which matches the reported UX issue (deploy key shows as pending and shared secret missing) even though returning the public deploy key would still be useful. Treat Forbidden the same as NotFound here (return sharedSecret="" while still returning the public key) to avoid breaking the endpoint for non-admins.
	secret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
	if err != nil && !k8serrors.IsNotFound(err) {
		return nil, err
	}

pkg/apis/application/secrets_test.go:204

  • assertWebhookSharedSecretIsGeneratedAndValid asserts err twice; the second assert.NoError(t, err) is redundant and can be removed to keep the helper focused.
	assert.GreaterOrEqual(t, len(decodedSecret), 10)
	assert.NoError(t, err)

api-server/api/applications/applications_controller_test.go:1305

  • This test no longer sends a requested shared secret in the regenerate call, but it still defines newSharedSecret and asserts the regenerated value is not equal to that constant. That assertion is effectively meaningless (it will almost always pass) and doesn’t verify that regeneration actually changed the secret. Capture the secret value before regeneration and assert the value changes afterwards.
	const newSharedSecret = "new shared secret"
	responseChannel := controllerTestUtils.ExecuteRequestWithParameters("POST", fmt.Sprintf("/api/v1/applications/%s/regenerate-shared-secret", appName), nil)
	response := <-responseChannel
	assert.Equal(t, http.StatusNoContent, response.Code)

	sharedSecret, err := kubeUtil.CoreV1().Secrets(builders.GetAppNamespace(appName)).Get(context.Background(), defaults.WebhookSharedSecretName, metav1.GetOptions{})
	require.NoError(t, err)
	decodedSharedSecret, err := base64.StdEncoding.DecodeString(string(sharedSecret.Data[defaults.WebhookSharedSecretKey]))
	require.NoError(t, err)
	assert.NotEqual(t, newSharedSecret, string(decodedSharedSecret))

e2e/radix_job_queueing_test.go:25

  • queueTestProgressTimeout is set to 60s and is used for the whole queue drain loop (PollUntilContextTimeout). This was previously several minutes and 60s is likely too short for real clusters/CI, making the e2e test flaky when pipelines are slow to start/stop. Increase the timeouts (or split prereq vs progress timeouts) to keep the test reliable.
const (
	queueTestPrereqTimeout   = 15 * time.Second
	queueTestProgressTimeout = 60 * time.Second
	queueTestPollInterval    = 200 * time.Millisecond
	queueTestCreationSpacing = 1100 * time.Millisecond
)

Copilot AI review requested due to automatic review settings July 31, 2026 09:37

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (3)

api-server/api/githubwebhook/github_controller.go:373

  • When the webhook shared-secret Secret is missing, getWebhookSharedSecret falls back to rr.Spec.SharedSecret without validating that it is non-empty/non-whitespace. This can lead to webhook signature validation being performed with an empty secret (effectively disabling authentication) if the deprecated field is empty during a migration window.
	secret, err := accounts.ServiceAccount.Client.CoreV1().Secrets(operatorutils.GetAppNamespace(rr.Name)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
	if err != nil {
		if k8serrors.IsNotFound(err) {
			// TODO: When all Secrets have been created and seeded, remove the deprecated Spec.SharedSecret field from RadixRegistration and stop seeding from it.
			return []byte(rr.Spec.SharedSecret), nil //nolint:staticcheck
		}

pkg/apis/application/secrets_test.go:205

  • assertWebhookSharedSecretIsGeneratedAndValid asserts err == nil twice; the final assert.NoError(t, err) is redundant and makes the helper noisier than necessary.
func assertWebhookSharedSecretIsGeneratedAndValid(t *testing.T, encodedSecret []byte) {
	decodedSecret, err := base64.StdEncoding.DecodeString(string(encodedSecret))
	assert.NoError(t, err)
	assert.NotEmpty(t, decodedSecret)

	assert.GreaterOrEqual(t, len(decodedSecret), 10)
	assert.NoError(t, err)
}

api-server/api/applications/applications_handler.go:447

  • RegenerateSharedSecret currently fails with NotFound if the webhook shared-secret Secret is missing (e.g., newly registered app before operator has synced, or if the Secret was deleted). That makes the regeneration endpoint unusable in exactly the scenarios where you’d want it to repair/seal the configuration.
func (ah *ApplicationHandler) RegenerateSharedSecret(ctx context.Context, appName string) error {
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		// Make check that this is an existing application and that the user has access to the secret
		currentSecret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
		if err != nil {
			return err
		}

Copilot AI review requested due to automatic review settings July 31, 2026 10:06

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (7)

api-server/api/githubwebhook/github_controller.go:373

  • If the Secret is missing, this falls back to rr.Spec.SharedSecret without validating that it is non-empty. If the legacy field is empty/whitespace (it is optional), webhook validation effectively uses an empty HMAC key, which is guessable and can allow unauthorized webhook triggers. Treat missing Secret + empty legacy value as a misconfiguration and return an error instead of an empty secret.
		if k8serrors.IsNotFound(err) {
			// TODO: When all Secrets have been created and seeded, remove the deprecated Spec.SharedSecret field from RadixRegistration and stop seeding from it.
			return []byte(rr.Spec.SharedSecret), nil //nolint:staticcheck
		}

api-server/api/applications/applications_handler.go:475

  • GetDeployKeyAndSecret currently treats a missing webhook shared secret as non-fatal and returns HTTP 200 with SharedSecret="". During migration (or if the operator hasn't seeded the secret yet), this makes the shared secret appear "missing" even for admins and is indistinguishable from an intentionally empty value. Consider falling back to the deprecated RadixRegistration.Spec.SharedSecret (as githubwebhook does) or returning an explicit error when the secret is not found.
	secret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
	if err != nil && !k8serrors.IsNotFound(err) {
		return nil, err
	}
	sharedSecret := ""

pkg/apis/application/secrets_test.go:204

  • The final assert.NoError(t, err) is redundant here; err is unchanged since the earlier assert.NoError(t, err) and can be removed to reduce noise.
	assert.GreaterOrEqual(t, len(decodedSecret), 10)
}

e2e/README.md:12

  • The example curl command for gateway-api is split across two lines ("../internal/g" + "ateway...") and the path differs from the embedded manifest location (e2e/internal). As written, copying this snippet will fail and/or write the file to the wrong place.
curl https://github.com/prometheus-operator/prometheus-operator/releases/download/v0.82.0/stripped-down-crds.yaml -L 
 > internal/prometheus-operator-stripped-down-crds.yaml

curl https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml -L > ../internal/g
ateway-api-standard-install.yaml

e2e/internal/prometheus_installer.go:15

  • This comment says the CRDs are installed "from GitHub", but the implementation now applies an embedded YAML manifest via stdin. Update the comment to match the new behavior.
// InstallPrometheusOperatorCRDs installs the Prometheus Operator CRDs from GitHub

e2e/internal/gatewayapi_installer.go:15

  • This comment says the CRDs are installed "from GitHub", but the implementation now applies an embedded YAML manifest via stdin. Update the comment to match the new behavior.
// InstallGatewayApiCRDs installs the Gateway API CRDs from GitHub

e2e/radix_job_queueing_test.go:22

  • queueTestProgressTimeout is used for the full queue-drain phase (activation of 3 build jobs + stopping active jobs). Reducing this to 60s (previously minutes) risks making the e2e test flaky on slower CI clusters. Consider restoring a longer timeout for the progress/drain phase.
	queueTestProgressTimeout = 60 * time.Second

Copilot AI review requested due to automatic review settings July 31, 2026 10:10

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

e2e/README.md:12

  • The curl example is currently broken across lines ("../internal/g\nateway...") and points to ../internal even though the embedded YAML files live under e2e/internal. This makes the copy/paste instructions produce a wrong filename/path.
curl https://github.com/prometheus-operator/prometheus-operator/releases/download/v0.82.0/stripped-down-crds.yaml -L 
 > internal/prometheus-operator-stripped-down-crds.yaml

curl https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml -L > ../internal/g
ateway-api-standard-install.yaml

e2e/radix_job_queueing_test.go:22

  • queueTestProgressTimeout was reduced to 60s but it now bounds both the apply-config start wait and the full queue-drain loop. In slower CI/Kind environments, job activation can take longer than 60s, making this e2e test flaky. Consider restoring a multi-minute timeout for the progress phase (the previous drain loop allowed up to 4 minutes).
	queueTestProgressTimeout = 60 * time.Second

Comment thread api-server/api/applications/applications_handler.go
Copilot AI review requested due to automatic review settings July 31, 2026 10:36

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (5)

pkg/apis/application/secrets.go:185

  • The existing webhook shared-secret is treated as valid as long as the key exists. This does not match the function’s contract (“valid data”) and means empty or non-base64 values will never be re-seeded, potentially breaking webhook validation until someone manually fixes the Secret.
		sharedSecret, valid := desired.Data[defaults.WebhookSharedSecretKey]
		if !valid {
			secret := []byte(rand.Text())

			// Legacy support, check if the secret exist on the deprecated field on RadixRegistration and seed from it if it does

e2e/README.md:12

  • The Gateway API curl command is split mid-path, which makes the snippet invalid to copy/paste.
curl https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml -L > ../internal/g
ateway-api-standard-install.yaml

e2e/internal/gatewayapi_installer.go:15

  • This comment says the CRDs are installed “from GitHub”, but the implementation now installs from the embedded YAML. Updating the comment avoids confusion when debugging e2e failures.
// InstallGatewayApiCRDs installs the Gateway API CRDs from GitHub

e2e/radix_job_queueing_test.go:25

  • queueTestProgressTimeout is used for draining/observing multiple RadixJobs. Setting it to 60s (down from the previous multi-minute timeout) can make this e2e test flaky on slower CI nodes or when the cluster is under load.
	queueTestPrereqTimeout   = 30 * time.Second
	queueTestProgressTimeout = 60 * time.Second
	queueTestPollInterval    = 200 * time.Millisecond
	queueTestCreationSpacing = 1100 * time.Millisecond
)

e2e/internal/prometheus_installer.go:15

  • This comment says the CRDs are installed “from GitHub”, but the implementation now installs from the embedded YAML. Updating the comment avoids confusion when debugging e2e failures.
// InstallPrometheusOperatorCRDs installs the Prometheus Operator CRDs from GitHub

Copilot AI review requested due to automatic review settings July 31, 2026 11:10

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (3)

pkg/apis/application/secrets.go:185

  • applyWebhookSharedSecret treats the webhook key as “valid” purely based on map presence. If the key exists but is empty (or contains non-decodable/whitespace-only data), the operator will preserve it, but api-server webhook validation will later fail to decode / reject it. Treat empty/invalid base64/blank values as invalid and reseed.
		sharedSecret, valid := desired.Data[defaults.WebhookSharedSecretKey]
		if !valid {
			secret := []byte(rand.Text())

			// Legacy support, check if the secret exist on the deprecated field on RadixRegistration and seed from it if it does

api-server/api/applications/applications_handler.go:474

  • GetDeployKeyAndSecret returns the raw Secret GET error for anything except NotFound. With this PR, the webhook shared secret is deliberately restricted to admins (RoleBinding is only created for app-admin), so reader users will typically hit a Forbidden here, causing the endpoint to fail and the web-console UX to still look broken. Consider treating Forbidden as “not available to this user” and return the response with an empty SharedSecret instead of erroring.
	secret, err := ah.getUserAccount().Client.CoreV1().Secrets(operatorUtils.GetAppNamespace(appName)).Get(ctx, defaults.WebhookSharedSecretName, metav1.GetOptions{})
	if err != nil && !k8serrors.IsNotFound(err) {
		return nil, err
	}

e2e/README.md:12

  • The Gateway API curl command is broken across lines ("../internal/g" + "ateway...") and also points outside the e2e directory. This makes the documented update step unusable and won’t update the embedded manifest under e2e/internal/.
curl https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml -L > ../internal/g
ateway-api-standard-install.yaml

Copilot AI review requested due to automatic review settings July 31, 2026 11:15

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

pkg/apis/application/secrets.go:185

  • crypto/rand doesn’t provide rand.Text(), so this won’t compile. Also, the code only checks whether the key exists, but the function comment says the Secret should be reseeded when it “does not contain valid data”; currently an empty/invalid-base64 value will be preserved and later break webhook validation.
		sharedSecret, valid := desired.Data[defaults.WebhookSharedSecretKey]
		if !valid {
			secret := []byte(rand.Text())

			// Legacy support, check if the secret exist on the deprecated field on RadixRegistration and seed from it if it does

pkg/apis/application/secrets.go:202

  • crypto/rand doesn’t provide rand.Text(), so this fallback path won’t compile. Consider generating a printable random secret with rand.Read + base64 when the deprecated RR field is empty.
	sharedSecret := strings.TrimSpace(app.registration.Spec.SharedSecret) //nolint:staticcheck
	if sharedSecret == "" {
		sharedSecret = rand.Text()
	}
	encodedSharedSecret := base64.StdEncoding.EncodeToString([]byte(sharedSecret))

e2e/README.md:12

  • The gateway-api curl command path is broken by a newline, and it also writes to ../internal/... while the embedded manifest is expected at e2e/internal/gateway-api-standard-install.yaml (same dir as this README’s internal/ path). This will make the embed file stale/missing.
curl https://github.com/prometheus-operator/prometheus-operator/releases/download/v0.82.0/stripped-down-crds.yaml -L 
 > internal/prometheus-operator-stripped-down-crds.yaml

curl https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.1/standard-install.yaml -L > ../internal/g
ateway-api-standard-install.yaml

Comment thread api-server/api/applications/applications_handler.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants