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

Secrets enable in Docker plugin with support for passing environment variables as secrets #332

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

wszychta
Copy link

@wszychta wszychta commented Sep 16, 2021

PR purpose

This PR is providing three new settings to the plugin:

  • secrets_from_env - Pass environment variables as secret file into the build
  • secrets - Provide full secrets paths from local filesystem
  • secret_separator - separator between each of the secrets variable

Also when any secrets or secrets_from_env are provided then environment variable DOCKER_BUILDKIT is set to 1. If nothing is provided then plugin willl not create/change that variable.

Warning

Because of urfave/cli library is not able to Slice Flags with different sign than , then we have introduced another variable called secret_separator. The content of this variable will be replaced with , on docker build command creation.

Example usage

.drone.yml:

- name: test Docker
  image: plugins/docker
  environment:
    TEST_VAR:
      from_secret: testvar
  settings:
    dry_run: true
    repo: foo/bar
    tags: latest
    dockerfile: Dockerfile
    secrets_from_env:
      - TEST_VAR
    secret_separator: ":"
    secrets: 
      - id=test-secret:src=test-file.txt
      - id=test-secret2:src=test-file2.txt

Dockerfile:

# syntax=docker/dockerfile:1.2
ARG IMAGE_VERSION=latest
FROM ubuntu:${IMAGE_VERSION}
RUN --mount=type=secret,id=test_var,dst=/root/test_var.txt cat /root/test_var.txt
RUN --mount=type=secret,id=test-secret,dst=/root/test-secret.txt cat /root/test-secret.txt
RUN --mount=type=secret,id=test-secret2,dst=/root/test-secret2.txt cat /root/test-secret2.txt

Other work to do

This PR requires update to drone docker plugin documentation

@bradrydzewski
Copy link
Member

👋 Hey there, while I appreciate the pull request, I recommend reading this thread:
https://discourse.drone.io/t/drone-netrc-username-documentation-usage/4485/2

The netrc file is considered Internal use only and is subject to change. We are working to narrow the use of the netrc file and if we can find a suitable alternative, we may consider removing in the future. If you want to inject git credentials into this plugin it should be done using secrets.

In terms of using docker secrets, it might make more sense to model this behavior after build_args_from_env as opposed to reading values from a file. I recommend working with the maintainers to design the yaml syntax before writing additional code.

Copy link
Member

@bradrydzewski bradrydzewski left a comment

Choose a reason for hiding this comment

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

please remove netrc functionality from this pull request. we should scope this pull request around the use of the --secrets flag.

@wszychta
Copy link
Author

@bradrydzewski I have modified code as you suggested. I have also modified my Pull Request description with new changes. Right now this Pull Request doesn't create netrc file automatically.

@bkk-bcd
Copy link

bkk-bcd commented Sep 21, 2021

The build is failing on go vet, that doesn't seem expected?

@bradrydzewski this looks like something we could use today, can you take another look to help get this merged in?

@bkk-bcd
Copy link

bkk-bcd commented Sep 21, 2021

Looks like the build container is using go 1.11 while go mod requires go 1.13.

Go 1.13 has os.UserHomeDir()

Not until go 1.16 does os.WriteFile() exists...maybe we can use https://pkg.go.dev/[email protected]#OpenFile instead or https://pkg.go.dev/io/ioutil (see https://golang.org/doc/go1.16#ioutil for explanation)

@bradrydzewski
Copy link
Member

bradrydzewski commented Sep 21, 2021

I think a more thorough design review is going to be required and I want to be up front that this will take some time. We will assign this to a team member in an upcoming sprint and we will post updates to this thread when we have more details to share. We have a strong commitment to backward compatibility which means once this feature is merged it cannot easily be changed, so we need to make sure we are happy with the design and implementation.

@wszychta wszychta changed the title Secrets enable in Docker plugin with support for netrc file Secrets enable in Docker plugin with support for passing environment variables as secrets Sep 27, 2021
@wszychta
Copy link
Author

wszychta commented Sep 27, 2021

@bradrydzewski ok, I will wait until one of your teammates will review this PR in next sprint.
Also I would suggest to change go version on vet to newer than 1.11.
Locally I'm working on go 1.16 and I didn't have any problems with docker and ECR plugins, but I was unable to check other plugins.

@alexef
Copy link

alexef commented Oct 25, 2021

Any luck getting some time to review this PR?

@wszychta
Copy link
Author

@alexef it looks like plugin maintainers upgraded plugin test to golang 1.13
If I will have time, then mayby I will try to downgrade golang to the same version and rewrite my PR code.
@bradrydzewski Do you have any plans on reviving this PR?

@alexef
Copy link

alexef commented Oct 19, 2022

Looks like this could be closed now that #359 was merged.

@wszychta
Copy link
Author

wszychta commented Oct 19, 2022

@alexef I'm not working for the company which was requesting this feature. I will contact somebody from them and ask for comment here.

@ghost
Copy link

ghost commented Oct 19, 2022

We will, test out the release https://github.com/drone-plugins/drone-docker/releases/tag/v20.12.0, thats is refered in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants