-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for BuildKit secrets #36
Conversation
Any chance this can be merged? This is very important to avoid leaking secrets into images history https://pythonspeed.com/articles/docker-build-secrets/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, and sorry for the wait! There's a merge conflict that needs to be fixed, and I have some minor feedback.
@@ -69,6 +69,16 @@ Next, any of the following optional parameters may be specified: | |||
DO_THING=false | |||
``` | |||
|
|||
* `$BUILDKIT_SECRET_*`: extra secrets which are made available via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit curious as to whether to call this BUILDKIT
or BUILD
, since I'm trying to keep from exposing any buildkit-specific flags in this image so that the building can be swapped out with e.g. Kaniko or some other builder in the future. But it looks like the secret mounting stuff is pretty buildkit-specific, with the Kaniko flow being to just tuck credentials under /kaniko
which is ignored wholesale.
Since there doesn't seem to be a general way of handling this across image building tools, I'm OK with just being upfront and calling it BUILDKIT
. 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does feel a bit gross putting specific flags in for a given tool (buildkit, in this case) into this resource. Maybe, in the future, would it be worth having multiple tool-specific build resources, eg. buildkit-build-task
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe - I had hoped to hide this all away so I can change out the details later, but perhaps it would have been easier to just be explicit about the toolchain. Food for thought for when we convert this to a Prototype. :)
task_test.go
Outdated
@@ -43,7 +43,9 @@ func (s *TaskSuite) SetupTest() { | |||
|
|||
s.req = task.Request{ | |||
ResponsePath: filepath.Join(s.outputsDir, "response.json"), | |||
Config: task.Config{}, | |||
Config: task.Config{ | |||
BuildkitSecrets: make(map[string]string), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's cleaner to not have to fill this in, since this is suite-wide setup. (1/2)
task_test.go
Outdated
@@ -142,6 +144,14 @@ func (s *TaskSuite) TestBuildArgsStaticAndFile() { | |||
s.NoError(err) | |||
} | |||
|
|||
func (s *TaskSuite) TestBuildkitSecrets() { | |||
s.req.Config.ContextDir = "testdata/buildkit-secret" | |||
s.req.Config.BuildkitSecrets["secret"] = "testdata/buildkit-secret/secret" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2/2) - so can this just set the whole value instead?
s.req.Config.BuildkitSecrets["secret"] = "testdata/buildkit-secret/secret" | |
s.req.Config.BuildkitSecrets = map[string]string{"secret": "testdata/buildkit-secret/secret"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yep, that makes sense. Still getting my head around Go. I'll make this change now and test it in our environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks!
Buildkit supports the ability to mount secrets into certain steps of a dockerfile, via
--mount=type=secret,id=[secret-id]
. I've attempted to add support for setting up these secrets via theBUILDKIT_SECRET_[secret-id]
environment variables.I've been able to test this in Concourse and build an image which uses credentials passed in via a secret 🙂