[DOM-47678] Add Keycloak service account token support for image builds #101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ddl-ebrown
commented
Jun 13, 2023
ddl-ebrown
force-pushed
the
DOM-47678-build-service-credentials
branch
2 times, most recently
from
8ba7ab6
to
10a4895
Compare
ddl-ebrown
commented
Jun 13, 2023
| // cfg.Scopes... | ||
| jwt, err := kc.LoginClient(buildCtx, c.keycloakCfg.ClientID, c.keycloakCfg.ClientSecret, c.keycloakCfg.Realm) | ||
| if err != nil { | ||
| log.Error(err, fmt.Sprintf("Failed to acquire Keycloak credentials %s", c.keycloakCfg.ClientID)) |
There was a problem hiding this comment.
maybe this should fail the build?
There was a problem hiding this comment.
If the build requested it and it's not available, that would makes sense to me... just want to understand what the retry process looks like. I assume client is fully responsible for owning that process.
ddl-ebrown
force-pushed
the
DOM-47678-build-service-credentials
branch
11 times, most recently
from
a81653b
to
344b48e
Compare
- Exposes config in values.yaml for chart, then inject into the equivalent secret values for the operator
- Temporary import to prevent removal
ddl-ebrown
force-pushed
the
DOM-47678-build-service-credentials
branch
from
344b48e
to
245af02
Compare
ddl-ebrown
commented
Jun 22, 2023
ddl-ebrown
force-pushed
the
DOM-47678-build-service-credentials
branch
from
3043408
to
a704e39
Compare
- When configured to inject service account tokens into the build, pass a build arg to the Docker build as SERVICE_TOKEN=<JWT>. This allows builds to contact additional services in the cluster using a specific service account identity. The value is consumed by adding to the Dockerfile: ARG SERVICE_TOKEN
- If Keycloak configuration is disabled, but the incoming CR request asked for Keycloak token injection, then error / exit the build
ddl-ebrown
force-pushed
the
DOM-47678-build-service-credentials
branch
from
a704e39
to
76f3d90
Compare
ddl-ebrown
commented
Jun 22, 2023
| @@ -409,6 +409,19 @@ func (suite *GenericImageBuilderTestSuite) TestImageBuilding() { | |||
| assert.Equalf(t, ib.Status.Phase, hephv1.PhaseSucceeded, "failed build with message %q", ib.Status.Conditions[0].Message) | |||
| }) | |||
|
|
|||
| suite.T().Run("token_injection", func(t *testing.T) { | |||
There was a problem hiding this comment.
Note - this test doesn't run as part of PRs :(
ddl-ebrown
commented
Jun 30, 2023
| return ctrl.Result{}, c.phase.SetFailed(ctx, obj, fmt.Errorf("keycloak token acquire failed: %w", err)) | ||
| } | ||
|
|
||
| obj.Spec.BuildArgs = append(obj.Spec.BuildArgs, fmt.Sprintf("%s=%s", hephv1.ServiceTokenArgName, jwt.AccessToken)) |
There was a problem hiding this comment.
Exposing as a build arg is a bad way to do this as sensitive values may find their way into logs or image metadata
10 tasks
|
Going to close this as we've decided to move forward with the approach in #106 instead |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Described in https://docs.google.com/document/d/1LXl0aUKAZk9jpnKKGZZxrw3VFRxjAdLqa2WZriSj6Fc/edit as
Add Keycloak token acquisition support to HephaestusImage is consumable at https://github.com/dominodatalab/hephaestus/pkgs/container/hephaestus/103580396?tag=pr-101
Testing in a real deploy via cli invocation like
And a
dev-values.yamlconfig likeInside of Domino, there is an environment defined like:
FROMset todocker.io/library/alpine:latestInitial implementation verification
Build succeeded at https://mldemo4507.train-sandbox.domino.tech/environments/64920eb0bb2e9b737895a5b6/revisions/649396a7bb2e9b737895a5f5/build/649396a7bb2e9b737895a5f7/logs
After updating CRD with
EnableServiceAccountTokenInjectionfield defaulting to falseWithout client updates
I triggered another build without updating any code to verify that the new field is optional and defaults to false at https://mldemo4507.train-sandbox.domino.tech/environments/64920eb0bb2e9b737895a5b6/revisions/6493a50ebb2e9b737895a5f8/build/6493a50ebb2e9b737895a5fa/logs
The build completed successfully and did not inject the token as a build-arg
Submitting a CR that sets the value
EnableServiceAccountTokenInjection** TBD **