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.

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

Fix /etc/pki/ca-trust/extracted/pem permissions issue #263

Conversation

akhil-rane
Copy link
Contributor

@akhil-rane akhil-rane commented Oct 28, 2020

Currently, CCO e2es are failing because CI builds volume mount
'/etc/pki/ca-trust' at runtime. Previously we used Dockerfile RUN
command to make '/etc/pki/ca-trust/extracted/pem' world-writable.
Because this command is now being run with a volume mounted at
/etc/pki/ca-trust, the directories and files whose permissions are
modified are those in the volume and are not included in the final
image.

To fix this we add an archive of the contents (writable empty files)
of '/etc/pki/ca-trust/extracted' to the build context using Dockerfile
ADD and then run 'update-ca-trust extract' as a container
command to populate these files

jira: https://issues.redhat.com/browse/CO-1272

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2020
@akhil-rane
Copy link
Contributor Author

Testing this fix

@dgoodwin
Copy link
Contributor

Let me know if I'm wrong but I wouldn't expect this to work, if something is now mounting to this location at runtime I don't think changing the perms during a build will help. I expect the fix needs to be a new way to get the cert in there, and in the command in the Deployment yaml where we do the copy today. It looked like there were some suggestions for a command to run to import certs now.

@akhil-rane
Copy link
Contributor Author

Let me know if I'm wrong but I wouldn't expect this to work, if something is now mounting to this location at runtime I don't think changing the perms during a build will help. I expect the fix needs to be a new way to get the cert in there, and in the command in the Deployment yaml where we do the copy today. It looked like there were some suggestions for a command to run to import certs now.

Agreed. I just wanted to test what happens in CI for this case by fetching the image.

@akhil-rane akhil-rane force-pushed the fix_permissions_issue_trust_bundle branch from e6fb833 to 728891f Compare October 29, 2020 21:25
@akhil-rane
Copy link
Contributor Author

akhil-rane commented Oct 30, 2020

@dgoodwin what do you think of this approach? instead of /etc/pki/ca-trust/extracted we make /etc/pki/ca-trust/extracted/pem as our mounted volume and just copy trust bundle in there. Do not need to run update-ca-trust extract here. I am having hard time running update-ca-trust extract inside a container. The container just goes into error state with nothing in the logs or the status. I might be doing something wrong there, trying to figure it out.

@dgoodwin
Copy link
Contributor

I'm really not sure, it feels unsafe but it would be great if it's a reasonable solution. Would you mind asking in that email thread started this morning and see what folks more familiar with think?

@joelddiaz
Copy link
Contributor

How are other operators handling this? CCO is not alone in having to set this up.

@akhil-rane
Copy link
Contributor Author

How are other operators handling this? CCO is not alone in having to set this up.

Only CCO and image registry repos have reported issues so far. It is not fixed yet in the Image registry. I do know if any other operators are facing this.

@akhil-rane akhil-rane force-pushed the fix_permissions_issue_trust_bundle branch from 728891f to 922048a Compare November 2, 2020 18:52
@akhil-rane
Copy link
Contributor Author

/retest

@akhil-rane akhil-rane force-pushed the fix_permissions_issue_trust_bundle branch from 922048a to d296bef Compare November 3, 2020 15:27
@akhil-rane
Copy link
Contributor Author

/assign @dgoodwin
/cc @joelddiaz

Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

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

Can you update the commit text with an explanation of what this writeable-extracted.tar.gz file is and why/how it fixes things?
Othewise 👍

@dgoodwin
Copy link
Contributor

dgoodwin commented Nov 4, 2020

Same here, just want a clear description of what that is and how it works, and it should probably be in the commit message and comments around the Dockerfile line.

Otherwise looks great indeed and nice work.

@dgoodwin
Copy link
Contributor

dgoodwin commented Nov 5, 2020

I believe they're going to revert the change that caused this, however it will probably take time to make it to the build/ci clusters, so I think we probably should still merge this?

@akhil-rane
Copy link
Contributor Author

I believe they're going to revert the change that caused this, however it will probably take time to make it to the build/ci clusters, so I think we probably should still merge this?

Yes, I think we should merge. I will update the PR with the commit message.

Currently, CCO e2es are failing because CI builds volume mount
'/etc/pki/ca-trust' at runtime. Previously we used Dockerfile RUN
command to make '/etc/pki/ca-trust/extracted/pem' world-writable.
Because this command is now being run with a volume mounted at
/etc/pki/ca-trust, the directories and files whose permissions are
modified are those in the volume and are not included in the final
image.

To fix this we add an archive of the contents (writable empty files)
of '/etc/pki/ca-trust/extracted' to the build context using
Dockerfile ADD and then run 'update-ca-trust extract' as a container
command to populate these files
@akhil-rane akhil-rane force-pushed the fix_permissions_issue_trust_bundle branch from d296bef to 2a8b86d Compare November 5, 2020 22:53
@akhil-rane akhil-rane changed the title WIP: Fix /etc/pki/ca-trust/extracted/pem permissions issue Fix /etc/pki/ca-trust/extracted/pem permissions issue Nov 5, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2020
@akhil-rane
Copy link
Contributor Author

akhil-rane commented Nov 5, 2020

@dgoodwin I have updated the commit message to the best of my knowledge. Let me know if it does not make sense. I might be wrong about some of the things here.

I believe changes to previous releases won't clear CI if we don't backport. So do we want to backport or wait for the CI team to revert the change that caused this issue?

@dgoodwin
Copy link
Contributor

dgoodwin commented Nov 6, 2020

/lgtm

Need to think on backporting next week, I have fixes blocked for 4.5 in the queue, we may have to clone a bug for 4.6 and 4.5 both, and backport to each.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akhil-rane, dgoodwin

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit 46b4254 into openshift:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants