Add proxy support#254
Conversation
openshift-ci-robot
added
the
size/M
| } | ||
|
|
||
| func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, canMountCustomLogo bool) *appsv1.Deployment { | ||
| func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, rt *routev1.Route, proxyConfig *configv1.Proxy, canMountCustomLogo bool) *appsv1.Deployment { |
There was a problem hiding this comment.
I think this function call has gotten long enough that we should pass in a struct. Maybe add that to tech debt?
There was a problem hiding this comment.
Yeah definitely would add it as techdebt 👍
| if proxyConfig == nil { | ||
| return envVars | ||
| } | ||
| if len(proxyConfig.Status.HTTPSProxy) != 0 { |
There was a problem hiding this comment.
You do a check on proxyConfig if it is nil, but what about proxyConfig.Status? Is that guaranteed to be there?
There was a problem hiding this comment.
Shouldn't be needed since it's not a pointer
|
/retest |
1 similar comment
|
/retest |
|
|
||
| proxyConfig, err := c.proxyCfgLister.Get("cluster") | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { |
There was a problem hiding this comment.
👍 The proxy config might not be there always on upgrade, and we shouldn't error. Good catch.
I believe we can remove this in 4.3.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, spadgett, zherman0 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@jhadvig We'll need to make sure the proxy is not used when connecting to internal services from console, but is used for the OAuth server. See openshift/console#1249 |
|
/retest |
| resourceSyncer: resourceSyncer, | ||
| // proxy | ||
| proxyCfgLister: proxyCfgInformer.Lister(), | ||
| proxyCfgInformer: proxyCfgInformer.Informer(), |
There was a problem hiding this comment.
I don't believe proxyCfgInformer: proxyCfgInformer.Informer(), is needed here. You have the WithInformer() call with the proxyCfgInformer below.
6 participants
Adding console support for cluster proxy. If the Proxy CRD is set, operator will read them and set proper environment variables inside the console container.
FollowUp should be setting the proxy on the console server http client as does in.
/assign @benjaminapetersen @zherman0
Story: https://jira.coreos.com/browse/CONSOLE-1440