[DOM-47678] Add support for passing buildkit secrets via k8s secrets #106
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
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
c8a2690
to
fd2fc2d
Compare
ddl-ebrown
changed the title
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
fd2fc2d
to
b9f2664
Compare
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
3 times, most recently
from
030ab96
to
63d5ec5
Compare
|
I think we've got an existing bad test / race condition or something here... running tests locally, the failing test passes - but it keeps popping up in CI via I've seen different random fails that all appear to be in |
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
7 times, most recently
from
cfdf3b3
to
36a7546
Compare
ddl-ebrown
commented
Jul 3, 2023
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
2 times, most recently
from
ad607c2
to
4d4b346
Compare
ddl-ebrown
commented
Jul 4, 2023
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
4 times, most recently
from
038d060
to
a9ee412
Compare
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
a9ee412
to
8c3c884
Compare
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
8c3c884
to
53d15a0
Compare
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
53d15a0
to
818ac96
Compare
ddl-ryan-connor
approved these changes
Jul 5, 2023
Secretions
approved these changes
Jul 5, 2023
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
2 times, most recently
from
772b603
to
5158579
Compare
|
/functional-test |
- Adds the ability to pass k8s secret names by namespace / name to be consumed by the ImageBuild request using buildkit secrets. The `secrets` field is added to the CR as optional, and is fully backwards compatible with previous requests. Previously, the Helm chart supported exposing service level secrets into *all* builds, but this adds supports on a per request basis. Buildkit secrets must be mounted in via Dockerfile syntax: https://docs.docker.com/engine/reference/builder/#run---mounttypesecret Consumption via Dockerfile is therefore similar to: RUN --mount=type=secret,id=domino-compute/mysecret/foo cat /run/secrets/foo - For the secret to be accessible by Hephaestus, it must have the label `hephaestus-accessible: "true"`. This prevents the build service from having access to arbitrary secrets in the cluster and requires clients to specifically opt-in. - Additionally the `hephaestus-owned: "true"` label can be added to secrets to help manage their lifecycle. When set, the secret will be updated to specify the attached ImageBuild as the owner -- when ImageBuild resources are routinely purged by the service, those secrets will be cleaned up at the same time. This removes the burden of secret cleanup from clients, but changes the cleanup timing to be non-determinstic. The ClusterRole for Hephaestus is updated to allow for secret resource updates to support this feature.
ddl-ebrown
force-pushed
the
DOM-47678-add-secrets-to-CR
branch
from
aa0a66b
to
01ab710
Compare
|
Removed addition of functional tests for now since the machinery for that is currently totally broken: cc @steved Will try to get those working and do a follow-on PR, but I'm running short on time. |
|
Going to try to fix the tests in #108 |
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.
Adds the ability to pass k8s secret names by namespace / name to be
consumed by the ImageBuild request using buildkit secrets. The
secretsfield is added to the CR as optional, and is fullybackwards compatible with previous requests.
Previously, the Helm chart supported exposing service level secrets
into all builds, but this adds supports on a per request basis.
Buildkit secrets must be mounted in via Dockerfile syntax:
https://docs.docker.com/engine/reference/builder/#run---mounttypesecret
Consumption via Dockerfile is therefore similar to:
RUN --mount=type=secret,id=domino-compute/mysecret/foo cat /run/secrets/foo
For the secret to be accessible by Hephaestus, it must have the
label
hephaestus-accessible: "true". This prevents the buildservice from having access to arbitrary secrets in the cluster and
requires clients to specifically opt-in.
Additionally the
hephaestus-owned: "true"label can be added tosecrets to help manage their lifecycle. When set, the secret will be
updated to specify the attached ImageBuild as the owner -- when
ImageBuild resources are routinely purged by the service, those
secrets will be cleaned up at the same time. This removes the burden
of secret cleanup from clients, but changes the cleanup timing to be
non-determinstic.
The ClusterRole for Hephaestus is updated to allow for secret
resource updates to support this feature.
TODO:
zz_generated.openapi.goandswagger.jsonis properly updated -make sdksshould do this, but it doesn't appear to be... something in the repo tooling seems brokenownerReferenceson secrets is possible and is technically considered resource "adoption", but the code doesn't appear to be working. (See related issue on Added certificate owner ref field cert-manager/cert-manager#5158)Verify with tests
secretswork against new server that supports it)ImageBuildare correctly pulled (hacked server)ImageBuildare correctly pulledhephaestus-accessiblelabel cannot be read (generates a build error)hephaestus-ownedhave ownership set so that they're attached toImageBuildand deleted upon delete ofImageBuildresourcesDescribed in https://docs.google.com/document/d/1LXl0aUKAZk9jpnKKGZZxrw3VFRxjAdLqa2WZriSj6Fc/edit as
Add support to Hephaestus for buildkit secrets(see alternative approach in #101)Triggering an environment build (existing code without this PR)
Mounting secrets in a
Dockerfileusage should be documented in https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---mounttypesecretWithout the changes in this PR, the following already works in a
Dockerfilebecauseca-certificates.crtis already mounted automaticallyRUN --mount=type=secret,id=ca-certificates.crt cat /run/secrets/ca-certificates.crtThe installation of Hephaestus could be modified to add another secret via the Helm chart values like:
The
GlobalSecretreference simply adds another name for the existing embedded secret file.Triggering a build that consumes a referenced k8s secret
Installation in existing cluster
Given configuration like
Add a secret to the cluster like this, making sure it has the label
hephaestus-access: "true"so that it can be accessed:Initial Validation
The code at cfdf3b3 has been temporarily modified to always inject a reference to this secret into the incoming
ImageBuildspecification to make testing easierBuild output available at https://mldemo4507.train-sandbox.domino.tech/environments/649f7bbcbb2e9b737895a60d/revisions/64a31bf0bb2e9b737895a63a/build/64a31bf0bb2e9b737895a63c/logs demonstrates the secrets are accessible
Subsequent Validation
I was trying to get the functional tests going to valid this in automation, but it looks like that's going to be pretty hard to do at the moment since they're not working
I was able to pull a little trick with the
Dockerfilefrom that attempt to pull the images like so, given Github is hosting the archive.tgz used for the context in a commit at https://github.com/dominodatalab/hephaestus/raw/51585793939ab0de611bea74ab14e29d846b2070/test/functional/testdata/docker-context/secrets/archive.tgzGiven secret.yaml like
And an image definition to apply like so (based on a previous environment build in my cluster)
I was able to get it to read back / emit the secret