Inject payload's system store with proxy CA when specified #172

stlaz · 2019-08-09T14:11:46Z

enj · 2019-08-10T05:10:01Z

/hold

This looks weird to me. Please link me to the requirements for this.

stlaz · 2019-08-13T15:01:56Z

Adding WIP, this is needlessly complicated now, apparently the trusted-ca-bundle config map will always exist and should contain the CA bundle we need (if I get https://docs.google.com/document/d/1y0t0yEOSnKc4abxsjxEQjrFa1AP8iHcGyxlBpqGLO08/edit#heading=h.y6ieif41wmlc right), let's just use the sync logic we already have + getting the CM resourceVersion for deployment as added just lately and always mount the CM to the oauth-server pods

stlaz · 2019-08-13T15:24:00Z

This PR will now be failing until something starts creating the trusted-ca-bundle configmap in the openshift-config-managed namespace.

I suspect the main blocker is https://github.com/openshift/cluster-network-operator/pull/274, although the configmap creation may eventually be done elsewhere.

pkg/operator2/deployment.go

stlaz · 2019-08-14T10:20:02Z

Added the injection for the operator as well, similarly to openshift/cluster-image-registry-operator#340 but without the optional mount. If the CM is supposed to be present, then why not fail if it isn't.

enj

/hold

I do not think this will work.

manifests/03_configmap.yaml

pkg/operator2/starter.go

manifests/07_deployment.yaml

pkg/operator2/deployment.go

pkg/operator2/operator.go

danehans · 2019-08-16T21:07:34Z

@stlaz now that [1] and [2] have merged, operators/operands requiring proxy support should no longer be blocked from implementing this feature.

[1] openshift/cluster-network-operator#271
[2] openshift/cluster-network-operator#274

danehans · 2019-08-16T21:14:18Z

Added the injection for the operator as well, similarly to openshift/cluster-image-registry-operator#340 but without the optional mount. If the CM is supposed to be present, then why not fail if it isn't.

@stlaz @bparees has suggested to make the ca bundle mount optional. He doesn't want an operator to fail to schedule, causing a domino effect of failures. Operator's can successfully start without the CA trust bundle and then restart when the bundle is available.

stlaz · 2019-08-19T13:56:53Z

@danehans I made the CM optional, then. I don't think we'll need to restart the pod, we're dynamically reloading our transports for given CAs, I just added the reload for the system-default trust store, too.

enj

Mount the CA bundle at a standard path and have the operator use it where it needs to (route, OpenID, etc).

pkg/operator2/transport.go

pkg/operator2/deployment.go

manifests/07_deployment.yaml

enj

After these comments are addressed I am going to want to look at this more closely locally.

pkg/operator2/idp.go

pkg/operator2/route.go

pkg/operator2/operator.go

stlaz · 2019-08-21T09:05:14Z

/test e2e-aws

…st store This uses the CVO to create and watch a CM in openshift-authentication namespace, the name of the CM is prefixed in a way so that the config map's resourceVersion is watched by the authn operator and oauth-server is reloaded upon its change.

stlaz · 2019-08-21T10:07:57Z

/test e2e-aws-upgrade
aws problem

danehans · 2019-08-21T15:41:42Z

/lgtm

openshift-ci-robot · 2019-08-21T15:41:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans, stlaz

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:

~~OWNERS~~ [stlaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enj · 2019-08-21T21:00:23Z

Even with openshift/cluster-network-operator#296 in mind, I think this is fine.

/hold cancel

openshift-bot · 2019-08-21T23:39:15Z

/retest

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

sttts · 2019-08-28T17:14:58Z

manifests/03_configmap.yaml

+  annotations:
+    release.openshift.io/create-only: "true"
+  labels:
+    config.openshift.io/inject-trusted-cabundle: "true"


won't CVO stomp this configmap after network operator added the CA ?

same below of course

@danehans ^^

presumably not, because of the create-only annotation...

nvmd. The create-only label will make sure it is not overwritten.

sttts · 2019-08-28T17:19:07Z

pkg/operator2/idp.go

@@ -343,3 +346,12 @@ func encodeOrDie(obj runtime.Object) []byte {
 	}
 	return bytes
 }
+
+func trustedCABytes() []byte {


are these additional to the system trust store? The file only includes some CAs, not all. Especially not possibly required intermediate ones.

sttts · 2019-08-28T17:19:50Z

pkg/operator2/operator.go

+	trustedCABundleName      = systemConfigPrefix + "trusted-ca-bundle"
+	trustedCABundleKey       = "ca-bundle.crt"
+	trustedCABundleMountDir  = "/etc/pki/ca-trust/extracted/pem"
+	trustedCABundleMountFile = "tls-ca-bundle.pem"


a plain yaml text file would me much more readable than 35 constansts.

sttts · 2019-08-28T17:20:54Z

pkg/operator2/operator.go

@@ -525,7 +532,8 @@ func (c *authOperator) checkDeploymentReady(deployment *appsv1.Deployment, opera
 func (c *authOperator) checkRouteHealthy(route *routev1.Route, routerSecret *corev1.Secret, ingress *configv1.Ingress) (ready bool, msg, reason string, err error) {
 	caData := routerSecretToCA(route, routerSecret, ingress)

-	rt, err := transportFor("", caData, nil, nil)
+	// merge trustedCA data with router cert in case TLS intercept proxy is in place
+	rt, err := transportFor("", append(caData, trustedCABytes()...), nil, nil)


this reads the file on every request, not good.

Are we sure these bytes always append cleanly, i.e. that we have a trailing newline in caData? Looks fragile.

ic, we probably reread to get the latest version (which can change in the background).

deads2k · 2019-08-28T17:34:46Z

This appears to miss other usages. We should use the standard construction of proxies and simply get ourselves into the right shape. A fileobserver (not the watchdog, the other one) and the bash glue from OAS ought to get this into shape.

openshift-ci-robot requested review from enj and sttts August 9, 2019 14:11

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2019

stlaz changed the title ~~Inject payload's system store with proxy CA when specified~~ WIP: Inject payload's system store with proxy CA when specified Aug 13, 2019

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 Aug 13, 2019

stlaz force-pushed the mount_proxy_ca branch from 5852629 to 308e9a7 Compare August 13, 2019 15:22

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2019

stlaz commented Aug 13, 2019

View reviewed changes

pkg/operator2/deployment.go Outdated Show resolved Hide resolved

stlaz force-pushed the mount_proxy_ca branch from 308e9a7 to c53ab1d Compare August 14, 2019 09:51

stlaz changed the title ~~WIP: Inject payload's system store with proxy CA when specified~~ Inject payload's system store with proxy CA when specified Aug 14, 2019

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 Aug 14, 2019

enj suggested changes Aug 14, 2019

View reviewed changes

stlaz force-pushed the mount_proxy_ca branch 5 times, most recently from af21cb5 to 03ebe5b Compare August 19, 2019 13:55

enj suggested changes Aug 20, 2019

View reviewed changes

pkg/operator2/transport.go Outdated Show resolved Hide resolved

pkg/operator2/deployment.go Outdated Show resolved Hide resolved

manifests/07_deployment.yaml Outdated Show resolved Hide resolved

manifests/07_deployment.yaml Outdated Show resolved Hide resolved

stlaz force-pushed the mount_proxy_ca branch from 03ebe5b to ab06861 Compare August 20, 2019 11:39

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2019

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 20, 2019

stlaz force-pushed the mount_proxy_ca branch 2 times, most recently from c2b376d to 4b95ae8 Compare August 20, 2019 12:05

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2019

stlaz force-pushed the mount_proxy_ca branch 2 times, most recently from 86de23b to d37494f Compare August 20, 2019 14:38

enj reviewed Aug 20, 2019

View reviewed changes

pkg/operator2/idp.go Outdated Show resolved Hide resolved

pkg/operator2/route.go Outdated Show resolved Hide resolved

pkg/operator2/operator.go Outdated Show resolved Hide resolved

pkg/operator2/operator.go Outdated Show resolved Hide resolved

stlaz force-pushed the mount_proxy_ca branch from d37494f to 108b87c Compare August 21, 2019 07:29

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2019

stlaz added 2 commits August 21, 2019 11:12

Inject operator's trust store with trusted-ca-bundle

df55535

stlaz force-pushed the mount_proxy_ca branch from 108b87c to df55535 Compare August 21, 2019 09:12

openshift-ci-robot assigned danehans Aug 21, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2019

openshift-merge-robot merged commit 546aca0 into openshift:master Aug 22, 2019

sttts reviewed Aug 28, 2019

View reviewed changes

sttts mentioned this pull request Aug 29, 2019

Wire trusted-ca-bundle via /etc/pki #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inject payload's system store with proxy CA when specified #172

Inject payload's system store with proxy CA when specified #172

stlaz commented Aug 9, 2019

enj commented Aug 10, 2019

stlaz commented Aug 13, 2019 •

edited

Loading

stlaz commented Aug 13, 2019

stlaz commented Aug 14, 2019

enj left a comment

danehans commented Aug 16, 2019

danehans commented Aug 16, 2019

stlaz commented Aug 19, 2019

enj left a comment

enj left a comment

stlaz commented Aug 21, 2019

stlaz commented Aug 21, 2019

danehans commented Aug 21, 2019

openshift-ci-robot commented Aug 21, 2019

enj commented Aug 21, 2019

openshift-bot commented Aug 21, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

squeed Aug 29, 2019

sttts Aug 29, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

sttts Aug 28, 2019

deads2k commented Aug 28, 2019

Inject payload's system store with proxy CA when specified #172

Inject payload's system store with proxy CA when specified #172

Conversation

stlaz commented Aug 9, 2019

enj commented Aug 10, 2019

stlaz commented Aug 13, 2019 • edited Loading

stlaz commented Aug 13, 2019

stlaz commented Aug 14, 2019

enj left a comment

Choose a reason for hiding this comment

danehans commented Aug 16, 2019

danehans commented Aug 16, 2019

stlaz commented Aug 19, 2019

enj left a comment

Choose a reason for hiding this comment

enj left a comment

Choose a reason for hiding this comment

stlaz commented Aug 21, 2019

stlaz commented Aug 21, 2019

danehans commented Aug 21, 2019

openshift-ci-robot commented Aug 21, 2019

enj commented Aug 21, 2019

openshift-bot commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Aug 28, 2019

stlaz commented Aug 13, 2019 •

edited

Loading