DEVEXP-403: Mount cluster trusted CA to image registry operator #340

adambkaplan · 2019-07-30T20:41:39Z

Injects the cluster proxy CA into /etc/pki/ca-trust/source/ca-bundle.crt

manifests/04-ca-proxy.yaml

adambkaplan

adambkaplan · 2019-07-30T20:43:31Z

manifests/04-ca-proxy.yaml

+kind: ConfigMap
+metadata:
+  annotations:
+    config.openshift.io/inject-proxy-cabundle: "true"


I noticed in https://github.com/openshift/cluster-image-registry-operator/blob/master/manifests/04-ca-config.yaml#L5-L6 that we are not marking the other CA ConfigMap as "create only" via the annotation release.openshift.io/create-only: "true". Is this right?

It seems wrong to me, yet i don't see that CM getting stomped by the CVO. Perhaps the CVO does not stomp configmaps back to empty?

@deads2k @abhinavdahiya ? (tldr: we have a CM being created in the manifest for service CAs. It's not marked create-only, but it seems to be working ok, the service-ca gets injected and the value doesn't seem to be getting stomped back out).

(after looking at a cluster i think it is getting stomped and then re-injected. though i'm a little surprised that isn't causing rollouts of the registry operator so we should probably confirm the registry operator is actually doing the right thing when the service ca changes. In this case the cert doesn't actually change, but it's presumably flapping back and forth between not existing and existing, in the CM)

I was able to dig deeper on this - there are three separate control loops that are modifying the serviceca ConfigMap:

The CVO

The ca-configmap-injector

The image registry operator itself - https://github.com/openshift/cluster-image-registry-operator/blob/master/pkg/resource/serviceca.go#L52-L62

Will fix this in a separate PR

good catch on (3). seems like we should not need that generator at all?

for (1) we should just set the create-only annotation on the CM.

adambkaplan · 2019-07-30T20:44:37Z

manifests/07-operator.yaml

+      volumes:
+        - name: proxyca
+          configMap:
+            name: proxyca


Don't block mounting the specific ca-bundle.crt path.

bparees · 2019-07-30T20:47:12Z

manifests/07-operator.yaml

@@ -53,3 +53,10 @@ spec:
              value: "cluster-image-registry-operator"
            - name: IMAGE
              value: docker.io/openshift/origin-docker-registry:latest
+          volumeMounts:
+            - name: proxyca
+              mountPath: /etc/pki/ca-trust/source/


pretty sure the bundle you're trying to overwrite is this one:
/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem

but i guess i'm not positive which one golang is actually reading since there is also:
/usr/share/pki/ca-trust-legacy/ca-bundle.legacy.default.crt

but it should be one of those two (there are various symlinks that point to those 2 files)

I've called this out in the proxy design doc. Will the operator container need to run update-ca-trust extract before it begins?

i would not expect so, no. pretty sure update-ca-trust extract is what produces those bundles.

bparees · 2019-07-30T20:48:00Z

the other bit of this should be some logic in the operator itself to watch the mounted file. If it changes, the operator should die (so it gets restarted).

(The file on disk will change if the configmap content changes).

adambkaplan · 2019-07-31T02:05:14Z

the other bit of this should be some logic in the operator itself to watch the mounted file. If it changes, the operator should die (so it gets restarted).

@ricardomaraschini you and I will work on a follow-up PR to do this.

@bparees this PR will also stage the proxy CA that will be mounted into the registry itself - I assume we can use the same ConfigMap for both as long as the volume mounts are read-only.

bparees · 2019-07-31T03:40:04Z

@bparees this PR will also stage the proxy CA that will be mounted into the registry itself - I assume we can use the same ConfigMap for both as long as the volume mounts are read-only.

i'm not entirely sure what you mean, but the CM that's being annotated for proxy-ca injection needs to be used only for the proxy-ca, in the same way that CMs annotated for service-ca injection are used only for that. They can't have multiple writers.

adambkaplan · 2019-07-31T17:40:02Z

i'm not entirely sure what you mean, but the CM that's being annotated for proxy-ca injection needs to be used only for the proxy-ca,

My intent is that we only have one CM for the proxy ca (rename this trusted-ca?). This ConfigMap will be mounted into the registry operator as well as the registry pod - hence read-only mounting.

I assume we don't need to mount this into the nodeca daemonset, and that another operator will be responsible for this.

bparees · 2019-07-31T18:00:17Z

My intent is that we only have one CM for the proxy ca (rename this trusted-ca?). This ConfigMap will be mounted into the registry operator as well as the registry pod - hence read-only mounting.

yes mounting it in two different pods should be fine.

I assume we don't need to mount this into the nodeca daemonset, and that another operator will be responsible for this.

would be good to confirm with @mrunalp how they are handling it(not on this thread please) but i believe the intent is for the MCO to get the trusted CAs onto the nodes and CRIO should just pick it up from there.

mrunalp · 2019-08-01T17:38:30Z

would be good to confirm with @mrunalp how they are handling it(not on this thread please) but i believe the intent is for the MCO to get the trusted CAs onto the nodes and CRIO should just pick it up from there.
Yes, that's how it will work for CRI-O 👍

adambkaplan · 2019-08-01T17:40:16Z

Huzzah! No trust = no AWS storage (though we separately trust the apiserver?):

 {
            "apiVersion": "config.openshift.io/v1",
            "kind": "ClusterOperator",
            "metadata": {
                "creationTimestamp": "2019-08-01T15:58:23Z",
                "generation": 1,
                "name": "image-registry",
                "resourceVersion": "12484",
                "selfLink": "/apis/config.openshift.io/v1/clusteroperators/image-registry",
                "uid": "30c468a5-b475-11e9-a066-0afad4972984"
            },
            "spec": {},
            "status": {
                "conditions": [
                    {
                        "lastTransitionTime": "2019-08-01T15:58:23Z",
                        "message": "The deployment does not exist",
                        "reason": "DeploymentNotFound",
                        "status": "False",
                        "type": "Available"
                    },
                    {
                        "lastTransitionTime": "2019-08-01T15:58:23Z",
                        "message": "Unable to apply resources: unable to sync storage configuration: RequestError: send request failed\ncaused by: Head https://image-registry-us-east-1-ciop5nrpdzv2f0b2b5wzf6-30d741eeb47511.s3.amazonaws.com/: x509: certificate signed by unknown authority",
                        "reason": "Error",
                        "status": "True",
                        "type": "Progressing"
                    },
                    {
                        "lastTransitionTime": "2019-08-01T15:58:23Z",
                        "status": "False",
                        "type": "Degraded"
                    }
                ],
                "extension": null
            }
        },

danehans · 2019-08-02T00:21:49Z

Should s3.amazonaws.com be added to noProxy for aws installs?

adambkaplan · 2019-08-02T14:04:41Z

Should s3.amazonaws.com be added to noProxy for aws installs?

If we use VPC endpoints for s3, then yes. Otherwise no - iirc traffic to s3 by default routes over the public internet.

Is our proxy controller smart enough to detect this?

fyi @openshift/openshift-team-network-edge

adambkaplan · 2019-08-08T15:26:47Z

/hold

Blocked by openshift/cluster-network-operator#274

adambkaplan · 2019-08-08T15:29:37Z

manifests/07-operator.yaml

+            name: trusted-ca
+            optional: true
+            items:
+            - key: ca-bundle.crt


@bparees this uses the optional mount and overwrites the filename.

does "optional" apply to the keys as well as the configmap itself?

@bparees I can confirm it does. If the key is not present, the pod is still scheduled (and in my testing had lots of unknown CA errors).

* Injects the cluster proxy CA into /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem * Tell the CVO to only create the trusted-ca ConfigMap

adambkaplan · 2019-08-09T21:34:19Z

/hold cancel

Once openshift/cluster-network-operator#274 this should work!

danehans · 2019-08-16T02:50:16Z

@adambkaplan openshift/cluster-network-operator#271 is also needed, but should merge tomorrow.

danehans · 2019-08-16T18:35:11Z

/retest

danehans · 2019-08-16T18:36:26Z

@adambkaplan openshift/cluster-network-operator#271 has merged.

danehans · 2019-08-18T20:35:37Z

fail [k8s.io/kubernetes/test/e2e/framework/framework.go:338]: Aug 16 19:37:17.100: Couldn't delete ns: "e2e-test-build-webhooks-9brtm": namespace e2e-test-build-webhooks-9brtm was not deleted with limit: timed out waiting for the condition, namespace is empty but is not yet removed (&errors.errorString{s:"namespace e2e-test-build-webhooks-9brtm was not deleted with limit: timed out waiting for the condition, namespace is empty but is not yet removed"})

/test e2e-aws

adambkaplan · 2019-08-19T13:39:26Z

/test e2e-aws

/test e2e-aws-image-registry

/test e2e-aws-operator

/test e2e-aws-upgrade

danehans · 2019-08-19T16:33:52Z

level=error msg="Error: Error creating S3 bucket: TooManyBuckets: You have attempted to create more buckets than allowed"
/test e2e-aws
/test e2e-aws-image-registry
/test e2e-aws-upgrade

adambkaplan · 2019-08-19T19:16:06Z

/retest

bparees · 2019-08-19T20:22:40Z

/lgtm

openshift-bot · 2019-08-20T10:45:28Z

/retest

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

ricardomaraschini · 2019-08-20T11:54:34Z

/lgtm

openshift-ci-robot · 2019-08-20T11:54:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, bparees, ricardomaraschini

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~~ [adambkaplan,bparees]

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 dmage and gabemontero July 30, 2019 20:41

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 30, 2019

bparees reviewed Jul 30, 2019

View reviewed changes

manifests/04-ca-proxy.yaml Outdated Show resolved Hide resolved

adambkaplan commented Jul 30, 2019

View reviewed changes

openshift-ci-robot assigned bparees Jul 30, 2019

openshift-ci-robot requested review from coreydaley and ricardomaraschini July 30, 2019 20:46

bparees reviewed Jul 30, 2019

View reviewed changes

adambkaplan force-pushed the inject-proxy-ca branch from 08e9fa9 to 029e67b Compare July 30, 2019 21:27

adambkaplan changed the title ~~Mount cluster proxy CA to image registry operator~~ WIP - Mount cluster proxy CA to image registry operator Aug 1, 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 1, 2019

adambkaplan mentioned this pull request Aug 2, 2019

WIP - Mount trusted CA to the registry operand #349

Closed

adambkaplan force-pushed the inject-proxy-ca branch from 171bced to 44ea4a5 Compare August 8, 2019 15:24

adambkaplan changed the title ~~WIP - Mount cluster proxy CA to image registry operator~~ DEVEXP-402: Mount cluster trusted CA to image registry operator Aug 8, 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 8, 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 8, 2019

adambkaplan commented Aug 8, 2019

View reviewed changes

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

adambkaplan force-pushed the inject-proxy-ca branch from 44ea4a5 to de4fd43 Compare August 9, 2019 19:41

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

Mount cluster proxy CA to image registry operator

a505e00

* Injects the cluster proxy CA into /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem * Tell the CVO to only create the trusted-ca ConfigMap

adambkaplan force-pushed the inject-proxy-ca branch from de4fd43 to a505e00 Compare August 9, 2019 21:31

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 9, 2019

adambkaplan mentioned this pull request Aug 9, 2019

DEVEXP-402: Mount trusted-ca to registry base pki trust source #360

Merged

adambkaplan changed the title ~~DEVEXP-402: Mount cluster trusted CA to image registry operator~~ DEVEXP-403: Mount cluster trusted CA to image registry operator Aug 9, 2019

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

stlaz mentioned this pull request Aug 14, 2019

Inject payload's system store with proxy CA when specified openshift/cluster-authentication-operator#172

Merged

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

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

openshift-ci-robot assigned ricardomaraschini Aug 20, 2019

openshift-merge-robot merged commit 75e8e85 into openshift:master Aug 20, 2019

DEVEXP-403: Mount cluster trusted CA to image registry operator #340

DEVEXP-403: Mount cluster trusted CA to image registry operator #340

Conversation

adambkaplan commented Jul 30, 2019

adambkaplan left a 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

Choose a reason for hiding this comment

bparees commented Jul 30, 2019

adambkaplan commented Jul 31, 2019

bparees commented Jul 31, 2019 • edited Loading

adambkaplan commented Jul 31, 2019

bparees commented Jul 31, 2019

mrunalp commented Aug 1, 2019

adambkaplan commented Aug 1, 2019

danehans commented Aug 2, 2019

adambkaplan commented Aug 2, 2019

adambkaplan commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambkaplan commented Aug 9, 2019

danehans commented Aug 16, 2019

danehans commented Aug 16, 2019

danehans commented Aug 16, 2019

danehans commented Aug 18, 2019

adambkaplan commented Aug 19, 2019

danehans commented Aug 19, 2019

adambkaplan commented Aug 19, 2019

bparees commented Aug 19, 2019

openshift-bot commented Aug 20, 2019

ricardomaraschini commented Aug 20, 2019

openshift-ci-robot commented Aug 20, 2019

bparees commented Jul 31, 2019 •

edited

Loading