Add certificate option to cincinnati client #226

jcpowermac · 2019-07-25T18:57:58Z

Adds support to use a ConfigMap that contains a certificate bundle if a MITM proxy is used with cincinnati http client.

Modify cincinnati client struct to included tlsConfig
Modify cincinnati NewClient for tls change
Modify getHTTPSProxyURL to return ConfigMapNameReference
Add new method getTLSConfig that uses ConfigMap to extract the CA bundle and creates a tlsConfig

abhinavdahiya · 2019-07-25T19:12:29Z

pkg/cvo/availableupdates.go

+	if err != nil {
+		return err
+	}
+	tlsConfig, err := optr.getTLSConfig(cmNameRef)


the trustedCA for proxies is optional.. so we should keep that in mind.

abhinavdahiya · 2019-07-25T19:13:19Z

pkg/cvo/availableupdates.go

+	return nil, configv1.ConfigMapNameReference{}, nil
+}
+
+func (optr *Operator) getTLSConfig(cmNameRef configv1.ConfigMapNameReference) (*tls.Config, error) {


this function should assume that the reference is required.

abhinavdahiya · 2019-07-25T19:16:15Z

pkg/cvo/availableupdates.go

+}
+
+func (optr *Operator) getTLSConfig(cmNameRef configv1.ConfigMapNameReference) (*tls.Config, error) {
+	cm, err := optr.kubeClient.CoreV1().ConfigMaps("openshift-config-managed").Get(cmNameRef.Name, metav1.GetOptions{})


c147732

added an informer/lister for configmaps for openshif-config namespaces.. could we do somethings like that do that we are calling the api and using the cache if possible..

jcpowermac · 2019-07-29T14:53:27Z

I think we should fix the name of previous informer and create a new one based on namespace rather the name for proxy ca

@abhinavdahiya
Could we create a shared informer for both? Or do we want to limit the namespaces?

jcpowermac · 2019-07-29T16:12:11Z

I think we should fix the name of previous informer and create a new one based on namespace rather the name for proxy ca

@abhinavdahiya
Could we create a shared informer for both? Or do we want to limit the namespaces?

Nevermind.

abhinavdahiya · 2019-07-29T16:14:20Z

pkg/start/start.go

 		opts.FieldSelector = fmt.Sprintf("metadata.name=%s", internal.InstallerConfigMap)
 	})
+	cmManagedInformer := informers.NewFilteredSharedInformerFactory(kubeClient, resyncPeriod(o.ResyncInterval)(), internal.ConfigManagedNamespace, func(opts *metav1.ListOptions) {
+		opts.FieldSelector = fmt.Sprintf("metadata.name=%s", internal.InstallerConfigMap)


this is make it so that the informer will be restricted to only one object (named internal.InstallerConfigMap) in managed namespace. this will not cache the proxy-ca.

abhinavdahiya · 2019-07-29T16:15:26Z

pkg/start/start.go

 	sharedInformers := externalversions.NewSharedInformerFactory(client, resyncPeriod(o.ResyncInterval)())

 	ctx := &Context{
 		CVInformerFactory: cvInformer,
-		CMInformerFactory: cmInformer,
+		CMInformerFactory: cmConfigInformer,


if you don't add the informer here, then the informer cannot be started here

cluster-version-operator/pkg/start/start.go

Lines 366 to 368 in f443b93

c.CVInformerFactory.Start(ch)

c.CMInformerFactory.Start(ch)

c.InformerFactory.Start(ch)

jcpowermac · 2019-07-30T22:08:44Z

/retest

abhinavdahiya · 2019-07-30T22:11:31Z

pkg/cvo/availableupdates.go

+func (optr *Operator) getTLSConfig(cmNameRef string) (*tls.Config, error) {
+	cm, err := optr.cmManagedLister.Get(cmNameRef)
+
+	if errors.IsNotFound(err) {


can make this function fail on not found.... ?
what the effects of doing that..?

I see your point. Since we are checking if cmNameRef is not empty if the ConfigMap isn't found that is probably not a good thing.

The latest commit would return err:
https://github.com/openshift/cluster-version-operator/pull/226/files#diff-78f2af341fa49292dd6930f378018867R49

abhinavdahiya · 2019-07-30T22:14:05Z

pkg/start/start.go

 		opts.FieldSelector = fmt.Sprintf("metadata.name=%s", internal.InstallerConfigMap)
 	})
+	cmManagedInformer := informers.NewFilteredSharedInformerFactory(kubeClient, resyncPeriod(o.ResyncInterval)(), internal.ConfigManagedNamespace, func(opts *metav1.ListOptions) {
+		opts.FieldSelector = fmt.Sprintf("metadata.name=%s", internal.UserCAConfigMap)


I don't think we can force ourselves to only objects with name UserCAConfigMap.. I think we can create a list for maybe keep it for entire namespace to begin with.

ack...will fix this tomorrow.

abhinavdahiya · 2019-07-31T16:48:31Z

pkg/internal/constants.go

+	ConfigNamespace        = "openshift-config"
+	ConfigManagedNamespace = "openshift-config-managed"
+	InstallerConfigMap     = "openshift-install"
+	UserCAConfigMap        = "user-ca-bundle"


this is no longer use.

Adds support to use a ConfigMap that contains a certificate bundle if a MITM proxy is used with cincinnati http client. Modify cincinnati client struct to included tlsConfig Modify cincinnati NewClient for tls change Modify getHTTPSProxyURL to return string of ConfigMapNameReference Add new method getTLSConfig that uses ConfigMap to extract the CA bundle and creates a tlsConfig

abhinavdahiya · 2019-07-31T16:53:29Z

/lgtm

openshift-ci-robot · 2019-07-31T16:53:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jcpowermac

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

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

Vendor updates openshift/api

a22f63d

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 Jul 25, 2019

openshift-ci-robot requested review from smarterclayton and wking July 25, 2019 18:58

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2019

abhinavdahiya reviewed Jul 25, 2019

View reviewed changes

jcpowermac force-pushed the CORS-1162 branch from 6f5add3 to a1b282e Compare July 29, 2019 13:33

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2019

abhinavdahiya reviewed Jul 29, 2019

View reviewed changes

jcpowermac force-pushed the CORS-1162 branch from 2efeab8 to 547edf8 Compare July 29, 2019 16:47

jcpowermac changed the title ~~[WIP] Add certificate option to cincinnati client~~ Add certificate option to cincinnati client Jul 29, 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 Jul 29, 2019

abhinavdahiya reviewed Jul 30, 2019

View reviewed changes

abhinavdahiya reviewed Jul 31, 2019

View reviewed changes

jcpowermac force-pushed the CORS-1162 branch from ad3fcfe to 3875a44 Compare July 31, 2019 16:52

openshift-ci-robot assigned abhinavdahiya Jul 31, 2019

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019

openshift-merge-robot merged commit b57ee63 into openshift:master Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add certificate option to cincinnati client #226

Add certificate option to cincinnati client #226

jcpowermac commented Jul 25, 2019

abhinavdahiya Jul 25, 2019

abhinavdahiya Jul 25, 2019

abhinavdahiya Jul 25, 2019

jcpowermac commented Jul 29, 2019

jcpowermac commented Jul 29, 2019

abhinavdahiya Jul 29, 2019

jcpowermac Jul 29, 2019

abhinavdahiya Jul 29, 2019

jcpowermac commented Jul 30, 2019

abhinavdahiya Jul 30, 2019

jcpowermac Jul 30, 2019

abhinavdahiya Jul 30, 2019

jcpowermac Jul 30, 2019

abhinavdahiya Jul 31, 2019

abhinavdahiya commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

	c.CVInformerFactory.Start(ch)
	c.CMInformerFactory.Start(ch)
	c.InformerFactory.Start(ch)

Add certificate option to cincinnati client #226

Add certificate option to cincinnati client #226

Conversation

jcpowermac commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac commented Jul 29, 2019

jcpowermac commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac commented Jul 30, 2019

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

abhinavdahiya commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019