Add CA bundle injector #274

JacobTanenbaum · 2019-08-06T20:34:34Z

This adds the ability to inject a combined CA bundle into configmaps that are setup to want them

JIRA SDN-496

PTAL @danehans

pkg/controller/configmapcainjector/configmapcainjector_controller.go

pkg/names/names.go

pkg/controller/configmapcainjector/configmapcainjector_controller.go

danehans · 2019-08-07T18:04:51Z

pkg/controller/configmapcainjector/configmapcainjector_controller.go

+func (r *ReconcileConfigMapInjector) Reconcile(request reconcile.Request) (reconcile.Result, error) {
+	log.Printf("Reconciling update for ca-certs from %s/%s\n", request.Name, request.Namespace)
+
+	combinedCAbundleConfigMap := &corev1.ConfigMap{}


I would like to refrain from referencing the trusted ca bundle by different names such as combined, merged, etc. to avoid confusion. I would like to standardize with trustedCABundle + whatever representing the trusted ca bundle (i.e. combined user/system bundle) and additionalTrustBundle + whatever representing the user-provided bundle. trustBundle can be used to represent a generic trust bundle, not additionalTrustBundle or trustedCABundle specific. I'll make sure my PR's follow the same naming convention.

pkg/controller/configmapcainjector/configmapcainjector_controller.go

danehans · 2019-08-07T18:30:41Z

pkg/names/names.go

+const TRUST_BUNDLE_CONFIGMAP_ANNOTATION = "config.openshift.io/inject-trusted-cabundle"
+
+// Proxy returns the namespaced name of the proxy
+// object named "cluster" in namespace "openshift-config-managed".


proxy "cluster' resided in the default ns, not the "openshift-config-managed" ns. cc @squeed

it doesn't reside in any namespace, it's a cluster scoped resource.

@JacobTanenbaum this is a doc bug that was fixed in https://github.com/openshift/cluster-network-operator/pull/245/files#diff-dc24d687a587f957b553ce29a8dfaff8R40-R46. Rebase your PR from latest master since PR has merged to fix this bug.

@bparees what is the difference? I thought default, cluster-scoped and non-namespaced are the same.

default is an actual namespace you can go look at on a cluster

oc get sa -n default NAME SECRETS AGE builder 2 124m default 2 130m deployer 2 124m

@bparees thanks for the clarification.

danehans · 2019-08-07T18:31:23Z

pkg/names/names.go

+
+// TrustBundleConfigMap returns the namespaced name of the ConfigMap
+// containing the merged user/system trust bundle.
+func TrustBundleConfigMap() types.NamespacedName {


s/TrustBundleConfigMap/TrustedCABundleConfigMap/

danehans · 2019-08-07T18:31:56Z

pkg/names/names.go

+
+// TRUST_BUNDLE_CONFIGMAP is the name of the ConfigMap
+// containing the combined user/system trust bundle.
+const TRUST_BUNDLE_CONFIGMAP = "trusted-ca-bundle"


s/TRUST_BUNDLE_CONFIGMAP/TRUSTED_CA_BUNDLE_CONFIGMAP/

I think we're likely to have multiple different trust bundles. Let's add proxy to this name so that we can distinguish. We can always provide combinations at some point, but we cannot resplit.

We ended up on this name because the thinking (as driven by @smarterclayton) was that ultimately we would want this to be the mechanism for providing the canonical "CAs to trust" to things on the cluster that wanted to be told what to trust. e.g. if an admin wanted to entirely replace the system trust bundle, this would eventually be that admin-provided bundle. (today it aggregates in system, but at some point we might make it optional to not do that).

So given that target use, i'm reluctant to put proxy back in the name of this thing. But i'm not sure how to reconcile that w/ your goal of "there might be many CA bundles".

I suppose if we ever reach the world I described above, we could introduce "trusted-ca-bundle" as a thing at that point in time, so I guess i'm ok w/ the proposed rename.

tldr: we started out including proxy in the name, i'm ok going back to it I guess.

But this probably also means you're not going to like the label name that we've socialized for people to put on their configmaps (that gets the CAs injected into the CM). So we better settle on that asap as we're causing downstream teams churn every time we change direction on it (we already moved them from annotations to labels).

@smarterclayton is going to weigh in on this, please don't rename anything yet.

danehans · 2019-08-07T18:32:08Z

pkg/names/names.go

+// TRUST_BUNDLE_CONFIGMAP_NS is the namespace that hosts the
+// ADDL_TRUST_BUNDLE_CONFIGMAP and TRUST_BUNDLE_CONFIGMAP
+// ConfigMaps.
+const TRUST_BUNDLE_CONFIGMAP_NS = "openshift-config-managed"


s/TRUST_BUNDLE_CONFIGMAP_NS/TRUSTED_CA_BUNDLE_CONFIGMAP_NS/

danehans · 2019-08-07T18:32:47Z

pkg/names/names.go

+
+// TRUST_BUNDLE_CONFIGMAP_ANNOTATION is the name of the annotation that
+// determines whether or not to inject the combined ca certificate
+const TRUST_BUNDLE_CONFIGMAP_ANNOTATION = "config.openshift.io/inject-trusted-cabundle"


s/TRUST_BUNDLE_CONFIGMAP_ANNOTATION/TRUSTED_CA_BUNDLE_CONFIGMAP_ANNOTATION/

I think @deads2k is going to suggest this name include proxy for the same reasons he mentioned in another PR (that we might have other bundles to inject in the future). but i'll let him comment.

and @smarterclayton since we went down this path somewhat at your behest, if you do not think we should be anticipating multiple bundles, would also like to hear from you.

@smarterclayton is going to weigh in on this, please don't rename anything yet.

pkg/controller/configmap_ca_injector/controller.go

ricardomaraschini · 2019-08-08T12:14:18Z

pkg/controller/configmap_ca_injector/controller.go

+
+func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager) reconcile.Reconciler {
+	if err := configv1.Install(mgr.GetScheme()); err != nil {
+		return &ReconcileConfigMapInjector{}


Shouldn't this be return nil ?

Maybe even "bubble-up" the error returned by Install()

squeed · 2019-08-08T12:22:22Z

So, a question - by making this reliant on an annotation, it means that we always need to watch all configmaps forever. This could easily result in scaling issues, since our client will have every configmap in cache. Can we make this a label?

While the operator-framework doesn't currently support label-selected watches, the underlying client machinery certainly does. That will help us down-the-line.

@enj as the maintainer of the service-ca-controller (which does a similar thing), what do you think?

pkg/controller/configmap_ca_injector/controller.go

squeed · 2019-08-13T18:38:40Z

I think this is ready to merge - with one question: is the validation cherry-picked from #271 still correct?

squeed · 2019-08-13T18:39:03Z

I'll approve this - @danehans, can you give it the final lgtm?
/approve

pkg/controller/configmap_ca_injector/controller.go

This adds the ability to inject a combined CA bundle into configmaps that are setup to want them

danehans · 2019-08-14T19:19:12Z

/lgtm

danehans · 2019-08-14T19:28:18Z

@JacobTanenbaum this PR makes an api call to get the trust bundle configmap for every configmap injection request. See if you can improve in a future PR by adding []byte to ReconcileConfigMapInjector, storing the trust bundle configmap data in the slice, updating the []byte whenever a change to the trust bundle configmap is observed and the configmap data and local []byte differ, using the []byte for injection requests when it's not empty or getting the trust bundle data and populating the []byte when it's empty. Thoughts @squeed?

bparees · 2019-08-14T20:52:26Z

/lgtm

openshift-ci-robot · 2019-08-14T20:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, danehans, JacobTanenbaum, squeed

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~~ [squeed]

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

bparees · 2019-08-14T20:52:41Z

/hold

bparees · 2019-08-14T20:52:59Z

holding in case squashing is in order or other housekeeping.

squeed · 2019-08-15T12:52:14Z

this PR makes an api call to get the trust bundle configmap for every configmap injection request.

That should be cached, so I'm not so concerned about that.

squeed · 2019-08-15T12:58:07Z

Looks fine to me, commit-wise
/hold cancel
woo

squeed · 2019-08-15T12:58:23Z

/woof

openshift-ci-robot · 2019-08-15T12:58:24Z

@squeed:

In response to this:

/woof

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.

akostadinov · 2019-08-19T11:02:02Z

Is there any documentation about how this should be used from user point of view?

…e} log inversion Typo from e4e10d7 (Adds a CA bundle injector, 2019-08-06, openshift#274).

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2019

openshift-ci-robot requested review from danwinship and squeed August 6, 2019 20:34

JacobTanenbaum changed the title ~~WIP Add CA bundle injector~~ [WIP] Add CA bundle injector Aug 6, 2019

ricardomaraschini reviewed Aug 7, 2019

View reviewed changes