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.
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
Bug 1506978 - Apply proxy settings to running APBs #654
Conversation
* Broker should inspect its own enviornment, and in the presence of a proxy configuration, should apply those settings to executing APBs.
|
Currently trying to test this in the example locked down environment, but I confirmed the settings are being applied to running APBs as expected when the various env vars are set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, but otherwise LGTM
| httpsProxy, httpsProxyPresent := os.LookupEnv(httpsProxyEnvVar) | ||
| noProxy, noProxyPresent := os.LookupEnv(noProxyEnvVar) | ||
|
|
||
| // TODO: Probably some more permutations of these that should be validated? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be made into an issue to be tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I can file an issue after this merges and follow up with a patch + tests. Need to figure out what combinations are valid vs. not valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -201,6 +215,52 @@ func createExtraVars(context *Context, parameters *Parameters) (string, error) { | |||
| return string(extraVars), err | |||
| } | |||
|
|
|||
| func createPodEnv(executionContext ExecutionContext) []v1.EnvVar { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
Changes Unknown when pulling 6826358 on eriknelson:proxy-bug into ** on openshift:master**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
| }, | ||
| }, | ||
| }, | ||
| Env: createPodEnv(executionContext), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
| NoProxy: noProxy, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| @@ -201,6 +215,52 @@ func createExtraVars(context *Context, parameters *Parameters) (string, error) { | |||
| return string(extraVars), err | |||
| } | |||
|
|
|||
| func createPodEnv(executionContext ExecutionContext) []v1.EnvVar { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EXCELLENT!
* Broker should inspect its own enviornment, and in the presence of a proxy configuration, should apply those settings to executing APBs.
a proxy configuration, should apply those settings to executing APBs.