Skip to content

Add Proxy to InstallConfig#1827

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
patrickdillon:install-config-proxy
Jun 13, 2019
Merged

Add Proxy to InstallConfig#1827
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
patrickdillon:install-config-proxy

Conversation

@patrickdillon
Copy link
Copy Markdown
Contributor

@patrickdillon patrickdillon commented Jun 6, 2019

As a step toward cluster proxies, add support for proxies to the InstallConfig. This PR creates the Proxy type struct and adds it to the InstallConfig. It also adds validation and unit tests for the validation.

Jira: CORS-1075
Related PRS: openshift/api#335, openshift/cluster-config-operator#66

This is a work in progress but I am looking for feedback:

  • I think this is close so I need to know what is missing
  • Confirm: httpProxy & httpsProxy must include scheme, i.e. start with http:// or https://
  • Do we need further validation?
  • Suggestions on how to validate NoProxy. Currently "bad-proxy" receives no error when passed to validate.DomainName

Just realized I also need to fill in the commit messages.

@openshift-ci-robot 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2019
abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/installconfig.go Outdated
abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/installconfig.go Outdated
abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/installconfig.go Outdated
abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/installconfig.go Outdated
abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/validation/installconfig.go Outdated
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 6, 2019

Choose a reason for hiding this comment

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

i would have assumed, atleast one of HTTPProxy and HTTPSProxy; user could have empty no-proxy list.

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 10, 2019

Choose a reason for hiding this comment

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

I am updating the conditional to:

 p.HTTPProxy == "" && p.HTTPSProxy == "" && p.NoProxy != "*"

I believe this is a valid edge case, which effectively disables the proxy.

Copy link
Copy Markdown
Contributor

@staebler staebler Jun 10, 2019

Choose a reason for hiding this comment

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

Setting HTTPProxy and HTTPSProxy to empty are sufficient to disable the proxy. Any valid value in NoProxy would be effectively ignored since there is no proxy to bypass. However, the way to disable the proxy is to exclude the Proxy field form the install config. If the Proxy field is present, then at least on of HTTPProxy and HTTPSProxy should be present. The user could still choose to disable the proxy by setting NoProxy to *, but even if this case at least one of HTTPProxy and HTTPSProxy should be set.

I don't think that the installer should support setting * for NoProxy at all. The necessity for supporting that in the API is that it is the way to disable the proxy, where a Proxy resource should exist. In the installer, however, the preferred way to disable the proxy should be to omit Proxy from the install config.

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 10, 2019

Choose a reason for hiding this comment

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

The user could still choose to disable the proxy by setting NoProxy to *, but even if this case at least one of HTTPProxy and HTTPSProxy should be set.

This makes sense.

I don't think that the installer should support setting * for NoProxy at all.

I am inclined to agree, but my reservation is that it was supported in 3.11 and I believe the situation was similar (proxy could just be omitted from the Ansible inventory). So it would be good to know if there was an intention behind the 3.11 design.

Copy link
Copy Markdown

@knobunc knobunc Jun 11, 2019

Choose a reason for hiding this comment

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

@sdodson do you know the intent ^

abhinavdahiya
abhinavdahiya reviewed Jun 6, 2019
View reviewed changes
Comment thread pkg/types/validation/installconfig_test.go Outdated
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 6, 2019

Choose a reason for hiding this comment

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

maybe add cases like:

  • noProxy invalid domain
  • noProxy invalid cidr
  • noProxy one of many invalid
  • noProxy two of many invalid

@jcpowermac jcpowermac mentioned this pull request Jun 7, 2019
Merged
@patrickdillon patrickdillon force-pushed the install-config-proxy branch from d65ac3a to b884346 Compare June 10, 2019 19:27
@openshift-ci-robot 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 Jun 10, 2019
@patrickdillon patrickdillon force-pushed the install-config-proxy branch 3 times, most recently from e020817 to 062e800 Compare June 10, 2019 19:34
@patrickdillon patrickdillon changed the title WIP: Add Proxy to InstallConfig Add Proxy to InstallConfig Jun 10, 2019
@openshift-ci-robot 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 Jun 10, 2019
@patrickdillon
Copy link
Copy Markdown
Contributor Author

patrickdillon commented Jun 10, 2019

I have pushed new commits which I believe address all outstanding comments/concerns. PTAL,

staebler
staebler reviewed Jun 10, 2019
View reviewed changes
Comment thread pkg/types/installconfig.go Outdated
Comment thread pkg/types/installconfig.go Outdated
Comment thread pkg/types/installconfig.go Outdated
Comment thread pkg/types/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
Comment thread pkg/types/validation/installconfig.go Outdated
abhinavdahiya
abhinavdahiya reviewed Jun 11, 2019
View reviewed changes
Comment thread pkg/types/validation/installconfig.go Outdated
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 11, 2019

Choose a reason for hiding this comment

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

maybe extend the URI to check the scheme of the URI...?

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 11, 2019

Choose a reason for hiding this comment

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

I created URIWithProtocol to do this. Let me know if that is not what you had in mind or suggestions.

@patrickdillon patrickdillon force-pushed the install-config-proxy branch 2 times, most recently from 58ce222 to af38961 Compare June 12, 2019 23:18
@patrickdillon
Copy link
Copy Markdown
Contributor Author

patrickdillon commented Jun 13, 2019

Related PRs:

abhinavdahiya
abhinavdahiya reviewed Jun 13, 2019
View reviewed changes
Comment thread pkg/validate/validate.go Outdated
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 13, 2019

Choose a reason for hiding this comment

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

you can parse to URL and check for scheme.... ? rather than string prefix.

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 13, 2019

Choose a reason for hiding this comment

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

Updated.

@patrickdillon patrickdillon force-pushed the install-config-proxy branch from af38961 to bd7b189 Compare June 13, 2019 02:11
@patrickdillon
Copy link
Copy Markdown
Contributor Author

patrickdillon commented Jun 13, 2019

/skip

abhinavdahiya
abhinavdahiya reviewed Jun 13, 2019
View reviewed changes
Comment thread pkg/validate/validate.go Outdated
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 13, 2019

Choose a reason for hiding this comment

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

nit: this else is not required..

from https://golang.org/doc/effective_go.html#if

In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted.

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 13, 2019

Choose a reason for hiding this comment

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

updated.

@abhinavdahiya
Copy link
Copy Markdown
Contributor

abhinavdahiya commented Jun 13, 2019

/approve

one small nit #1827 (comment)

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2019
@patrickdillon
install-config: add proxy support
08b7bc4 
Add Proxy struct from OpenShift API to Install Config so users can specify cluster-wide proxy configuration. Add basic validation to ensure that the proxy configuration being created is sane.

Jira: CORS-1075
@patrickdillon patrickdillon force-pushed the install-config-proxy branch from bd7b189 to 08b7bc4 Compare June 13, 2019 13:23
@abhinavdahiya
Copy link
Copy Markdown
Contributor

abhinavdahiya commented Jun 13, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jun 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jun 13, 2019

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

Test name Commit Details Rerun command
ci/prow/e2e-openstack 08b7bc4 link /test e2e-openstack
ci/prow/e2e-aws-scaleup-rhel7 08b7bc4 link /test e2e-aws-scaleup-rhel7

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.

@openshift-merge-robot openshift-merge-robot merged commit 7974552 into openshift:master Jun 13, 2019
@patrickdillon patrickdillon deleted the install-config-proxy branch October 24, 2019 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knobunc knobunc knobunc left review comments

@jstuever jstuever Awaiting requested review from jstuever

@mtnbikenc mtnbikenc Awaiting requested review from mtnbikenc

+2 more reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@staebler staebler staebler left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@abhinavdahiya abhinavdahiya

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@patrickdillon @abhinavdahiya @openshift-ci-robot @knobunc @staebler @openshift-merge-robot