Repository navigation
graduate credential injection from experimental - #2101
Lior Lieberman (LiorLieberman) merged 7 commits into
Conversation
214b71b to
ee2e850
Compare
|
|
why do we use explicit |
EgressPolicy allows setting credential injection unconditionally. IMO we need to enable a easy UX for users to understand they need to setup their credential injection provider, otherwise they (or their harnesses may attempt to set creds but no credential provider will be installed) - which makes the non happy path more common. So the TL;DR is that I think we should have a way to understand if "off" explicitly set or "user just forgot to install". Disallowing empty gives us this, and --credential-provider-name=k8s should be what we encourage for "easy setup" IMO. Open to feedback ofc |
ee2e850 to
065c3a5
Compare
Steven Shriver (stshrive)
left a comment
Steven Shriver (stshrive)
left a comment
There was a problem hiding this comment.
Generally looks good. The comments and documentation smell of Claude and could use some cleaning up. I generally recommend giving a prompt to:
"update the comments in this change - make them as clear and concise as possible without using mannered prose or other narratorial wording."
065c3a5 to
c3751d9
Compare
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review.
Looks good overall! Two things to address around provider scheduling and e2e cleanup:
| return err | ||
| } | ||
| } | ||
| return e.renderResolveApply(ctx, e.k8sCredentialProviderPath(k8sCredentialProviderManifest)) |
There was a problem hiding this comment.
Can we add k8s-credential-provider to the Deployment targets in manifests/ate-install/components/cordon-control-plane/kustomization.yaml? The render goes through that component, but its name selector doesn't include the new provider, so --cordon-control-plane gives it neither the node selector nor the toleration for the control-plane pool. It ends up on worker nodes, or stays Pending and times out the install if no untainted nodes are available.
There was a problem hiding this comment.
done
| return | ||
| } | ||
| kubectl("delete", "--ignore-not-found", "-f", fixture) | ||
| kubectl("apply", "-f", filepath.Join(root, credentialProviderPolicyManifest)) |
There was a problem hiding this comment.
Can we restart the provider and wait for its rollout after restoring the policy here, as we do during setup? The provider only loads authorization at startup. Now that cleanup leaves the Deployment running, applying the empty ConfigMap doesn't revoke the test grants in the running process; they remain active until an unrelated restart.
There was a problem hiding this comment.
done
…injection knob Egress credential injection was switched on by a separate boolean, --experimental-egress-credential-injection (ATE_CREDENTIAL_INJECTION_ENABLED), with --credential-provider-name and --credential-provider-address defaulting to the bundled Kubernetes Secrets provider underneath it. Two flags for one decision, and an install that said nothing got injection silently off. The boolean is gone. --credential-provider-name (or ATE_CREDENTIAL_PROVIDER_NAME) is now required by deploy ate-system and deploy atenet, the two commands that render the egress gateway, and takes one of three shapes: "off" leaves the gateway without provider flags, ate-secret://k8s.io selects the bundled provider at its known address, and any other ate-secret:// prefix names a provider of the operator's own and requires --credential-provider-address. Malformed names, a provider on the agentgateway dataplane, and a foreign provider without an address are refused when the flags are read; the missing-name error is raised by the deploy paths before they touch the cluster, so publish, delete, and the other commands do not demand the flag. The address is ignored with "off" so a value left in the environment cannot keep injection from being turned off from the command line. The marker splice in the egress manifest now always runs: it emits the provider flags for a selected provider and removes the marker for "off", so a manifest that lost the marker fails either way. CI sets ATE_CREDENTIAL_PROVIDER_NAME per dataplane. Every documented install command gains the flag, and .ate-dev-env.sh.example carries the choice for GKE installs that pick their settings up from the dev env.
The Kubernetes Secrets credential provider was a manual step: the installer pointed the gateway at k8s-credential-provider.ate-system.svc and left the operator to ko-apply the provider manifests by hand, which the e2e suite also did for itself. With --credential-provider-name required, the selection is explicit enough to act on. deploy ate-system and deploy atenet now deploy the provider, from manifests/egress-credential-injection, when the name is ate-secret://k8s.io, and wait for its rollout with the gateway's. Its namespace policy ConfigMap is created only when absent: the provider mounts it, so it has to exist before the first pod, but it is the operator's atespace allow-list and a redeploy must not reset it. The sample secret is never applied. Any other selection, "off" included, removes a bundled provider an earlier install left behind, since nothing points at it and it holds cluster-wide Secret read. delete ate-system and delete atenet remove it as well; its ClusterRole and binding would otherwise outlive the namespace. The e2e helper no longer deploys the provider. It swaps the policy ConfigMap for the fixture's, restarts the provider, and puts the shipped policy back when the test passes, so a provider restarted later still has a ConfigMap to mount. A test pins the names the step relies on to the manifests.
…red injection only works with replace for v1
125b47d to
1d3fd4b
Compare
| ```bash | ||
| # .ate-dev-env.sh at the repo root, then: | ||
| hack/install-ate.sh --deploy-ate-system | ||
| hack/install-ate.sh --deploy-ate-system --credential-provider='{"name":"k8s.io"}' |
There was a problem hiding this comment.
why isn't this just default? why are we making virtually all calls to the installer pass a json string?
|
|
||
| # install ate, PostgreSQL, rustfs | ||
| hack/install-ate-kind.sh --deploy-ate-system | ||
| hack/install-ate-kind.sh --deploy-ate-system --credential-provider='{"name":"k8s.io"}' |
There was a problem hiding this comment.
If we need even the README demo to specify this, it should probably just be the default.
After this change, hack/run-microvm-demo.sh fails without passing it as well, but those docs weren't updated. And the demo shouldn't even need the credential provider.
|
Thought about those some more, I think at least for kind let's just do #2628 |
Graduates egress credential injection out of experimental: removes
--experimental-egress-credential-injectionandATE_CREDENTIAL_INJECTION_ENABLED.Breaking change
deploy ate-systemanddeploy atenetnow require--credential-provider(orATE_CREDENTIAL_PROVIDER). It's a JSON value that replaces--credential-provider-nameand--credential-provider-address:{"name":"k8s.io"}deploys the bundled Kubernetes Secrets provider, plus a NetworkPolicy that only lets atenet-egress connect.addressis optional.{"enabled":false}turns injection off.{"name":"<provider-name>","address":"<host>:<port>"}uses a provider you deploy yourself.More
Provider names are bare DNS names; credential URIs keep
ate-secret://.The gateway now fails closed (500) when a rule needs a credential but no provider is configured.
Added a network policy with the k8s secret provider to allow only access from atenet-egress
The agentgateway dataplane refuses a provider for now (Eitan Yarmush (@EItanya) will follow up).
Docs and scripts use
{"name":"k8s.io"}.Related to above, we took a deliberate decision to install k8s.io credential provider deployment when k8s.io provider is specified. For this provider, namespace-policy ConfigMap is created only when absent.
Tests pass
Appropriate changes to documentation are included in the PR