Skip to content

WIP - Mount trusted CA to the registry operand#349

Closed
adambkaplan wants to merge 4 commits intoopenshift:masterfrom
adambkaplan:watch-trusted-ca
Closed

WIP - Mount trusted CA to the registry operand#349
adambkaplan wants to merge 4 commits intoopenshift:masterfrom
adambkaplan:watch-trusted-ca

Conversation

@adambkaplan
Copy link
Contributor

@adambkaplan adambkaplan commented Aug 2, 2019

Building on top of #340 - mounting the cluster trusted CA to the "low-priority" trust source at /usr/share/pki/ca-trust-source/ within the registry.

The registry calls update-ca-trust extract to create the combined CA bundle, which includes the CA for the internal registry and other externally trusted registries.

* Injects the cluster proxy CA into /etc/pki/ca-trust/source/ca-bundle.crt
* Tell the CVO to only create the proxyca ConfigMap
Comment out volume mounts to unblock other work.
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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:

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 Aug 2, 2019
@adambkaplan
Copy link
Contributor Author

/assign @coreydaley

/cc @danehans @bparees

Copy link

@coreydaley coreydaley left a comment

Choose a reason for hiding this comment

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

a few small nits

configapiv1 "github.com/openshift/api/config/v1"
configlisters "github.com/openshift/client-go/config/listers/config/v1"
"github.com/openshift/cluster-image-registry-operator/pkg/apis/imageregistry/v1"
v1 "github.com/openshift/cluster-image-registry-operator/pkg/apis/imageregistry/v1"

Choose a reason for hiding this comment

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

this seems redundant, your ide probably did it autonomously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. vscode's Golang extension seems to do this if multiple packages have the same name.

// Registry runs update-ca-trust extract on startup, which merges the registry CAs with the cluster's trusted CAs
// into a single CA bundle.
vol = corev1.Volume{
Name: "trusted-ca",

Choose a reason for hiding this comment

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

nit: I think you could use params.TrustedCA.Name here also, but this might be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do once the ca controller lands

value: "cluster-image-registry-operator"
- name: IMAGE
value: docker.io/openshift/origin-docker-registry:latest
# volumeMounts:

Choose a reason for hiding this comment

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

I assume you are going to remove these and just have them commented out for testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@coreydaley Question on this one: the work being made on this PR is to mount the trusted-ca config map on the operand, not on the operator, right? If that is indeed the case then we gonna need these lines to be active in order to mount the config map also on operator during the deployment, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coreydaley yes once the CA controller lands

@ricardomaraschini this PR is built off of #340, which will mount trusted-ca to the operator. I did that because I also need to mount trusted-ca to the registry itself (operand)

@ricardomaraschini
Copy link
Contributor

Just as a reminder, I need this on to finish #342

@openshift-ci-robot 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 6, 2019
@openshift-ci-robot
Copy link
Contributor

@adambkaplan: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-image-registry f172b5c link /test e2e-aws-image-registry
ci/prow/e2e-aws f172b5c link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link
Contributor

@adambkaplan: PR needs rebase.

Details

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 the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2019
@adambkaplan
Copy link
Contributor Author

/close

Moved to #360

@openshift-ci-robot
Copy link
Contributor

@adambkaplan: Closed this PR.

Details

In response to this:

/close

Moved to #360

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.

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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants