Manage Build Controller Additional Trusted CA #32

adambkaplan · 2018-11-08T22:55:13Z

Controller manager mounts additional CA bundle from ConfigMap
Operator watches build CRD and configmaps in openshift-config for changes
Operator validates additional CA data, merges into a single PEM block
Observed additional CA recorded in operator config spec, including sha1 hash
Primary sync loop merges additional CA data into a bundle mounted and read by the openshift-controller-manager

adambkaplan

adambkaplan · 2018-11-08T23:00:57Z

pkg/operator/observe_config.go

+func observeBuildAdditionalCAIsSet(listers Listers, observedConfig map[string]interface{}) (map[string]interface{}, error) {
+	// observed state of the build controller's Additional CA bundle is stored in a ConfigMap within the operator namespace
+	// this observes if there is data in this ConfigMap, and if the ca bundle exists
+	caMap, err := listers.configmapLister.ConfigMaps(operatorNamespaceName).Get("openshift-build-additional-ca")


Note - the "observed CA" ConfigMap will be added in a later PR.

Added to what?

(what is the "observed CA" configmap?)

adambkaplan · 2018-11-08T23:05:11Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

 }
+
+func manageBuildAdditionalCAConfigMap(client coreclientv1.ConfigMapsGetter) (*corev1.ConfigMap, bool, error) {
+	caMap := resourceread.ReadConfigMapV1OrDie(v311_00_assets.MustAsset("v3.11.0/openshift-controller-manager/build-additional-ca-cm.yaml"))


This syncs the observed CA bundle into the controller-manager's additional-ca configMap.

adambkaplan · 2018-11-08T23:07:37Z

/retest

bparees · 2018-11-08T23:13:46Z

pkg/operator/observe_config.go

 	kubeInformersForOpenshiftCoreOperators.Core().V1().Namespaces().Informer().AddEventHandler(c.eventHandler())
 	configInformer.Config().V1().Images().Informer().AddEventHandler(c.eventHandler())
+	// Watch for changes to configMaps within the core operator's namespace
+	kubeInformersForOpenshiftCoreOperators.Core().V1().ConfigMaps().Informer().AddEventHandler(c.eventHandler())


You need to get the buildCA configmap's namespace from the BuildConfig object's ConfigMapReference. You can't assume it's in the operator's namespace.

This also means that if the BuildConfig's ConfigMapReference changes, you have to detect that and, at a minimum, panic/exit the operator (it will then restart, read the new reference, and setup the watch for the new namespace/configmap to be watched).

You should also only be watching exactly the configmap that the ConfigMapReference points to (@deads2k says he can help point you to how to setup a single resource watch).

bparees · 2018-11-08T23:15:14Z

pkg/operator/observe_config.go

 }

+// observeBuildAdditionalCAPath observes the data in the controller's generated
+func observeBuildAdditionalCAIsSet(listers Listers, observedConfig map[string]interface{}) (map[string]interface{}, error) {


godoc + function name not aligned

bparees · 2018-11-08T23:19:21Z

pkg/operator/observe_config.go

+func observeBuildAdditionalCAIsSet(listers Listers, observedConfig map[string]interface{}) (map[string]interface{}, error) {
+	// observed state of the build controller's Additional CA bundle is stored in a ConfigMap within the operator namespace
+	// this observes if there is data in this ConfigMap, and if the ca bundle exists
+	caMap, err := listers.configmapLister.ConfigMaps(operatorNamespaceName).Get("openshift-build-additional-ca")


Added to what?

bparees · 2018-11-08T23:19:56Z

pkg/operator/observe_config.go

+func observeBuildAdditionalCAIsSet(listers Listers, observedConfig map[string]interface{}) (map[string]interface{}, error) {
+	// observed state of the build controller's Additional CA bundle is stored in a ConfigMap within the operator namespace
+	// this observes if there is data in this ConfigMap, and if the ca bundle exists
+	caMap, err := listers.configmapLister.ConfigMaps(operatorNamespaceName).Get("openshift-build-additional-ca")


(what is the "observed CA" configmap?)

bparees · 2018-11-08T23:28:33Z

Maybe i've misunderstood what you're doing here... is this PR starting from the assumption that the referenced ConfigMap has already been copied into the controller-manager's namespace? (note that that is not the same as the operator's namespace....)

Are we now going to have 3 copies of the configmap?

the configmap the user defined in some namespace and pointed to via ConfigMapReference
the copy of (1) that the operator puts in its own namespace to represented the observed/desired content to apply to the controller
the copy of (2) that the operator puts into the controller's namespace so the controller can actually mount/consume it?

It seems like (2) should not be needed.

bparees · 2018-11-08T23:36:00Z

(I also likely have an insufficient understanding of the existing operator behavior, especially around how it handles mounted resources)

adambkaplan · 2018-11-09T14:57:31Z

Are we now going to have 3 copies of the configmap?

the configmap the user defined in some namespace and pointed to via ConfigMapReference

the copy of (1) that the operator puts in its own namespace to represented the observed/desired content to apply to the controller

the copy of (2) that the operator puts into the controller's namespace so the controller can actually mount/consume it?

Yes, but (2) is not an exact copy. The operator will read the first user-provided configmap, merge the content (verifying proper encoding in the process), and then copy into the second configmap as the canonical "observed" CA. Hence why in this PR we are watching the configmaps in the core operators namespace.

This approach gives us two main advantages:

Admins can easily see the merged CA bundle that the operator wants to apply to the controller manager
Changes to the canonical observed CA trigger rollout.

However, I agree the additional configMap is a big minus and may not be necessary. Worth exploring storing hashes in the operator status.

adambkaplan · 2018-11-09T15:00:17Z

(I also say will because I plan to add the logic to watch the build CRD and associated ConfigMap in a separate PR)

bparees · 2018-11-09T18:26:03Z

This is hard to think through(for me) but i think if you get rid of the intermediary configmap, you save some work on reconciling that all the right pieces are in place.

I think the flow can be:

every time we observe cluster-configmap:

if hash(cluster-configmap)!=operator.status.hash
then
translate/copy cluster-configmap to controller-configmap(the thing the controller pod mounts)
set controller.status.hash=hash(cluster-configmap) (triggers deployment)
set operator.status.hash=hash(cluster-configmap) (indicates we are up to date)

By doing it in that order, we ensure that if we fail to update the controller deployment, next time we reconcile we'll go through all the steps again (because operator.status.hash still won't match the hash(cluster-configmap))

@deads2k am i missing something?

openshift-ci-robot · 2018-11-14T21:27:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adambkaplan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: bparees

If they are not already assigned, you can assign the PR to them by writing /assign @bparees in a comment when ready.

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

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

* Controller manager mounts additional CA bundle from ConfigMap * Operator watches build CRD and configmaps in openshift-config for changes * Operator validates additional CA data, merges into a single PEM block * Observed additional CA recorded in operator config spec, including sha1 hash * Primary sync loop merges additional CA data into a bundle mounted and read by the openshift-controller-manager

adambkaplan

In working condition. Note that this will need re-work if we are going to ditch the merged CA bundle and instead mount CAs by hostname.

/assign @bparees
/cc @deads2k

adambkaplan · 2018-11-28T14:24:38Z

pkg/apis/openshiftcontrollermanager/v1alpha1/types.go

 	ObservedConfig runtime.RawExtension `json:"observedConfig"`
+
+	// additionalTrustedCA references the additional trusted certificate authorities that operator should attempt to configure for the build controller.
+	AdditionalTrustedCA *AdditionalTrustedCA `json:"additionalTrustedCA,omitempty"`


This breaks the "no pointers" rule for API, but results in much cleaner code.
This will likely go away when certs are split out by hostname.

@deads2k do we need protobuf as well as json?

adambkaplan · 2018-11-28T14:27:13Z

pkg/apis/openshiftcontrollermanager/v1alpha1/types.go

+	SHA1Hash string `json:"sha1Hash,omitempty" protobuf:"bytes,1,opt,name=sha1Hash"`
+
+	// configMapName is the name of the ConfigMap in the openshift-config namespace containing the additional trusted CAs for the build controller.
+	ConfigMapName string `json:"configMap,omitempty" protobuf:"bytes,2,opt,name=configMap"`


CA data stores the SHA1Hash of the merged CA bundle, and the name of the ConfigMap.
Per discussion with @bparees and @deads2k, cluster admin provided configurations will live in the openshift-config namespace.

SHA probably belongs only in status, not spec, as it is not something the user gets to set.

I did this for consistency, though we can probably get away without it.

if we don't need it, let's get rid of it. just a source of confusion otherwise.

Now I remember...I do this so we can capture that the cluster admin updated the data in the referenced ConfigMap. I want to avoid races due to having an informer for openshift-config here and in the main operator controller.

adambkaplan · 2018-11-28T14:28:21Z

pkg/operator/observe_config.go

+			imageConfigLister:            configInformer.Config().V1().Images().Lister(),
+			coreOperatorsConfigMapLister: kubeInformersForOpenshiftCoreOperators.Core().V1().ConfigMaps().Lister(),
+			buildConfigLister:            configInformer.Config().V1().Builds().Lister(),
+			clusterConfigConfigMapLister: openshiftConfigInformer.Core().V1().ConfigMaps().Lister(),


Lister for ConfigMaps in the openshift-config namespace.

adambkaplan · 2018-11-28T14:29:28Z

pkg/operator/observe_config.go


 	c.operatorConfigSynced = operatorConfigInformer.Informer().HasSynced
-	c.configmapSynced = kubeInformersForOpenshiftCoreOperators.Core().V1().ConfigMaps().Informer().HasSynced
+	c.coreOperatorsConfigSynced = kubeInformersForOpenshiftCoreOperators.Core().V1().ConfigMaps().Informer().HasSynced


Do we still need to watch the openshift-core-operators namespace?

adambkaplan · 2018-11-28T14:31:37Z

pkg/operator/observe_config.go

+	if err != nil {
+		return err
+	}
+	currentCASpec := operatorConfig.Spec.AdditionalTrustedCA.DeepCopy()


Fetch operator config first, which can be mutated by the observer functions.
Deep copy the current CA spec so we can exit early if no updates are needed.

adambkaplan · 2018-11-28T14:42:44Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+		caMap.Data = nil
+		_, modified, err := resourceapply.ApplyConfigMap(client, caMap)
+		operatorConfig.Status.AdditionalTrustedCA = nil
+		return "", modified, err


If no build CRD, clear the data in the controller manager's ConfigMap and nil the status for AdditionalTrustedCA

adambkaplan · 2018-11-28T14:44:33Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+		caMap.Data = nil
+		_, modified, err := resourceapply.ApplyConfigMap(client, caMap)
+		operatorConfig.Status.AdditionalTrustedCA = nil
+		return "", modified, err


Handle case of empty data in referenced ConfigMap

adambkaplan · 2018-11-28T14:44:50Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+	if operatorConfig.Status.AdditionalTrustedCA != nil &&
+		operatorConfig.Status.AdditionalTrustedCA.SHA1Hash == caHash {
+		return caHash, false, nil
+	}


If hashes are equivalent, no updates needed.

adambkaplan · 2018-11-28T14:46:10Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+	}
+	caMap.Data["additional-ca.crt"] = string(caData)
+	_, modified, err := resourceapply.ApplyConfigMap(client, caMap)
+	return caHash, modified, err


If updates are happening, copy the merged data into the ConfigMap, which is then mounted into the build controller

adambkaplan · 2018-11-28T14:49:02Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

+			}
+			data = rest
+		}
+	}


mergePEMData ensures that the provided map contains valid PEM-encoded data for every key. This code may live on even if we are not merging CAs, as IMO the operator should validate that the provided certificates have valid encoding.

adambkaplan · 2018-11-28T14:50:48Z

/retest

adambkaplan · 2018-11-30T13:39:24Z

/hold

Will never merge, but leaving this up for teams with similar requirements.

openshift-bot · 2018-12-02T07:22:28Z

@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 · 2018-12-05T04:16:04Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`f2db4e5`	link	`/test e2e-aws`
ci/prow/rhel-images	`f2db4e5`	link	`/test rhel-images`

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.

adambkaplan · 2019-02-08T15:19:50Z

This functionality was added in openshift/origin#21614

…ft-4.7-ose-cluster-kube-storage-version-migrator-operator Updating ose-cluster-kube-storage-version-migrator-operator builder & base images to be consistent with ART

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2018

openshift-ci-robot requested review from bparees and soltysh November 8, 2018 22:56

adambkaplan force-pushed the observe-to-user-ca branch from 2c25dbf to cc8c976 Compare November 8, 2018 22:58

adambkaplan commented Nov 8, 2018

View reviewed changes

openshift-ci-robot assigned bparees Nov 8, 2018

bparees requested changes Nov 8, 2018

View reviewed changes

adambkaplan changed the title ~~Add build additional CA bundle to controller ds~~ [WIP] Add build additional CA bundle to controller ds Nov 14, 2018

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 Nov 14, 2018

adambkaplan force-pushed the observe-to-user-ca branch 2 times, most recently from 0edcb25 to 6ffa353 Compare November 27, 2018 22:41

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2018

adambkaplan force-pushed the observe-to-user-ca branch from 6ffa353 to f2db4e5 Compare November 27, 2018 22:48

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 27, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2018

adambkaplan changed the title ~~[WIP] Add build additional CA bundle to controller ds~~ Manage Build Controller Additional Trusted CA Nov 27, 2018

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 Nov 27, 2018

adambkaplan changed the title ~~Manage Build Controller Additional Trusted CA~~ [WIP] Manage Build Controller Additional Trusted CA Nov 27, 2018

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 Nov 27, 2018

adambkaplan changed the title ~~[WIP] Manage Build Controller Additional Trusted CA~~ Manage Build Controller Additional Trusted CA Nov 28, 2018

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 Nov 28, 2018

adambkaplan commented Nov 28, 2018

View reviewed changes

openshift-ci-robot requested a review from deads2k November 28, 2018 14:50

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

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2018

bparees removed their assignment Jan 8, 2019

adambkaplan closed this Feb 8, 2019

Manage Build Controller Additional Trusted CA #32

Manage Build Controller Additional Trusted CA #32

Uh oh!

Conversation

adambkaplan commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Nov 8, 2018

Uh oh!

bparees commented Nov 8, 2018

Uh oh!

adambkaplan commented Nov 9, 2018

Uh oh!

adambkaplan commented Nov 9, 2018

Uh oh!

bparees commented Nov 9, 2018

Uh oh!

openshift-ci-robot commented Nov 14, 2018

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Nov 28, 2018

Uh oh!

adambkaplan commented Nov 30, 2018

Uh oh!

openshift-bot commented Dec 2, 2018

Uh oh!

openshift-ci-robot commented Dec 5, 2018

adambkaplan commented Nov 8, 2018 •

edited

Loading