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.

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

Add Proxy env variables for workspace containers in Openshift and Kub… #10387

Merged
merged 8 commits into from
Jul 31, 2018

Conversation

mkuznyetsov
Copy link
Contributor

…ernetes infrastructires

What does this PR do?

Add provisioning for http_proxy, https_proxy, no_proxy environment variables of workspace containers
in Kubernetes and Openshift deployment/template.

What issues does this PR fix or reference?

#10350

Release Notes

Docs PR

@mkuznyetsov mkuznyetsov requested a review from a user July 12, 2018 07:00
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Jul 12, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

can you use UPPER_CASE for proxy envs? HTTP_PROXY, HTTPS_PROXY, NO_PROXY

@Override
public void provision(KubernetesEnvironment k8sEnv, RuntimeIdentity identity)
throws InfrastructureException {
for (Pod pod : k8sEnv.getPods().values()) {
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to check if proxyEnvVars is not empty before foreaching all pods and containers

@@ -73,3 +73,6 @@ data:
{{- end }}
CHE_INFRA_KUBERNETES_SERVER__STRATEGY: {{ .Values.global.serverStrategy }}
CHE_LOGS_APPENDERS_IMPL: "plaintext"
CHE_WORKSPACE_HTTP__PROXY: ""
Copy link
Member

Choose a reason for hiding this comment

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

Is there an separated issue to make it possible to configure these properties via helm chart parameters?

import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;

/** @author Mykhailo Kuznietsov */
public class ProxySettingsProvisioner implements ConfigurationProvisioner {
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to use these properties on other infrastructures? Maybe it makes sense to move provisioning of these environment variables to Workspace API level.

private static final String HTTP_PROXY = "http_proxy";
private static final String NO_PROXY = "no_proxy";

// note that values are the same for now
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this comment? Why are they supposed to be the same since there are separated configuration properties?

import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;

/** @author Mykhailo Kuznietsov */
public class ProxySettingsProvisioner implements ConfigurationProvisioner {
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding java doc and maybe unit tests.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

@mkuznyetsov
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10387
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@ghost ghost mentioned this pull request Jul 25, 2018
@mkuznyetsov mkuznyetsov merged commit 1170a06 into master Jul 31, 2018
@mkuznyetsov mkuznyetsov deleted the che-10350 branch July 31, 2018 14:27
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 31, 2018
@benoitf benoitf added this to the 6.9.0 milestone Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants