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.

Sign up for GitHub

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

Jump to bottom

history, build: add pre-allowed variables e.g proxy vars to history if explicitly specified #3782

Merged
merged 1 commit into from
Feb 15, 2022
Merged

history, build: add pre-allowed variables e.g proxy vars to history if explicitly specified #3782

merged 1 commit into from
Feb 15, 2022

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Feb 15, 2022 •
edited
Loading

Buildkit/Docker only adds pre-allowed variables for e.g proxy variables to history if user explicitly specifies them in
Dockerfile/Containerfile using ARG.

By default variables in allowlist e.g proxy variables and values will be used
normally but will not be leaked into docker/OCI history of images.

A test for following behavior is added with this commit and similar test
can be verified against Docker/Buildkit

Closes: #2937

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flouthoc flouthoc force-pushed the silent-predefined-args-if-not-in-args branch from dec150f to 33fc349 Compare February 15, 2022 09:22
@flouthoc flouthoc requested review from nalind and rhatdan February 15, 2022 09:23
vrothberg
vrothberg reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Code LGTM, just a couple of nits.

Can you do a s/whitelist/allowlist/? "Whitelisting" is an old non-inclusive term that we want to avoid.

Can you add Fixes: https://github.com/containers/buildah/issues/2937 to the commit message? That would add some useful information to the git history.

imagebuildah/stage_executor.go Outdated Show resolved Hide resolved
imagebuildah/stage_executor.go Outdated Show resolved Hide resolved
imagebuildah/stage_executor.go Outdated Show resolved Hide resolved
imagebuildah/stage_executor.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the silent-predefined-args-if-not-in-args branch 2 times, most recently from 17ace93 to 5bf42d5 Compare February 15, 2022 09:49
@flouthoc flouthoc changed the title history, build: only add pre-whitelisted vars e.g proxy vars to history if explicitly specified history, build: only add pre-allowed variables e.g proxy vars to history if explicitly specified Feb 15, 2022
@flouthoc
Copy link
Collaborator Author

@vrothberg Thanks tweaked comments as requested.

vrothberg
vrothberg reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg 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, LGTM

@flouthoc flouthoc changed the title history, build: only add pre-allowed variables e.g proxy vars to history if explicitly specified history, build: add pre-allowed variables e.g proxy vars to history if explicitly specified Feb 15, 2022
@nalind
Copy link
Member

nalind commented Feb 15, 2022

Can you do a s/whitelist/allowlist/? "Whitelisting" is an old non-inclusive term that we want to avoid.

Test function names, too.

@flouthoc
history: only add proxy vars to history if specified
9c7efb9 
`Buildkit/Docker` adds variables in pre-allowlist for e.g `proxy
variables` to OCI/Docker history only if user explicitly specifies them in
Dockerfile using `ARG`.

By default variables in pre-allowlist e.g `proxy variables` will be used
normally but will not be leaked into `docker/OCI` history of images.

A test for following behviour is added with this commit and similar test
can be verified against `Docker/Buildkit`

Closes: containers#2937

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the silent-predefined-args-if-not-in-args branch from 5bf42d5 to 9c7efb9 Compare February 15, 2022 14:58
@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2022

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm label Feb 15, 2022
@flouthoc
Copy link
Collaborator Author

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit d2a8181 into containers:main Feb 15, 2022
@edsantiago edsantiago mentioned this pull request Feb 21, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@nalind nalind Awaiting requested review from nalind

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

Labels
approved buildkit lgtm locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman history exposes proxy related build args
5 participants
@flouthoc @nalind @rhatdan @vrothberg @openshift-merge-robot