Adds Proxy Controller #245

danehans · 2019-07-16T23:31:41Z

Required by #271

Add controller to reconcile Proxy object.
Validate Proxy.spec fields.
Write http|https|no Proxy spec to status upon successful validation.

Note: Proxy CA reconciliation requirements are being addressed in #271.

Jira: SDN-492

PTAL @squeed @openshift/networking

danehans · 2019-07-17T16:49:17Z

/assign @squeed

danwinship · 2019-07-18T15:55:21Z

Should proxy reconciliation result be reflected in network operator status?

Yes, and adding a new StatusLevel as you did is correct.

squeed · 2019-07-23T12:12:30Z

pkg/controller/proxyconfig/proxyconfig_controller.go

+	}
+
+	// Watch for changes to primary resource config.openshift.io/v1/Proxy
+	err = c.Watch(&source.Kind{Type: &configv1.Proxy{}}, &handler.EnqueueRequestForObject{})


It probably makes sense here to watch for changes to configv1.Network here too, so we can get the Network.Status field.

danwinship · 2019-07-25T17:50:40Z

Given the possibility of us having to deal with other abstractly-proxy-related things in the future, I think the naming should be more specific than just "proxy" (eg, in pkg/proxy/). Maybe "cluster-proxy" or "operator-proxy"? Or one of those plus "egress". Or cluster-operator-egress-proxy...

And if this is likely to be maintained by the edge team going forward then the directories should get their own OWNERS files. (And we should start reorganizing pkg/network/ too since this would now make two files there that aren't owned by the SDN team.)

danehans · 2019-07-26T16:30:11Z

pkg/controller/proxyconfig/status.go

+	)
+	platform := infra.Status.PlatformStatus.Type
+
+	// TODO: Does a better way exist to get machineCIDR and controlplane replicas?


@squeed @danwinship PTAL and let me know if you have a better approach for parsing replicas and machineCIDR from the installer config or using some other resource to get the data.

cmd/cluster-network-operator/main.go

pkg/controller/proxyconfig/controller.go

danehans · 2019-07-29T17:56:13Z

Given the possibility of us having to deal with other abstractly-proxy-related things in the future, I think the naming should be more specific than just "proxy" (eg, in pkg/proxy/)

@danwinship good call. I'll update the naming based on one of your suggestions.

And if this is likely to be maintained by the edge team going forward then the directories should get their own OWNERS files. (And we should start reorganizing pkg/network/ too since this would now make two files there that aren't owned by the SDN team.)

@knobunc can you confirm who will own the proxy controller? @danwinship it's my understanding that I'm helping with the proxy controller implementation, but the network edge team is not responsible for maintaining.

pkg/controller/proxyconfig/status.go

danehans · 2019-08-06T18:58:21Z

/test e2e-aws

pkg/controller/proxyconfig/controller.go

pkg/controller/proxyconfig/validation.go

pkg/controller/proxyconfig/status.go

bparees

couple minor comments, otherwise lgtm.

pkg/controller/proxyconfig/controller.go

pkg/controller/proxyconfig/validation.go

danehans · 2019-08-08T04:09:32Z

@squeed @bparees I addressed booth of your comments in my latest commit, PTAL.

bparees · 2019-08-08T04:15:42Z

pkg/controller/proxyconfig/validation.go

+			return fmt.Errorf("invalid httpsProxy URI: %v", err)
+		}
+		if scheme != proxyHTTPScheme && scheme != proxyHTTPSScheme {
+			return fmt.Errorf("httpsProxy requires a %q or %s URI scheme", proxyHTTPScheme, proxyHTTPSScheme)


why is one of these %q and the other %s?

squeed · 2019-08-08T12:13:58Z

/approve
/lgtm

Please take a note of the follow-up issues.

openshift-ci-robot · 2019-08-08T12:14:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

Drop the wording from aa39bf3 (OSDOCS-640: Adding docs for configuring proxy during installation, 2019-09-12, openshift#16635), because the network operator explicitly supports both the 'http' and 'https' schemes since openshift/cluster-network-operator@42dbcf8955 (Refactors PR to focus on http/https/no proxy reconciliation, 2019-08-06, openshift/cluster-network-operator#245), which landed before release-4.2 split off from the network operator's master.

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

openshift-ci-robot requested review from danwinship and knobunc July 16, 2019 23:31

danehans mentioned this pull request Jul 16, 2019

Bumps openshift/api to consume config.proxy.readinessendpoints #247

Closed

openshift-ci-robot assigned squeed Jul 17, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2019

danehans force-pushed the proxy_reconciler branch from 86864f8 to 79643b8 Compare July 22, 2019 20:32

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2019

squeed reviewed Jul 23, 2019

View reviewed changes

danehans force-pushed the proxy_reconciler branch from 79643b8 to 1204de1 Compare July 23, 2019 15:59

danehans mentioned this pull request Jul 23, 2019

Bumps openshift api deps to support proxy trustedCA #261

Merged

danehans force-pushed the proxy_reconciler branch from 1204de1 to b1aab72 Compare July 24, 2019 02:11

danehans mentioned this pull request Jul 24, 2019

inject proxy env vars into requesting workload resources openshift/cluster-version-operator#224

Merged

danehans force-pushed the proxy_reconciler branch from b1aab72 to d961d0a Compare July 24, 2019 19:58

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

danehans commented Jul 26, 2019

View reviewed changes

squeed reviewed Jul 26, 2019

View reviewed changes

cmd/cluster-network-operator/main.go Outdated Show resolved Hide resolved

squeed reviewed Jul 26, 2019

View reviewed changes

pkg/controller/proxyconfig/controller.go Show resolved Hide resolved

danehans force-pushed the proxy_reconciler branch 3 times, most recently from b8ab96a to 2ea1c4c Compare July 26, 2019 17:45

danehans force-pushed the proxy_reconciler branch 3 times, most recently from 77f1476 to 150521e Compare July 30, 2019 21:10

danehans force-pushed the proxy_reconciler branch from 78b5407 to 6b189c8 Compare August 5, 2019 22:15

danehans mentioned this pull request Aug 5, 2019

Adds Trust Bundle Publishing to Proxy Controller #271

Merged

6 tasks

squeed reviewed Aug 6, 2019

View reviewed changes

pkg/controller/proxyconfig/status.go Outdated Show resolved Hide resolved

squeed reviewed Aug 6, 2019

View reviewed changes

pkg/controller/proxyconfig/status.go Outdated Show resolved Hide resolved

squeed reviewed Aug 6, 2019

View reviewed changes

pkg/controller/proxyconfig/status.go Show resolved Hide resolved

bparees reviewed Aug 6, 2019

View reviewed changes

pkg/controller/proxyconfig/controller.go Outdated Show resolved Hide resolved

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

danehans force-pushed the proxy_reconciler branch from 865a22e to efca243 Compare August 6, 2019 21:10

stlaz mentioned this pull request Aug 7, 2019

Update observe proxy to use Status NoProxy openshift/library-go#501

Merged

squeed reviewed Aug 7, 2019

View reviewed changes

pkg/controller/proxyconfig/validation.go Show resolved Hide resolved

squeed reviewed Aug 7, 2019

View reviewed changes

pkg/controller/proxyconfig/status.go Outdated Show resolved Hide resolved

danehans force-pushed the proxy_reconciler branch from efca243 to bae9d27 Compare August 7, 2019 17:20

danehans mentioned this pull request Aug 7, 2019

Adds readinessEndpoint validation for Proxy API type. #277

Merged

3 tasks

bparees requested changes Aug 7, 2019

View reviewed changes

pkg/controller/proxyconfig/controller.go Show resolved Hide resolved

pkg/controller/proxyconfig/validation.go Outdated Show resolved Hide resolved

danehans force-pushed the proxy_reconciler branch from bae9d27 to 1293580 Compare August 7, 2019 22:46

Refactors PR to focus on http/https/no proxy reconciliation

42dbcf8

danehans force-pushed the proxy_reconciler branch from 1293580 to 42dbcf8 Compare August 7, 2019 23:10

bparees reviewed Aug 8, 2019

View reviewed changes

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

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

openshift-merge-robot merged commit c7c18b7 into openshift:master Aug 8, 2019

This was referenced Aug 8, 2019

Fixes proxy validation error formatting #280

Merged

Add CA bundle injector #274

Merged

wking mentioned this pull request Oct 29, 2019

modules/*proxy*: httpsProxy supports https:// too openshift/openshift-docs#17716

Closed

Adds Proxy Controller #245

Adds Proxy Controller #245

Uh oh!

Conversation

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

Uh oh!

Uh oh!

danehans commented Jul 17, 2019

Uh oh!

danwinship commented Jul 18, 2019

Uh oh!

squeed Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

danwinship commented Jul 25, 2019

Uh oh!

danehans Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danehans commented Jul 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danehans commented Aug 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danehans commented Aug 8, 2019

Uh oh!

bparees Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

squeed commented Aug 8, 2019

Uh oh!

openshift-ci-robot commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

danehans commented Jul 16, 2019 •

edited

Loading