Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
build: Support Mounted Resource Volumes #733
build: Support Mounted Resource Volumes #733
Changes from all commits
b310d8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
/cc @nalind
For mount options supported by buildah.
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.
The library accepts options for controlling propagation ("shared", "rshared", "private", "rprivate", "slave", "rslave"), regular options ("rw, "ro", "dev"/"nodev", "suid"/"nosuid", "exec"/"noexec"), and SELinux labeling controls that I think end up being ignored since we're already in a container ("Z", "z").
There are also a couple of custom ones: "U" to have the ownership of the volume's contents changed to match the current USER value, and "O" which causes the volume source to be used as the lower layer for an overlay mount at the mount point, using a per-run temporary directory as the upper (write) layer. I don't know that either of those is particularly useful in builds, and of course their functionality willl be limited in the future by the permissions that the builder binary has.
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.
Good to know - we will start with ReadOnly
ro
as the only supported option.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.
we should be able to generate our own volume names(both on the pod and in buildah) such that we avoid collisions. We don't have to use the volume name the user provides, when setting up volumes.
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.
True about creating our own volume names, though we get into fun situations where we need to validate/truncate superficially long volume names.
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.
@bparees I made this a "we can change this in a future enhancement" given the short window we have to land this in 4.8
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.
that's reasonable.
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.
btw i wasn't suggesting random suffixes. you don't have to use any part of the user given name, you can just call it "vol1", "vol2", etc. So no issue w/ truncation. Again we already do this when mounting the secrets/configmaps into the build pod. (generate our own volume names)
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.
Currently, when multiple volumes are set to be mounted at a given location, we choose one of them and proceed. This will require some patching.
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 this is something we can do in openshift/builder or in the build controller. Buildah itself doesn't have to fail here (unless you believe this is buggy behavior).
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.
is there a risk that a user can create a build that mounts a secret or configmap that they themselves don't have permission to view?
i think we mitigate that in the existing secret injection logic, though i don't recall how. We need to ensure it is mitigated here as well.
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 don't see any such logic in the build controller. Secrets and ConfigMap volume mounts are local object references in the pod spec. My understanding is that if a Secret exists in a namespace, it can be used by any service account in said namespace.
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 guess it is constrained by "you can't create builds if you can't create pods"