Bug 2015493: [OCPCLOUD-1306] User CA bundle sync controller #136

lobziik · 2021-10-11T14:45:50Z

Separate controller for syncing user defined ca bundle (seed doc: https://docs.openshift.com/container-platform/4.8/networking/configuring-a-custom-pki.html#nw-proxy-configure-object_configuring-a-custom-pki) to CCM namespace for merge it with FCOS system bundle and usage in CCM as trusted CA.

Implementation logic mostly got from cluster-network-operator one.

Notes:

cloud-config-sync-controller binary was renamed to config-sync-controllers
In case if user-ca-bundle is invalid, only system one will be used.

lobziik · 2021-10-12T11:20:17Z

/cc @JoelSpeed @elmiko

JoelSpeed

Looks good! I've added some stylistic go idiom stuff, but otherwise nothing to add. Very nitty review, sorry 😅 Please swap all the Expect(err).To(Succeed() for Expect(err).NotTo(HaveOccurred())

cmd/config-sync-controllers/main.go

docs/dev/cloud-config-sync.md

pkg/controllers/fixtures/trust_bundle_invalid.pem

pkg/util/trustbundle.go

pkg/controllers/trusted_ca_bundle_controller_test.go

Fedosin · 2021-10-12T12:55:24Z

cmd/config-sync-controllers/main.go

 		Client:          mgr.GetClient(),
 		Scheme:          mgr.GetScheme(),
-		Recorder:        mgr.GetEventRecorderFor("cloud-controller-manager-operator-config-sync-controller"),
+		Recorder:        mgr.GetEventRecorderFor("cloud-controller-manager-operator-cloud-config-sync-controllers"),


"cloud-controller-manager-operator-config-sync-controllers"

it is another controller. https://github.com/openshift/cluster-cloud-controller-manager-operator/pull/136/files#diff-1b5385a70b9fc88e69ee8b33229144a2c42b3a4182d16c0045ba543d6d9af027R132 this one for ca sync. Should it be same recorder?

pkg/controllers/trusted_ca_bundle_controller.go

Fedosin · 2021-10-12T13:13:54Z

pkg/controllers/trusted_ca_bundle_controller.go

+	return systemTrustBundlePath
+}
+
+func (r *TrustedCABundleReconciler) getSystemTrustBundle() ([]byte, error) {


I just wonder if we can cache these bytes somewhere. I'm sure that the system trust bundle won't be changed, but decoding that every time is resource-intensive.
No necessary to do it right now, it's just an idea.

Fedosin · 2021-10-12T13:14:27Z

/retest

JoelSpeed · 2021-10-12T16:05:40Z

pkg/cloud/aws/assets/deployment.yaml

+        - name: trusted-ca
+          mountPath: /etc/pki/ca-trust/extracted/pem
+          readOnly: true


Does this need to be marked optional?

I don't think so, since system CA will be written to this config-map anyway.

pkg/controllers/trusted_ca_bundle_controller.go

pkg/util/trustbundle.go

docs/dev/trusted_ca_bundle_sync.md

lobziik · 2021-10-12T19:01:31Z

/retest

lobziik · 2021-10-13T09:13:24Z

/test e2e-azure-ccm

pkg/util/trustbundle.go

pkg/controllers/trusted_ca_bundle_controller.go

JoelSpeed · 2021-10-13T12:24:09Z

/approve

In order to add more similar controllers within this binary, cloud-config-sync-controller binary renamed to config-sync controller

Introduce separate controller for sync `user-ca-bundle` from `openshift-config` namespace to target CCCMO namespace for use user defined certificate authority during communication with cloud-provider

openshift-ci · 2021-10-14T11:16:17Z

@lobziik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ccm	`d1d1d30`	link	false	`/test e2e-gcp-ccm`
ci/prow/e2e-openstack-ccm	`d1d1d30`	link	false	`/test e2e-openstack-ccm`
ci/prow/e2e-gcp-ccm-install	`d1d1d30`	link	false	`/test e2e-gcp-ccm-install`
ci/prow/e2e-azure-upgrade	`d1d1d30`	link	false	`/test e2e-azure-upgrade`
ci/prow/e2e-azure-ccm-install	`d1d1d30`	link	false	`/test e2e-azure-ccm-install`
ci/prow/e2e-azure-ccm	`d1d1d30`	link	false	`/test e2e-azure-ccm`

Full PR test history. Your PR dashboard.

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.

lobziik · 2021-10-15T15:12:53Z

/retest

Fedosin

/lgtm

openshift-ci · 2021-10-15T15:14:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin, JoelSpeed

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~~ [Fedosin,JoelSpeed]

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

openshift-ci · 2021-10-19T11:00:19Z

@lobziik: All pull requests linked via external trackers have merged:

Bugzilla bug 2015493 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2015493: [OCPCLOUD-1306] User CA bundle sync controller

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.

lobziik · 2021-10-19T11:02:44Z

/bugzilla-refresh

lobziik · 2021-10-19T11:29:50Z

/cherry-pick release-4.9

openshift-cherrypick-robot · 2021-10-19T11:30:26Z

@lobziik: #136 failed to apply on top of branch "release-4.9":

Applying: Rename cloud-config-sync-controller binary to config-sync-controllers
Using index info to reconstruct a base tree...
M	Dockerfile
M	Makefile
M	cmd/cloud-config-sync-controller/main.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/config-sync-controllers/main.go
Auto-merging Makefile
Auto-merging Dockerfile
Applying: Introduce trusted CA sync controller
Using index info to reconstruct a base tree...
M	cmd/config-sync-controllers/main.go
M	pkg/controllers/clusteroperator_controller.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controllers/clusteroperator_controller.go
CONFLICT (content): Merge conflict in pkg/controllers/clusteroperator_controller.go
Auto-merging cmd/config-sync-controllers/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Introduce trusted CA sync controller
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.9

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 bot requested review from Fedosin and alexander-demicev October 11, 2021 14:46

openshift-ci bot requested review from JoelSpeed and elmiko October 12, 2021 11:20

JoelSpeed reviewed Oct 12, 2021

View reviewed changes

Fedosin reviewed Oct 12, 2021

View reviewed changes

JoelSpeed reviewed Oct 12, 2021

View reviewed changes

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 13, 2021

lobziik requested a review from JoelSpeed October 13, 2021 09:53

lobziik changed the title ~~User CA bundle sync controller~~ [OCPCLOUD-1306] User CA bundle sync controller Oct 13, 2021

JoelSpeed reviewed Oct 13, 2021

View reviewed changes

pkg/util/trustbundle.go Outdated Show resolved Hide resolved

pkg/util/trustbundle.go Show resolved Hide resolved

pkg/controllers/trusted_ca_bundle_controller.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2021

lobziik added 4 commits October 14, 2021 10:49

Rename cloud-config-sync-controller binary to config-sync-controllers

4ff5bea

In order to add more similar controllers within this binary, cloud-config-sync-controller binary renamed to config-sync controller

Introduce trusted CA sync controller

30aa4b8

Introduce separate controller for sync `user-ca-bundle` from `openshift-config` namespace to target CCCMO namespace for use user defined certificate authority during communication with cloud-provider

CA sync controller description doc

3e224e1

Add trusted ca mounts to all CCMs

d1d1d30

Fedosin approved these changes Oct 15, 2021

View reviewed changes

openshift-ci bot assigned Fedosin Oct 15, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2021

openshift-merge-robot merged commit 01b73ca into openshift:master Oct 15, 2021

lobziik changed the title ~~[OCPCLOUD-1306] User CA bundle sync controller~~ Bug 2015493: [OCPCLOUD-1306] User CA bundle sync controller Oct 19, 2021

lobziik mentioned this pull request Oct 19, 2021

[release-4.9] Bug 2015503: User CA bundle sync controller #138

Merged

patrickdillon mentioned this pull request Oct 19, 2021

Bug 2010921: Azure Stack: add trust bundle to cloud config openshift/installer#5248

Merged

Bug 2015493: [OCPCLOUD-1306] User CA bundle sync controller #136

Bug 2015493: [OCPCLOUD-1306] User CA bundle sync controller #136

Uh oh!

Conversation

lobziik commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lobziik commented Oct 12, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fedosin Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

lobziik Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fedosin Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Fedosin commented Oct 12, 2021

Uh oh!

JoelSpeed Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

lobziik Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lobziik commented Oct 12, 2021

Uh oh!

lobziik commented Oct 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoelSpeed commented Oct 13, 2021

Uh oh!

openshift-ci bot commented Oct 14, 2021

Uh oh!

lobziik commented Oct 15, 2021

Uh oh!

Fedosin left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 15, 2021

Uh oh!

openshift-ci bot commented Oct 19, 2021

Uh oh!

lobziik commented Oct 19, 2021

Uh oh!

lobziik commented Oct 19, 2021

Uh oh!

openshift-cherrypick-robot commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

lobziik commented Oct 11, 2021 •

edited

Loading