Bug 1940488: add etc-pki-entitlements from pod secrets if available to build container#228
Conversation
|
@gabemontero: This pull request references Bugzilla bug 1940488, which is invalid:
Comment DetailsIn 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. |
|
/bugzilla refresh |
|
@gabemontero: This pull request references Bugzilla bug 1940488, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
|
Hmmm .... perhaps new req's wrt ci and the images used being mirrored: /test e2e-aws-image-ecosystem while I dig into this |
yep looks like openshift/origin#25958 was merged yesterday and enforces restrictions not previously enforced bet the https://github.com/openshift/origin/blob/master/test/extended/image_ecosystem/scl.go and https://github.com/openshift/origin/blob/master/test/extended/image_ecosystem/sample_repos.go combo is leading to this |
| func generateTransientMounts() []string { | ||
| mounts := []string{} | ||
| mounts = appendRHSMMount(mounts) | ||
| mounts = appendETCPKIMount(mounts) |
There was a problem hiding this comment.
We also need to mount the redhat.repo file. This is only one file.
There was a problem hiding this comment.
right, but how to handle a top level file vs. a dir with the buildah temporary mount options is not obvious to me @adambkaplan @nalind since all the man pages / info that I can find only talks in the context of dirs
or can I ultimate create an
"tmpdir/redhat.repo:/run/secrets/redhat.repo:rw,nodev,noexec,nosuid"
entry in the array of strings fed into the buildah transient mounts?
There was a problem hiding this comment.
That should work, but processes that try to modify it may get confused by it being a bind mount - it can't be removed, and nothing can be renamed on top of it.
There was a problem hiding this comment.
thanks @nalind
I think that should be OK
Or am I forgetting something @adambkaplan ?
I'll cook up the change in the interim
There was a problem hiding this comment.
I am a little curious how our e2e that does the recursive chmod would do if we had host entitlements in CI
I'm going to update the BZ to make sure QE covers this as well
b4c063d to
f61e907
Compare
|
CI fail on e2e-aws-builds ... tests never ran: |
|
/test e2e-aws-builds |
|
still waiting on openshift/origin#25985 for image eco |
|
/retest |
|
and all green tests @adambkaplan |
adambkaplan
left a comment
There was a problem hiding this comment.
Realized in the review that "subscription content" is a more user friendly term than "entitlements". We should update the log messages to reflect that.
In addition, if my understanding is correct the only thing a user has to have to access subscription content are the entitlement keys. redhat.repo and rhsm mounts are optional, and are used on nodes that talk to Satellite. If these aren't present, UBI8 falls back to using the Red Hat CDN to access content. Thus, I've suggested softening the language from "will not" to "may not" in those error messages.
|
One other thought - should we log all of these error messages as V(4)? Thought here:
|
my experience has been the opposite with this stuff ... the errors happen infrequently, but when they do, we have to go through a round trip with the customer getting trace As your the approver here :-) you can ultimately make the decision, but going with V(0) was done intentionally on my part. |
d5939eb to
36d571f
Compare
|
I accepted all your suggested commits @adambkaplan , pulled them down, squashed them, and re-pushed |
|
quay.io outage/degredation noted in announce-testplatform bit e2e-aws-builds /test e2e-aws-builds |
|
/bugzilla cc-qa |
|
@wewang58: This pull request references Bugzilla bug 1940488, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
I'll defer to your experience, then. Update the error logs to V(0) so they are consistent. |
|
/approve Otherwise looks good. Marking approved so anyone on the team can lgtm once the log statement is updated. |
ab0afec to
7382af7
Compare
|
/test e2e-aws |
|
/assign @coreydaley |
3b0a5cd to
315b4c4
Compare
| log.V(0).Infof("Red Hat entitlements will not be available in this build: rhsm secrets location /run/secrets/rhsm is not a directory") | ||
| if st.IsDir() { | ||
| // if the file is there, but an unexpected type, then always have log show up via V(0) | ||
| log.V(0).Infof("Falling back to the Red Hat yum repository configuration in the build's base image: redhat.repo secrets location /run/secrets/redhat.repo is a directory.") |
There was a problem hiding this comment.
This should probably reference the path variable instead of hard coding the /run/secrets/redhat.repo in case it changes in the future.
There was a problem hiding this comment.
part of next push
| } | ||
| if !st.IsDir() { | ||
| log.V(0).Infof("Red Hat entitlements will not be available in this build: rhsm secrets location /run/secrets/rhsm is not a directory") | ||
| if st.IsDir() { |
There was a problem hiding this comment.
if !st.isFile() might be more clear here
There was a problem hiding this comment.
not 100% sure that covers symlinks
I'll try it with the new unit tests and report back
There was a problem hiding this comment.
also fyi there is no st.IsFile() method
the closest is st.Mode().IsRegular()
There was a problem hiding this comment.
Yep, that is what I was looking at: https://pkg.go.dev/gopkg.in/src-d/go-git.v4@v4.13.1/plumbing/filemode#FileMode.IsFile
There was a problem hiding this comment.
ok it works .... part of next push
| func appendRHSMMount(mounts []string) []string { | ||
| st, err := os.Stat("/run/secrets/rhsm") | ||
| func appendRHRepoMount(pathStart string, mounts []string) []string { | ||
| path := filepath.Join(pathStart, "redhat.repo") |
There was a problem hiding this comment.
redhat.repo is used several places here, would it be worth using a constant for this as well?
There was a problem hiding this comment.
I'll do it for all 3 files / dirs ... part of next push
|
updates pushed as seperate commit @coreydaley ptal i'll squash if you are good |
|
@gabemontero looks good, squash away |
…o build container
86e04d6 to
ba2162e
Compare
commits squashed |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, coreydaley, gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
|
@gabemontero: All pull requests linked via external trackers have merged: Bugzilla bug 1940488 has been moved to the MODIFIED state. DetailsIn 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. |
|
I'll cherrypick this after QE verifies |
/assign @adambkaplan
@nalind FYI