Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg/asset/manifests/proxy: Create new proxy manifest #1866

Merged
merged 3 commits into from
Jun 21, 2019
Merged

pkg/asset/manifests/proxy: Create new proxy manifest #1866

merged 3 commits into from
Jun 21, 2019

Conversation

patrickdillon
Copy link
Contributor

Add proxy manifest to create config files and Proxy CR when users have a proxy for their cluster.

This PR uses a similar template to other manifests. There is some special logic for handling the NO_PROXY values. First, it generates values that need to be added to the NO_PROXY. Then there is a check to make sure if the user added those values, we will only include it once. I'm not sure if this check is necessary. (There is not currently a check to make sure the user's values are unique, but we could handle that in validation.)

JIRA: CORS-1074

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 18, 2019
@patrickdillon
Copy link
Contributor Author

/cc @abhinavdahiya

@abhinavdahiya
Copy link
Contributor

The asset currently is not being pushed to the cluster.. this assets needs to be wired like other config.openshift.io objects like

installer/pkg/asset/manifests/operators.go

Line 124 in a99bd59

m.FileList = append(m.FileList, infra.Files()...)

secondly another commit that updates the assets dep graph https://github.com/openshift/installer/blob/master/docs/design/assetgeneration.md#dependency-graph will be nice.

@abhinavdahiya abhinavdahiya mentioned this pull request Jun 19, 2019
Merged
@patrickdillon
Bump vendor dependencies for openshift/api/configv1/proxy.
e1c200c 
Update vendor dir to pull in the ProxyStatus which was added to proxies.config.openshift.io.
@openshift-ci-robot 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 Jun 20, 2019
@patrickdillon
Copy link
Contributor Author

secondly another commit that updates the assets dep graph https://github.com/openshift/installer/blob/master/docs/design/assetgeneration.md#dependency-graph will be nice.

I have updated the graph. manifests.Proxy shows up but I don't see an installConfig.Proxy. Should installConfig.Proxy be in the graph as well? My branch is up-to-date.

@patrickdillon
Copy link
Contributor Author

patrickdillon commented Jun 20, 2019 •
edited
Loading 

[padillon@localhost installer]$ cat assets/manifests/cluster-proxy-01-config.yaml 
apiVersion: config.openshift.io/v1
kind: Proxy
metadata:
  creationTimestamp: null
  name: cluster
spec:
  httpProxy: http://padillon:plaintextpassword@example.com
  httpsProxy: https://padillon:plaintextpassword@example.com
  noProxy: cluster.com
status:
  httpProxy: http://padillon:plaintextpassword@example.com
  httpsProxy: https://padillon:plaintextpassword@example.com
  noProxy: 10.128.0.0/14,127.0.0.1,172.30.0.0/16,cluster.com,etcd-0.padillon-dr.devcluster.openshift.com,etcd-1.padillon-dr.devcluster.openshift.com,etcd-2.padillon-dr.devcluster.openshift.com,https://api-int.padillon-dr.devcluster.openshift.com:6443,https://api.padillon-dr.devcluster.openshift.com:6443,localhost

EDIT: added sorting to status.noProxy values

@abhinavdahiya
Copy link
Contributor 

[padillon@localhost installer]$ cat assets/manifests/cluster-proxy-01-config.yaml 
apiVersion: config.openshift.io/v1
kind: Proxy
metadata:
  creationTimestamp: null
  name: cluster
spec:
  httpProxy: http://padillon:plaintextpassword@example.com
  httpsProxy: https://padillon:plaintextpassword@example.com
  noProxy: cluster.com
status:
  httpProxy: http://padillon:plaintextpassword@example.com
  httpsProxy: https://padillon:plaintextpassword@example.com
  noProxy: 10.128.0.0/14,127.0.0.1,172.30.0.0/16,cluster.com,etcd-0.padillon-dr.devcluster.openshift.com,etcd-1.padillon-dr.devcluster.openshift.com,etcd-2.padillon-dr.devcluster.openshift.com,https://api-int.padillon-dr.devcluster.openshift.com:6443,https://api.padillon-dr.devcluster.openshift.com:6443,localhost

EDIT: added sorting to status.noProxy values

@patrickdillon
few things pop-out:
https://api-int.padillon-dr.devcluster.openshift.com:6443 https://api.padillon-dr.devcluster.openshift.com:6443 are URI, when I think these should have been just the dns names right?

@patrickdillon
Update asset graph for Proxy manifest.
8e73b7e 
Update the design doc asset graph to include the addition of the proxy manifest.
@patrickdillon
Copy link
Contributor Author

https://api-int.padillon-dr.devcluster.openshift.com:6443 https://api.padillon-dr.devcluster.openshift.com:6443 are URI, when I think these should have been just the dns names right?

I removed the https protocol so it now reads:

noProxy: 10.128.0.0/14,127.0.0.1,172.30.0.0/16,api-int.padillon-dr.devcluster.openshift.com:6443,api.padillon-dr.devcluster.openshift.com:6443,cluster.com,etcd-0.padillon-dr.devcluster.openshift.com,etcd-1.padillon-dr.devcluster.openshift.com,etcd-2.padillon-dr.devcluster.openshift.com,localhost

I should probably remove the :6443 port postfix too, right?

Also did you see my question about installConfig.Proxy in the graph: #1866 (comment)?

@abhinavdahiya
Copy link
Contributor

secondly another commit that updates the assets dep graph https://github.com/openshift/installer/blob/master/docs/design/assetgeneration.md#dependency-graph will be nice.

I have updated the graph. manifests.Proxy shows up but I don't see an installConfig.Proxy. Should installConfig.Proxy be in the graph as well? My branch is up-to-date.

The dependency graph is better assets... Not parts of assets. So I think it's expected that manifest.proxy which is an asset has dependency on entire install config asset.

@abhinavdahiya
Copy link
Contributor

I should probably remove the :6443 port postfix too, right?

Yes.

I think you can net.ParseURL and then use Host field to add to NoProxy list...

@patrickdillon
Add proxy manifest.
ddb589f 
Create a proxy manifest asset for proxies.config.openshift.io. The Proxy spec is based on values read from the install config. This commit introduces logic to create the NoProxy status value by combining the user-provided values from the install config with default values for the cluster, such as the internal API server. These values are combined into a comma-separated string of unique values.

JIRA: CORS-1074 & CORS-1075
@patrickdillon
Copy link
Contributor Author

noProxy: 10.128.0.0/14,127.0.0.1,172.30.0.0/16,api-int.padillon-dr.devcluster.openshift.com,api.padillon-dr.devcluster.openshift.com,cluster.com,etcd-0.padillon-dr.devcluster.openshift.com,etcd-1.padillon-dr.devcluster.openshift.com,etcd-2.padillon-dr.devcluster.openshift.com,localhost

@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-openstack ddb589f link /test e2e-openstack
ci/prow/e2e-aws-scaleup-rhel7 ddb589f 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.

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.

@abhinavdahiya
Copy link
Contributor

/lgtm

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

[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

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
@openshift-merge-robot openshift-merge-robot merged commit 0502d44 into openshift:master Jun 21, 2019
@patrickdillon patrickdillon deleted the proxy-manifest 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

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@mtnbikenc mtnbikenc Awaiting requested review from mtnbikenc

@vrutkovs vrutkovs Awaiting requested review from vrutkovs

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@patrickdillon @abhinavdahiya @openshift-ci-robot @openshift-merge-robot