Add http transport for cincinnati to enable proxy #219

jcpowermac · 2019-07-05T16:28:29Z

Modified Cincinnati client to accept the http transport struct
so that a https proxy can be set
Created new function getHTTPTransportProxy that
retrieves the proxy config, creates the transport and returns the
transport ptr.

jcpowermac · 2019-07-05T17:19:24Z

https://github.com/openshift/cluster-version-operator/blob/master/vendor/github.com/openshift/api/config/v1/types_proxy.go
Does not have ProxyStatus. Either update or switch to using proxy.Spec instead of proxy.Status.

# github.com/openshift/cluster-version-operator/pkg/cvo pkg/cvo/availableupdates.go:204:10: proxy.Status undefined...enshift/api/config/v1".Proxy has no field or method Status)

jcpowermac · 2019-07-05T17:28:27Z

@abhinavdahiya when you have a moment to take a look for a initial review, thanks!

abhinavdahiya · 2019-07-05T21:07:20Z

pkg/cvo/availableupdates.go

+
+func getHTTPTransportProxy() (*http.Transport, error) {
+	transport := http.Transport{}
+	config, err := rest.InClusterConfig()


You cannot create a new config here, please reuse the one in the operator struct already.

Or add proxy type client /listers to the operator struct and use here...

That would help in integration tests

abhinavdahiya · 2019-07-05T21:09:23Z

pkg/cincinnati/cincinnati.go

 // NewClient creates a new Cincinnati client with the given client identifier.
-func NewClient(id uuid.UUID) Client {
-	return Client{id: id}
+func NewClient(id uuid.UUID, transport *http.Transport) Client {


This is pretty broad additon..
I think a restricted proxy information and creating a transport with that is more ideal.

abhinavdahiya · 2019-07-05T21:10:16Z

pkg/cincinnati/cincinnati_test.go

 	"github.com/google/uuid"
+	_ "k8s.io/klog" // integration tests set glog flags.
 )



Would also like to see proxy tests here or in integration tests

jcpowermac · 2019-07-06T15:30:18Z

@abhinavdahiya updated PTAL. I will work on tests once everything else looks good. Thanks!

abhinavdahiya · 2019-07-09T20:08:31Z

pkg/cincinnati/cincinnati.go

 // NewClient creates a new Cincinnati client with the given client identifier.
-func NewClient(id uuid.UUID) Client {
-	return Client{id: id}
+func NewClient(id uuid.UUID, url *url.URL) Client {


what's the url, comments will be useful

abhinavdahiya · 2019-07-09T20:08:59Z

pkg/cvo/availableupdates.go

+	"k8s.io/klog"
+
+	"k8s.io/apimachinery/pkg/api/errors"



abhinavdahiya · 2019-07-09T20:12:59Z

pkg/cvo/cvo.go

+	proxyInformer.Informer().AddEventHandler(optr.eventHandler())
+
+	optr.proxyLister = proxyInformer.Lister()
+	optr.cacheSynced = append(optr.cacheSynced, proxyInformer.Informer().HasSynced)


4.2 the type exists for proxy.
but 4.1 the type doesn't exit for proxy.

So when user goes from 4.1.z to 4.2.z ; the CVO updates itself to a version that expects proxy type to exist, but the proxy type will be created by the cluster-config-operator. So this cache will never sync and CVO will be stuck.

So it's important our changes keep in mind that the type might not exist.

abhinavdahiya · 2019-07-09T20:13:26Z

pkg/cvo/availableupdates.go

+func (optr *Operator) getHTTPSProxyURL() (*url.URL, error) {
+	proxy, err := optr.proxyLister.Get("cluster")
+
+	if errors.IsNotFound(err) {


see https://github.com/openshift/cluster-version-operator/pull/219/files#r301772882

we should do a no-op when the type doesn't exist.

abhinavdahiya · 2019-07-09T20:14:40Z

pkg/cincinnati/cincinnati.go

 type Client struct {
-	id uuid.UUID
+	id  uuid.UUID
+	url *url.URL


hmm, how is url separate from upstream in GetUpdates

jcpowermac · 2019-07-15T15:56:23Z

/retest

abhinavdahiya · 2019-07-15T15:59:13Z

pkg/cvo/cvo.go

+	}
+
+	discover := kubeClient.Discovery()
+	apiResourcesList, err := discover.ServerResourcesForGroupVersion("config.openshift.io/v1")


The discovery is not useful.. this will always be false for CVO start...

the proxy CRD is created by an top-level operator cluster-config-operator... that is created by cluster-version-operator.

If that is the case how would we check if Proxy kind exists? It seems like any check would fail based on the above description.

when you go to GET proxy object and you get not found, you don't use the proxy codepath..

if the proxy api exists and the resource exists, you will be able to GET successfully and then we should start using the Proxy codepath.

abhinavdahiya · 2019-07-15T17:35:16Z

pkg/cvo/availableupdates.go

+
+	if errors.IsNotFound(err) {
+		return nil, nil
+	} else if err != nil {


why do you need the else if ?

I was thinking we would want to know what the error was if it was anything except IsNotFound

errros.IsNotFound only returns true when the err is not-found..
ie if err is nil, it will not be true.. and if err is not-nil and but not not-found, it will not be true..

so

if errors.IsNotFound(err) { return nil, nil } if err != nil { return nil, err }

cover's all the cases...

abhinavdahiya · 2019-07-15T17:35:55Z

pkg/cvo/cvo.go

+		proxyExists:           false,
+	}
+
+	_, err := client.ConfigV1().Proxies().Get("cluster", metav1.GetOptions{})


there's no requirement for this conditional setup of the informer.

abhinavdahiya · 2019-07-15T17:37:05Z

pkg/cvo/cvo.go

+	if err == nil {
+		optr.proxyLister = proxyInformer.Lister()
+		proxyInformer.Informer().AddEventHandler(optr.eventHandler())
+		optr.cacheSynced = append(optr.cacheSynced, proxyInformer.Informer().HasSynced)


just drop the proxy from cachedSynced... it's goal is to make sure we don't start action on inconsistent cache... but there's no such requirement for proxy informer.

abhinavdahiya · 2019-07-15T17:37:14Z

pkg/cvo/availableupdates.go

 }

-func calculateAvailableUpdatesStatus(clusterID, upstream, channel, version string) ([]configv1.Update, configv1.ClusterOperatorStatusCondition) {
+func calculateAvailableUpdatesStatus(clusterID, upstream, channel, version string, proxyURL *url.URL) ([]configv1.Update, configv1.ClusterOperatorStatusCondition) {


proxyURL should be next to clusterID.

- Modified Cincinnati client to accept a url.URL so that a https proxy can be set - Created new function getHTTPSProxyURL() that retrieves the proxy config, creates the url.URL and returns the url ptr. - Modified tests to include proxy lister - Added ProxyLister to Operator struct, modified New() - Modified start.go for ProxyLister operator struct change

abhinavdahiya · 2019-07-18T18:23:10Z

/lgtm

/retest

openshift-ci-robot · 2019-07-18T18:23:18Z

[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

Details

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

openshift-bot · 2019-07-19T00:38:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jcpowermac · 2019-07-19T15:42:37Z

/test e2e-aws

Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.

Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been consuming httpsProxy, but not httpProxy or noProxy [1]. This commit fixes that. I'm handing off the logic that mixes those three together to pick the proxy URI to the httpproxy library, which is also what the Go standard library uses for this purpose. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1978749

Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been loading proxy config from the spec property. We should be loading from status instead, so we benefit from the network operator's validation. Risk is small, because unlike some other in-cluster components, the CVO is unlikely to break things if it is temporarily consuming a broken proxy configuration. This is similar to c9fab43 (pkg/cvo: Fetch proxy CA certs from openshift-config-managed/trusted-ca-bundle, 2020-01-31, openshift#311), where we moved our trusted CA source from the user-configured ConfigMap to the network-operator-validated ConfigMap.

Since 4.2's ea5e3bc (Add http transport for cincinnati to enable proxy, 2019-07-16, openshift#219), the CVO has been consuming httpsProxy, but not httpProxy or noProxy [1]. This commit fixes that. I'm handing off the logic that mixes those three together to pick the proxy URI to the httpproxy library, which is also what the Go standard library uses for this purpose. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1978749 Cherry-pick of: e43080d

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 5, 2019

openshift-ci-robot requested review from crawford and wking July 5, 2019 16:28

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 5, 2019

jcpowermac force-pushed the CORS-1077 branch from 9c25110 to 2c0c846 Compare July 5, 2019 18:22

jcpowermac mentioned this pull request Jul 5, 2019

Update openshift client-go and api #220

Merged

abhinavdahiya reviewed Jul 5, 2019

View reviewed changes

abhinavdahiya reviewed Jul 9, 2019

View reviewed changes

pkg/cvo/availableupdates.go

"k8s.io/klog"

"k8s.io/apimachinery/pkg/api/errors"

Copy link

Contributor

abhinavdahiya Jul 9, 2019

Choose a reason for hiding this comment

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

extra line

abhinavdahiya reviewed Jul 9, 2019

View reviewed changes

abhinavdahiya reviewed Jul 15, 2019

View reviewed changes

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

jcpowermac force-pushed the CORS-1077 branch from d7c63fc to ea5e3bc Compare July 16, 2019 12:31

jcpowermac changed the title ~~[WIP] Add http transport for cincinnati to enable proxy~~ Add http transport for cincinnati to enable proxy Jul 16, 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 16, 2019

openshift-ci-robot assigned abhinavdahiya Jul 18, 2019

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

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

openshift-merge-robot merged commit eedb815 into openshift:master Jul 19, 2019

abhinavdahiya mentioned this pull request Aug 1, 2019

HTTP_proxy not passed to pods openshift/installer#2142

Closed

wking mentioned this pull request Jul 2, 2021

Bug 1978774: pkg/cvo/egress: Load HTTPS proxy from Proxy status #621

Merged

wking mentioned this pull request Jul 3, 2021

Bug 1978749: pkg/cvo: Respect noProxy #622

Merged

palonsoro mentioned this pull request Jul 8, 2021

Bug 1980411: [release-4.8] pkg/cvo/egress: Load HTTPS proxy from Proxy status #627

Merged

Add http transport for cincinnati to enable proxy #219

Add http transport for cincinnati to enable proxy #219

Uh oh!

Conversation

jcpowermac commented Jul 5, 2019

Uh oh!

jcpowermac commented Jul 5, 2019

Uh oh!

jcpowermac commented Jul 5, 2019

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!

jcpowermac commented Jul 6, 2019

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!

jcpowermac commented Jul 15, 2019

Uh oh!

abhinavdahiya Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

abhinavdahiya commented Jul 18, 2019

Uh oh!

openshift-ci-robot commented Jul 18, 2019

Uh oh!

openshift-bot commented Jul 19, 2019

Uh oh!

jcpowermac commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

abhinavdahiya Jul 15, 2019 •

edited

Loading

abhinavdahiya Jul 15, 2019 •

edited

Loading