Skip to content

add secrets property to build section#238

Merged
ndeloof merged 4 commits intocompose-spec:masterfrom
glours:add-secrets-to-build
Apr 11, 2022
Merged

add secrets property to build section#238
ndeloof merged 4 commits intocompose-spec:masterfrom
glours:add-secrets-to-build

Conversation

@glours
Copy link
Contributor

@glours glours commented Mar 10, 2022

What this PR does / why we need it:
Add a secrets property to the build section which will allow Specification implementations to use secrets during the building step.

Which issue(s) this PR fixes:
Refer to #233

@glours glours self-assigned this Mar 10, 2022
This was referenced Mar 10, 2022
Closed
Merged
ndeloof
ndeloof requested changes Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some minor adjustments

schema/compose-spec.json Outdated
Comment on lines +106 to +118
"oneOf": [
{"type": "string"},
{
"type": "object",
"properties": {
"source": {"type": "string"},
"target": {"type": "string"},
"uid": {"type": "string"},
"gid": {"type": "string"},
"mode": {"type": "number"}
},
"additionalProperties": false,
"patternProperties": {"^x-": {}}
Copy link
Collaborator

@ndeloof ndeloof Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this model is already declared twice for service configs and secrets, maybe this is a good opportunity to extract it into it's own definition and use a $ref

Copy link
Contributor Author

@glours glours Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure 👍

EricHripko
EricHripko reviewed Mar 10, 2022
View reviewed changes
Copy link
Collaborator

@EricHripko EricHripko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into this! A few high-level questions first ❓

  • Does this allow for secrets to be optional?
  • What would the workflow look like for creating a secret? I'm aware that this crosses somewhat into implementation, but knowing the workflow E2E will help validate the schema changes.

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 10, 2022

@EricHripko this is basically a port of docker build --secret id=mysecret,src=/local/secret that is consistent with the way docker compose exposes secrets as a bind mount. Obviously, this also can apply to platforms with native secret support, where user might have registered some secret and want them to be available during the build as /run/secret/id.

@EricHripko
Copy link
Collaborator

EricHripko commented Mar 10, 2022

Thank you for confirming 👍 Is the idea that the user calls compose build --secret ... then?

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 10, 2022

The idea is that use can declare an entry in secrets section in yaml file, and use a reference in service.build.secrets so that this secret is exposed during the build process (comparable to the same mechanism used when running services)
secret definition can either include a file (typically, some ~/.secrets-token) that each user will have to set, or could use an external: true secret for platforms with native support for secrets.

@glours glours force-pushed the add-secrets-to-build branch from 9fe7290 to 02f367e Compare March 10, 2022 11:32
@glours glours requested a review from ndeloof March 10, 2022 11:32
@EricHripko
Copy link
Collaborator

EricHripko commented Mar 11, 2022

That makes sense, thank you for clarifying 👍 One last question, would this still allow for optional secrets?

@glours glours mentioned this pull request Mar 15, 2022
Closed
glours and others added 3 commits March 31, 2022 15:54
@glours
add secrets property to build section
5eb35fc 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours @ndeloof
Update when compose implementations should report an error with secre…
5253a4b 
…ts build property

Co-authored-by: Nicolas De loof <nicolas.deloof@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours
define a json ref for service secrets and confids properties
c607167 
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours glours force-pushed the add-secrets-to-build branch from 02f367e to c607167 Compare March 31, 2022 13:54
ulyssessouza
ulyssessouza approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndeloof ndeloof mentioned this pull request Apr 11, 2022
Closed
@ndeloof
Copy link
Collaborator

ndeloof commented Apr 11, 2022

One last question, would this still allow for optional secrets?

The secrets mechanism define in the compose specification has no optional capability. This is something we could consider for a future improvements, but then would apply to all secrets, not just the build secrets

@ndeloof ndeloof merged commit 846cabc into compose-spec:master Apr 11, 2022
@EricHripko
Copy link
Collaborator

EricHripko commented Apr 11, 2022

It would be great to allow for optional secrets, as development/CI/production environments can have different secrets (or none at all) even when it comes to build time.

@ciaranmcnulty ciaranmcnulty mentioned this pull request Apr 11, 2022
Closed
@ndeloof
Copy link
Collaborator

ndeloof commented Apr 11, 2022

different secrets makes sense to me (so the ability to provide overrides on compose file, or use paths relative to user's home ~/ to define a secret). I hardly understand how no secret would be used, as this would mean resource can be accessed without a secret? Maybe better discus ability to declare optional secrets in a separate proposal

@Kludex Kludex mentioned this pull request Apr 19, 2022
Closed
@EricHripko
Copy link
Collaborator

EricHripko commented Apr 29, 2022

Sure, I agree that this can be discussed in a separate thread. Let me get some resources ready to see if there is a substantial justification for such functionality 👍

@GhandiJones
Copy link

GhandiJones commented Feb 9, 2023

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EricHripko EricHripko EricHripko left review comments

@ndeloof ndeloof ndeloof approved these changes

@ulyssessouza ulyssessouza ulyssessouza approved these changes

Assignees

@glours glours

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@glours @ndeloof @EricHripko @GhandiJones @ulyssessouza