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.

Sign up for GitHub

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

Jump to bottom

Bug 1826183: Generate trust bundle for builds #158

Merged
merged 1 commit into from Jul 6, 2020
Merged

Bug 1826183: Generate trust bundle for builds #158

merged 1 commit into from Jul 6, 2020

Conversation

adambkaplan
Copy link
Contributor

@adambkaplan adambkaplan commented Jun 18, 2020 •
edited

  • Create entrypoint script which does the following:
    • Check if the CA trust bundle exists in the new neutral location.
    • Run update-ca-trust extract if a custom PKI is present.
  • Add mounts.conf to /etc/containers
    • Mount /run/secrets from build pod containers to buildah's containers.
      In RHEL/Fedora, /run/secrets is mounted in from /usr/share/rhel/secrets, and contains host
      information needed to access subscription content.
    • Mount /etc/pki from the build pod to buildah's containers. This contains the TLS trust store
      that the entrypoint script configures via update-ca-trust extract.
  • Organize image content to simplify the Dockerfile instructions for building openshift/builder.

@openshift-ci-robot
Copy link
Contributor

@adambkaplan: This pull request references Bugzilla bug 1826183, 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
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826183: Generate trust bundle for builds

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 openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jun 18, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020
@openshift-ci-robot
Copy link
Contributor

@adambkaplan: This pull request references Bugzilla bug 1826183, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826183: Generate trust bundle for builds

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.

This was referenced Jun 18, 2020
Merged
Closed
@gabemontero
Copy link
Contributor

/lgtm

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

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@gabemontero
Copy link
Contributor

/hold

the client plugin test is broke again via the upstream pipeline example in that repo @adambkaplan @akram @waveywaves

I'm talking the file https://github.com/openshift/jenkins-client-plugin/blob/master/examples/jenkins-image-sample.groovy

I saw the same thing in recent samples repo PRs

I'll create a PR shortly that will move the build suite off of its use and instead onto a more stable pipeline in openshift/origin so we still have regression coverage

I am also working on PR for jenkins-client-plugin that will finally allow e2e's in that repo to test against version of that test in that repo (and finally avoid the manual missteps that can occur testing that client plugin changes against the master branch vs. the version of the file in a given PR).

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2020
@adambkaplan
Copy link
Contributor Author

/hold

Placing a second hold on this based on the demo. The current chain of PRs will merge the trust bundle with the OS defaults. I need to check with the network team if this is acceptable.

/cc @danehans

@danehans
Copy link

The current chain of PRs will merge the trust bundle with the OS defaults.

@adambkaplan this is what the Network Operator does when an operator or operand requests proxy trust bundle injection, for example. If you're not requesting trust bundle injection, then yes, you need to combine the OS default bundle with the bundle referenced by proxy.spec.trustedCA, mount the combined bundle and run update-ca-trust extract.

@adambkaplan
Copy link
Contributor Author

@danehans then it isn't clear if we are doing the right thing.

The openshift controller manager operator creates a ConfigMap for the injected CA, which is then read in by the build controller. When a build is started, the build controller creates a new ConfigMap with this CA data, and mounts the contents into the build pod. This was originally mounted into /etc/pki/ca-trust/extracted. openshift/openshift-controller-manager#119 changes the mount point, forcing us to copy the contents and run update-ca-trust extract.

Trust bundle injection is not sufficient because not all processes can use the TLS trust bundle. Java processes need the trust bundle in the keytool format.

@danehans
Copy link

Plumbing the trust bundle configmap is up to the operator/operand implementation. For example, ingress operator mounts the trust bundle configmap to /etc/pki/ca-trust/extracted/pem. A file watcher was implemented that restarts the operator if a change is observed to the trust bundle file. Since ingress controllers (i.e. operand) does not need proxy support, the trusted ca bundle configmap does not need to get plumbed into any controllers.

@adambkaplan
Bug 1826183: Generate trust bundle for builds
32f5b57 
* Create entrypoint script which does the following:
** Check if the CA trust bundle exists in the new neutral location.
** Run `update-ca-trust extract` if a custom PKI is present.
* Add mounts.conf to /etc/containers
** Mount /run/secrets from build pod containers to buildah's containers.
   In RHEL/Fedora, /run/secrets is mounted in from /usr/share/rhel/secrets, and contains host
   information needed to access subscription content.
** Mount /etc/pki/ca-trust from the build pod to buildah's containers. This contains the TLS trust store
   that the entrypoint script configures via `update-ca-trust extract`.
* Organize image content to simplify the Dockerfile instructions for building `openshift/builder`.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020
@adambkaplan
Copy link
Contributor Author

/hold cancel

This PR can merge because it will run update-ca-trust extract with whatever trust bundle is provided.
Per @danehans comment, what we need to do is change the ConfigMap that the openshift controller manager receives from its operator. Instead of injecting the cluster CA, we should take the user CA directly from the openshift-config namespace and wire it through to build pods.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2020
@adambkaplan
Copy link
Contributor Author

@gabemontero @coreydaley @otaviof ptal - I had to change the mount point for /etc/pki/ca-trust

@coreydaley
Copy link
Member

Is there any testing that goes along with this or is the existing case covered and will automatically pass if this change is done correctly?

@adambkaplan
Copy link
Contributor Author

@coreydaley openshift/origin#25156 will verify once everything merges.

@coreydaley
Copy link
Member

/lgtm

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

[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

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-merge-robot openshift-merge-robot merged commit 00da867 into openshift:master Jul 6, 2020
@openshift-ci-robot
Copy link
Contributor

@adambkaplan: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:

In response to this:

Bug 1826183: Generate trust bundle for builds

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.

@adambkaplan adambkaplan mentioned this pull request Jul 15, 2020
Merged
@gabemontero gabemontero mentioned this pull request Jan 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coreydaley coreydaley Awaiting requested review from coreydaley

@gabemontero gabemontero Awaiting requested review from gabemontero

@danehans danehans Awaiting requested review from danehans

Assignees

@gabemontero gabemontero

@coreydaley coreydaley

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@adambkaplan @openshift-ci-robot @gabemontero @openshift-bot @danehans @coreydaley @openshift-merge-robot