Skip to content
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

vendor: buildkit 4d1f260e8490ec438ab66e08bb105577aca0ce06 #2656

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

thaJeztah
Copy link
Member

full diff: moby/buildkit@df35e98...4d1f260

- Description for the changelog

@thaJeztah
Copy link
Member Author

@tonistiigi @tiborvass @AkihiroSuda PTAL if the changes for secrets and git require documentation changes and if local changes are needed to expose the functionality

@thaJeztah
Copy link
Member Author

Ah, guess this answers some of my questions;

#14 0.375 Building statically linked build/docker-linux-amd64
#14 26.34 # github.com/docker/cli/cli/command/image
#14 26.34 cli/command/image/build_buildkit.go:427:15: undefined: secretsprovider.FileSource
#14 26.34 cli/command/image/build_buildkit.go:435:16: undefined: secretsprovider.NewFileStore
#14 26.34 cli/command/image/build_buildkit.go:442:34: undefined: secretsprovider.FileSource
#14 26.34 cli/command/image/build_buildkit.go:449:8: undefined: secretsprovider.FileSource
#14 ERROR: executor failed running [/bin/sh -c ./scripts/build/binary]: runc did not terminate sucessfully

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #2656 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2656      +/-   ##
==========================================
+ Coverage   58.54%   58.55%   +0.01%     
==========================================
  Files         296      296              
  Lines       21286    21293       +7     
==========================================
+ Hits        12462    12469       +7     
  Misses       7915     7915              
  Partials      909      909              

@tonistiigi
Copy link
Member

Spotted possible issue, fixed in moby/buildkit#1613 . Don't need to be vendored but just same fix can be done in cli.

@thaJeztah
Copy link
Member Author

I'm a bit confused by the UX. Looking at it, it doesn't seem consistent.

The --secret flag currently supports these keys: type, id, source, src, env

  • id is a required option
  • type is full ignored, unless an invalid type is specified (anything other than "file" or "env" (the code doesn't follow different paths if file or env is specified)
  • both source, src and env can be specified at the same time.
    • this is ambiguous if no explicit "type" is set
  • if type is set to "env", source / src acts as an alias for env (if unset)
  • if only id is set (and no source / src or env), and an environment variable is found that matches that name, the default is now switched to "take value from the environment variable"
    • This is a breaking change: previously id would be used as filename

I think

  • if no type is set, and no source / src / env, use the current default and use id as source (current behavior)
  • if no type is set, but source / src is set; implicitly assume type=file (could be seen as same as above, as file is default)
  • if no type is set, but env is set; implicitly assume type=env
  • if type=file
    • only look at source / src (ignore env)
    • fall back to id if not set
    • produce error if env is set? (invalid option for type=file ?
  • if type=env
    • look at env to use as "source"
    • fall back to source / src if not set
    • fall back to id if not set
    • produce error if both env and source / src are set?

However, from the above; does env= provide much value, given that we already have type=env, and that source/src and id already provide the functionality?
Effectively if would only be useful to allow switching between file and env without having to explicitly set type=.

One "feature" could be to explicitly allow env as a "fallback", so;

--secret id=foo-token,src=~/my-token.txt,env=FOO_TOKEN

Which would either pick the value from $FOO_TOKEN or from ~/my-token.txt (not sure in which order though)

If we would remove env=, the combinations would be reduced quite a bit;

  • if type=file or if no type is set (default)
    • look at source / src
    • fall back to id if not set
    • produce error if the file was not found
  • if type=env
    • look at source / src
    • fall back to id if not set
    • produce error if no environment variable with the given name was found? (TBD)

default:
return nil, errors.Errorf("unexpected key '%s' in '%s'", key, field)
}
}
if typ == "env" {
Copy link
Member Author

@thaJeztah thaJeztah Aug 13, 2020

Choose a reason for hiding this comment

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

need to add this

Suggested change
if typ == "env" {
if typ == "env" && fs.Env == "" {

full diff: moby/buildkit@df35e98...4d1f260

- moby/buildkit#1551 session: track sessions with a group construct
- moby/buildkit#1534 secrets: allow providing secrets with env
- moby/buildkit#1533 git: support for token authentication
- moby/buildkit#1549 progressui: fix logs time formatting

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants