Bug 1826183: Sync proxy CA to openshift-controller-manager #162

adambkaplan · 2020-07-07T15:34:16Z

Previously, builds mounted the global CA trust bundle as the PEM-encoded TLS
trust bundle. This trust bundle is generated by the Network Operator, which
validates any user-provided CA trust bundle and merges it with the default trus
bundle provided by RHCOS. Using the global CA is problematic because maven and
other JVM-based processes establish trust using keytool formatted trust
bundles.

To generate trust bundles in the keytool format (as well as other formats used
by RHEL), builds will run update-ca-trust extract on startup. To merge the
trust bundle correctly, the build controller needs to mount any user-provided CA
in a well-known location. The openshift-builder process is then responsible for
copying the CA to /etc/ca-trust/source/anchors prior to running
update-ca-trust extract.

To achieve this, the operator is being enhanced with the following:

Added a separate controller for observing the user CA config.
Added a ResourceSync controller to copy the user CA to the
openshift-controller-manager namespace.
Updated starter to launch the new controllers.

The operator will continue to create the openshift-global-ca ConfigMap to
ensure that the openshift controller manager does not have an outage during
upgrades. This can be removed in a subsequent release.

See also openshift/builder#153, openshift/builder#158, openshift/openshift-controller-manager#119

openshift-ci-robot · 2020-07-07T15:34:21Z

@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)

Details

In response to this:

[WIP] Bug 1826183: Sync proxy CA to openshift-controller-manager

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.

gabemontero

I would like to see some additional comments and details around the "whys" of the needed change.

Otherwise, the code certainly uses library-go to the utmost. No additional comments.

gabemontero · 2020-07-13T20:07:51Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+
+	// Bug 1826183: The global CA bundle is not intended to be used with workloads which run `update-ca-trust extract` on their own.
+	// In 4.6 we need to create this ConfigMap with the trust bundle injected so that the controller manager does not have an outage during upgrade.
+	// In 4.7 and beyond this ConfigMap should be deleted.


So to connect some dots:

builds, either docker or s2i strategy, always call update-ca-trust extract now?

if yes, then some clarification here (vs. explanations I assume exist in openshift/builder or openshift/openshift-controller-manager) on how the global CAs are picked up, so things work with the proxy in addition to the direct contact to external repos like nexus, artifactory, etc. would be good

if no, then don't we still need global CA bundle for the "non update-ca-trust" scenarios

* builds, either docker or s2i strategy, _always_ call `update-ca-trust extract` now?

Yes - this was added in openshift/builder#158

* if yes, then some clarification here (vs. explanations I assume exist in openshift/builder or openshift/openshift-controller-manager) on how the global CAs are picked up, so things work with the proxy in addition to the direct contact to external repos like nexus, artifactory, etc. would be good

The new flow will be a follows:

ocm-o copies the user trusted CA to openshift-controller-manager/openshift-user-ca

For each build, openshift-controller-manager reads openshift-user-ca and copies that into a ConfigMap in the build's namespace, mounting it at /var/run/configs/openshift.io/pki/tls-ca-bundle.pem inside the build pod containers.

When builds run, it copies this CA to /etc/pki/ca-trust/source/anchors and runs update-ca-trust extract. This merges the custom CA with the default CAs provided by the builder image.

I will update with additional comments.

gabemontero · 2020-07-13T20:14:46Z

pkg/operator/usercaobservation/user_ca_observation_controller.go

+		Namespace: util.TargetNamespace,
+		Name:      "openshift-user-ca",
+	}
+	err = c.resourceSyncer.SyncConfigMap(destination, source)


so either user ca is a singleton like proxy ca, or SyncConfigMap is doing a merge of some sort

come clarification either way would be good

also, some details on why we need to copy ... i.e. the specfics around what breaks down with update-ca-trust if we read the "original" global CA vs. the user CA we've copied to would be good

How does the fact that we copy the proxy's trusted CA reconcile, get us out of jail, play nice, etc. with what I read in https://github.com/openshift/api/blob/master/config/v1/types_proxy.go#L44-L69

I updated the comment above to clarify what is going on overall, and why we need to copy the user CA to the openshift-controller-manager namespace. We get out of jail because build pods run update-ca-trust extract themselves.

My updated commit message indicates why we need to do this (namely, the issue with keytool formatted trust bundles).

Previously, builds mounted the global CA trust bundle as the PEM-encoded TLS trust bundle. This trust bundle is generated by the Network Operator, which validates any user-provided CA trust bundle and merges it with the default trus bundle provided by RHCOS. Using the global CA is problematic because maven and other JVM-based processes establish trust using `keytool` formatted trust bundles. To generate trust bundles in the `keytool` format (as well as other formats used by RHEL), builds will run `update-ca-trust extract` on startup. To merge the trust bundle correctly, the build controller needs to mount any user-provided CA in a well-known location. The openshift-builder process is then responsible for copying the CA to /etc/ca-trust/source/anchors prior to running `update-ca-trust extract`. To achieve this, the operator is being enhanced with the following: * Added a separate controller for observing the user CA config. * Added a ResourceSync controller to copy the user CA to the openshift-controller-manager namespace. * Updated starter to launch the new controllers. The operator will continue to create the `openshift-global-ca` ConfigMap to ensure that the openshift controller manager does not have an outage during upgrades. This can be removed in a subsequent release.

Add fake clientset from openshift/client-go used for unit testing.

adambkaplan

/cc @otaviof @coreydaley

I've added additional code comments per Gabe's feedback and tried to clarify the big picture in the commit message. PR description will be updated accordingly.

adambkaplan · 2020-07-16T12:43:29Z

pkg/operator/usercaobservation/user_ca_observation_controller.go

+		Namespace: util.TargetNamespace,
+		Name:      "openshift-user-ca",
+	}
+	err = c.resourceSyncer.SyncConfigMap(destination, source)


I updated the comment above to clarify what is going on overall, and why we need to copy the user CA to the openshift-controller-manager namespace. We get out of jail because build pods run update-ca-trust extract themselves.

My updated commit message indicates why we need to do this (namely, the issue with keytool formatted trust bundles).

openshift-ci-robot · 2020-07-16T12:49:11Z

@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)

Details

In response to this:

Bug 1826183: Sync proxy CA to openshift-controller-manager

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 · 2020-07-22T15:31:15Z

/assign @coreydaley

/retest

coreydaley · 2020-07-22T19:37:06Z

/lgtm

openshift-ci-robot · 2020-07-22T19:37:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, coreydaley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

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 requested review from gabemontero and mfojtik July 7, 2020 15:34

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2020

adambkaplan force-pushed the sync-user-ca branch from 1f56174 to 10f3dc6 Compare July 13, 2020 19:14

adambkaplan changed the title ~~[WIP] Bug 1826183: Sync proxy CA to openshift-controller-manager~~ Bug 1826183: Sync proxy CA to openshift-controller-manager Jul 13, 2020

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 Jul 13, 2020

gabemontero suggested changes Jul 13, 2020

View reviewed changes

adambkaplan mentioned this pull request Jul 15, 2020

Bug 1826183: Configure Build Pods to Merge CAs openshift/openshift-controller-manager#119

Merged

adambkaplan added 2 commits July 16, 2020 08:15

go mod vendor

e4b644f

Add fake clientset from openshift/client-go used for unit testing.

adambkaplan force-pushed the sync-user-ca branch from 10f3dc6 to e4b644f Compare July 16, 2020 12:37

adambkaplan commented Jul 16, 2020

View reviewed changes

openshift-ci-robot requested review from coreydaley and otaviof July 16, 2020 12:46

openshift-ci-robot assigned coreydaley Jul 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020

openshift-merge-robot merged commit e32d70d into openshift:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1826183: Sync proxy CA to openshift-controller-manager #162

Bug 1826183: Sync proxy CA to openshift-controller-manager #162

Uh oh!

adambkaplan commented Jul 7, 2020 •

edited

Loading

Uh oh!

openshift-ci-robot commented Jul 7, 2020

Uh oh!

gabemontero left a comment

Uh oh!

gabemontero Jul 13, 2020

Uh oh!

adambkaplan Jul 15, 2020

Uh oh!

gabemontero Jul 13, 2020

Uh oh!

adambkaplan Jul 16, 2020

Uh oh!

adambkaplan left a comment

Uh oh!

adambkaplan Jul 16, 2020

Uh oh!

openshift-ci-robot commented Jul 16, 2020

Uh oh!

adambkaplan commented Jul 22, 2020

Uh oh!

coreydaley commented Jul 22, 2020

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Bug 1826183: Sync proxy CA to openshift-controller-manager #162

Bug 1826183: Sync proxy CA to openshift-controller-manager #162

Uh oh!

Conversation

adambkaplan commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 7, 2020

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

gabemontero Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

adambkaplan Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

gabemontero Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

adambkaplan Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 16, 2020

Uh oh!

adambkaplan commented Jul 22, 2020

Uh oh!

coreydaley commented Jul 22, 2020

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adambkaplan commented Jul 7, 2020 •

edited

Loading