Bug 1895053: Mount CA trust store in builds #218
Conversation
|
@adambkaplan: This pull request references Bugzilla bug 1895053, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/severity-high
adambkaplan
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
| st, err := os.Stat("/run/secrets/rhsm") | ||
| if err != nil { | ||
| return mounts, errors.Wrap(err, "failed to stat /run/secrets/rhsm") | ||
| } |
There was a problem hiding this comment.
@gabemontero this is a refactor of your original code. However, I think we should not return an error.
From our experience, consuming entitlements in builds is more of a "secondary" use case. The worst case is if a build does a yum install and we don't provide the entitlement keys, the build fails at yum install time. I don't think we should fail all builds because we can't set up the rhsm data.
There was a problem hiding this comment.
sure I'm good with that @adambkaplan
pkg/build/builder/daemonless.go
Outdated
| mountCA, err := strconv.ParseBool(mountCAEnv) | ||
| if err != nil { | ||
| log.V(5).Infof("failed to parse BUILD_MOUNT_ETC_PKI_CATRUST: %v", err) | ||
| return mounts, errors.Wrap(err, "failed to parse BUILD_MOUNT_ETC_PKI_CATRUST") |
There was a problem hiding this comment.
Likewise I don't think we should fail a build if we can't parse this env var or we don't find the trust bundle. If a build depends on this information, it will eventually fail.
| } | ||
|
|
||
| log.V(0).Infof("Adding transient ro bind mount for /etc/pki/ca-trust") | ||
| mounts = append(mounts, "/etc/pki/ca-trust:/etc/pki/ca-trust:ro") |
There was a problem hiding this comment.
@nalind is this the right setting for a read-only mount? What do the nodev,noexec,nosuid arguments do for the rhsm mount?
There was a problem hiding this comment.
Yes, that's the right flag for making the bind read-only. The nodev,noexec,nosuid arguments request that in the new mountpoint, device nodes not be accessible, binaries not be executable despite the execute bit on their permissions, and that the setuid/setgid bit and file capabilities not be honored.
pkg/build/builder/daemonless.go
Outdated
|
|
||
| mountCA, err := strconv.ParseBool(mountCAEnv) | ||
| if err != nil { | ||
| log.V(5).Infof("failed to parse BUILD_MOUNT_ETC_PKI_CATRUST: %v", err) |
There was a problem hiding this comment.
I'm fine with not failing on the parse error vs. later on @adambkaplan but if so I would prefer our clue/log here is not V(5) .... IMO a V(0) bread crumb is appropriate
pkg/build/builder/daemonless.go
Outdated
|
|
||
| st, err := os.Stat("/etc/pki/ca-trust") | ||
| if err != nil { | ||
| log.V(5).Infof("failed to stat /etc/pki/ca-trust: %v", err) |
There was a problem hiding this comment.
I am fine with this staying v(5) .... presumably it is not as susceptible to user error
- Add MountTrustedCA field to builds. This ensures we serialize the build JSON correctly.
- Add the build container's CA trust store as a transient mount for buildah if the `BUILD_MOUNT_ETC_PKI_CATRUST` environment variable is set to a "true" value. - Log "warning" that RHEL entitlements aren't available if we can't copy /run/secrets/rhsm data. - Log "warning" that custom PKI trust bundles aren't available if we can't find /etc/pki/ca-trust.
adambkaplan
force-pushed
the
build-mount-pki-trust
branch
from
f265c10
to
1ece4f9
Compare
adambkaplan
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@gabemontero this is ready for final review. |
|
/retest Builds hit known kubelet flake. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, gabemontero 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@adambkaplan: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 1895053 has not been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Add the build container's CA trust store as a transient mount for buildah
if the
BUILD_MOUNT_ETC_PKI_CATRUSTenvironment variable is set to a"true" value.