Skip to content

Conversation

@danehans
Copy link
Contributor

@danehans danehans commented Aug 8, 2019

Required by #277

Adds cvo proxy opt-in annotation to inject proxy env var's from openshift/cluster-version-operator#224.

/assign @squeed @bparees

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 8, 2019
@danehans
Copy link
Contributor Author

danehans commented Aug 8, 2019

@squeed I can use some guidance on how this should be tested. I was thinking an e2e, but I don't see any exist e2e for CNO.

@squeed
Copy link
Contributor

squeed commented Aug 8, 2019

@danehans yeah, we don't have plans for this kind of e2e (though we have designs for other e2e bits..). Honestly the best way to test this, if you're not sure that it does what it should, is to jump on the CI cluster and check everything out.

@squeed
Copy link
Contributor

squeed commented Aug 8, 2019

Hmm - if we're the ones generating this, can we just update our own env ;-) ?

@bparees
Copy link

bparees commented Aug 8, 2019

Hmm - if we're the ones generating this, can we just update our own env ;-) ?

?

the operator deployment is managed by the CVO. So you need this annotation so the CVO will add the proxy env vars to your deployment. If you tried to add them yourself, the CVO would stomp your deployment back to what it thinks it is supposed to look like (no env vars)

@squeed
Copy link
Contributor

squeed commented Aug 8, 2019

the operator deployment is managed by the CVO. So you need this annotation so the CVO will add the proxy env vars to your deployment. If you tried to add them yourself, the CVO would stomp your deployment back to what it thinks it is supposed to look like (no env vars)

I just meant with os.Setenv(). But perhaps that's racy.

@bparees
Copy link

bparees commented Aug 8, 2019

I just meant with os.Setenv(). But perhaps that's racy.

we were told that golang's transport libs initilize on load, so setting the env var after is not going to have a useful effect.

and @danehans that is something to consider when you are doing your readiness endpoint checking as well... ensuring that the transport connection you are opening is actually going through the proxy (in addition to using the generated CA bundle)

@bparees
Copy link

bparees commented Aug 8, 2019

/lgtm
(I don't have approver on this repo)

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bparees, danehans
To complete the pull request process, please assign squeed
You can assign the PR to them by writing /assign @squeed in a comment when ready.

The full list of commands accepted by this bot can be found 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

@danehans danehans force-pushed the operator_proxy_support branch from b0dcfed to 5802f26 Compare August 9, 2019 06:27
@openshift-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2019
@danehans
Copy link
Contributor Author

danehans commented Aug 9, 2019

@bparees @squat lgtm was removed since I pushed a new commit to fix the annotation value.

@squat
Copy link

squat commented Aug 9, 2019

cc the othe squ.*: @squeed

@danehans
Copy link
Contributor Author

danehans commented Aug 9, 2019

This PR may not be needed since #277 will use static proxy settings instead of from env vars.

/hold

@bparees
Copy link

bparees commented Aug 9, 2019

This PR may not be needed since #277 will use static proxy settings instead of from env vars.

it's needed if and only if the network operator itself (not its operands) needs to make off-cluster requests. (not including the off-cluster requests that will be made to the proxy readiness endpoint)

@danehans
Copy link
Contributor Author

danehans commented Aug 9, 2019

Closing b/c we don't believe CNO needs to consume proxy.

@danehans danehans closed this Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants