DEVEXP-423: Mounting node's pull secrets on build pod. #76
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/test |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
ricardomaraschini
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @gabemontero @adambkaplan |
| { | ||
| Name: "node-pullsecrets", | ||
| VolumeSource: v1.VolumeSource{ | ||
| HostPath: &v1.HostPathVolumeSource{ |
There was a problem hiding this comment.
So this comment here @ricardomaraschini is as much of a broader comment for builds in general as it is for this piece (though we could start addressing things with your changes).
Namely, we've added another thing that necessitates a privileged pod.
So, thoughts that come to mind @ricardomaraschini @adambkaplan @bparees :
- we have reqs on the books for devising a build solution that does not require privileged pods, no?
- if I'm wrong there, ignore the rest :-)
- are we hoping to address that with build v1, or will we defer to build v2 for this?
- if not build v1, ignore the rest :-)
- do we have the concise list of places we need privileged? and if so, should this feature be added to that list?
- and finally, to facilitate that future work, should we start abstracting out privileged requiring elements like this into utility methods, decorating/mutating abstractions, that in the future could consume a config setting and if these privileged elements should be added to the build pod?
thanks
There was a problem hiding this comment.
we have reqs on the books for devising a build solution that does not require privileged pods, no?
Unprivileged builds won't happen until we drop support for RHEL7 workers. We need fuse-overlay to give us some level of performance parity.
are we hoping to address that with build v1, or will we defer to build v2 for this?
It's unclear at this point if builds v2 will be the place where we land the unprivileged builds feature. I like this idea since it lowers the barrier for the upstream community to consume. I.e. builds v2 could be a regular workload on any k8s cluster, but it works best if you have Fedora CoreOS/RHCOS as your host nodes.
do we have the concise list of places we need privileged? and if so, should this feature be added to that list?
We're already going down the path of adding more host path dependencies with the build image cache enhancement, so I wouldn't worry about this right now.
and finally, to facilitate that future work, should we start abstracting out privileged requiring elements like this into utility methods, decorating/mutating abstractions, that in the future could consume a config setting and if these privileged elements should be added to the build pod?
I'm not inclined to do this right now. In many respects I'd prefer builds v2 avoid host dependencies so that it could run on any k8s cluster. If there's demand for it, then we can look to refactor things out.
There was a problem hiding this comment.
we have reqs on the books for devising a build solution that does not require privileged pods, no?
Unprivileged builds won't happen until we drop support for RHEL7 workers. We need fuse-overlay to give us some level of performance parity.
are we hoping to address that with build v1, or will we defer to build v2 for this?
It's unclear at this point if builds v2 will be the place where we land the unprivileged builds feature. I like this idea since it lowers the barrier for the upstream community to consume. I.e. builds v2 could be a regular workload on any k8s cluster, but it works best if you have Fedora CoreOS/RHCOS as your host nodes.
do we have the concise list of places we need privileged? and if so, should this feature be added to that list?
We're already going down the path of adding more host path dependencies with the build image cache enhancement, so I wouldn't worry about this right now.
and finally, to facilitate that future work, should we start abstracting out privileged requiring elements like this into utility methods, decorating/mutating abstractions, that in the future could consume a config setting and if these privileged elements should be added to the build pod?
I'm not inclined to do this right now. In many respects I'd prefer builds v2 avoid host dependencies so that it could run on any k8s cluster. If there's demand for it, then we can look to refactor things out.
my last comment was meant to be in the context of build v1, not build v2. Regardless, it only has bearing if the RHEL7/fuse_overlay items you noted initially are addressed.
| @@ -39,6 +39,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA | |||
|
|
|||
| privileged := true | |||
| strategy := build.Spec.Strategy.DockerStrategy | |||
| typeFile := v1.HostPathType("File") | |||
There was a problem hiding this comment.
Shouldn't this be a constant in the API?
There was a problem hiding this comment.
It turns out we do have a constant on corev1. I have sent over a commit to address this.
| { | ||
| Name: "node-pullsecrets", | ||
| VolumeSource: v1.VolumeSource{ | ||
| HostPath: &v1.HostPathVolumeSource{ |
There was a problem hiding this comment.
we have reqs on the books for devising a build solution that does not require privileged pods, no?
Unprivileged builds won't happen until we drop support for RHEL7 workers. We need fuse-overlay to give us some level of performance parity.
are we hoping to address that with build v1, or will we defer to build v2 for this?
It's unclear at this point if builds v2 will be the place where we land the unprivileged builds feature. I like this idea since it lowers the barrier for the upstream community to consume. I.e. builds v2 could be a regular workload on any k8s cluster, but it works best if you have Fedora CoreOS/RHCOS as your host nodes.
do we have the concise list of places we need privileged? and if so, should this feature be added to that list?
We're already going down the path of adding more host path dependencies with the build image cache enhancement, so I wouldn't worry about this right now.
and finally, to facilitate that future work, should we start abstracting out privileged requiring elements like this into utility methods, decorating/mutating abstractions, that in the future could consume a config setting and if these privileged elements should be added to the build pod?
I'm not inclined to do this right now. In many respects I'd prefer builds v2 avoid host dependencies so that it could run on any k8s cluster. If there's demand for it, then we can look to refactor things out.
| t.Fatalf("Expected 11 volumes in container, got %d", len(container.VolumeMounts)) | ||
| // node pull secrets | ||
| if len(container.VolumeMounts) != 12 { | ||
| t.Fatalf("Expected 12 volumes in container, got %d", len(container.VolumeMounts)) |
There was a problem hiding this comment.
@ricardomaraschini Need to add the node pull secret to the source strategy (sti) and custom strategy builds, too.
There was a problem hiding this comment.
Thanks! I forgot those. I think we have a problem when it comes to custom strategy as by mounting these secrets one may easily copy them to the outside world, no?
Our documentation states that:
"Custom builds run with a very high level of privilege and are not available to users by default. Only users who can be trusted with cluster administration permissions should be granted access to run custom builds."
What do you reckon?
There was a problem hiding this comment.
good question @ricardomaraschini. my view would be that it's reasonable to leave this out of custom builds for now and force those users to still provide their own pull secrets. People doing custom builds already have to be pretty sophisticated.
If we get a lot of pushback about the pain that causes we can revisit the decision, since as you note, someone doing custom builds also already has to be a highly trusted user.
There was a problem hiding this comment.
@bparees that sounds like a fair compromise to me. We need to doc this in JIRA so we make this clear in the release notes.
|
/retest |
Mounting node pull secrets on build pod, according to: openshift/enhancements/enhancements/node-pull-credentials
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, ricardomaraschini 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 |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Mounting node pull secrets on build pod, according to:
openshift/enhancements/enhancements/node-pull-credentials