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

Configure DWO to not process proxy settings for workspaces Che manages #1741

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Aug 11, 2023

What does this PR do?

If Che detects proxy settings on a cluster (either through configuration or the OpenShift cluster-wide proxy), configure the DevWorkspace Operator to not use any proxy settings internally for workspaces.

This is necessary to allow the proxy-settings configmap provisioned by Che to actually be used in workspaces; otherwise, DWO will set the proxy environment variables in the workspace deployment directly, overriding env vars set from the configmap.

In addition, this PR updates the proxy env configmap to include lowercase versions of environment variables (i.e. http_proxy, https_proxy, no_proxy) to try to ensure compatibility with all tools (e.g. curl only uses http_proxy, not HTTP_PROXY)

This PR is marked WIP as it depends on changes from devfile/devworkspace-operator#1147; once DWO v0.23 is released, the first commit in this PR needs to be updated to use 0.23 instead of a main-branch commit.

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

Closes eclipse-che/che#22370

How to test this PR?

Changes from this PR are pushed to quay.io/amisevsk/che-operator:proxy-config

To test,

  1. Start an OpenShift cluster with a cluster-wide proxy configured
  2. Install Che + DWO: chectl server:deploy -p openshift --olm-channel next
  3. Update the eclipse-che CSV (eclipse-che.<ver>.next) to use quay.io/amisevsk/che-operator:proxy-config for the Che Operator image
  4. Start a workspace

Once workspace is running, verify

  1. DevWorkspace operator config is configured to not set proxy variables:
    oc get dwoc -n eclipse-che devworkspace-config -o yaml
    (...)
      config:
        routing:
          proxyConfig:
            httpProxy: ""
            httpsProxy: ""
            noProxy: ""
    (...)
    
  2. The workspace's deployment does not set proxy environment variables directly
  3. Proxy variables are still present in workspace, mounted from configmap
    $ env | grep -i proxy
    HTTP_PROXY=http://<snip>@10.0.15.91:3128/
    https_proxy=http://<snip>@10.0.15.91:3128/
    http_proxy=http://<snip>@10.0.15.91:3128/
    no_proxy=.cluster.local,#etc
    NO_PROXY=.cluster.local,#etc
    HTTPS_PROXY=http://<snip>@10.0.15.91:3128/
    
  4. Changes to proxy settings in CheCluster propagate down to workspaces and aren't ignored due to DWO's conflicting config

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

…tected

If we allow the DevWorkspace Operator to handle proxy settings for workspaces,
it will add proxy environment variables to workspace containers with the
values it detects on the cluster (or through its own
DevWorkspaceOperatorConfig)

Since these environment variables are defined in the deployment yaml,
their values override values for proxy environment variables defined by
the automount 'proxy-config' configmap.

To avoid this, we configure DWO to not set any proxy settings for
workspaces we manage.

Signed-off-by: Angel Misevski <[email protected]>
To hopefully ensure all tools respect proxy settings for a workspace,
add both upper- and lower-case versions of the proxy environment
variables to workspaces.

For example, curl will only use the lower-case http_proxy, and will
ignore HTTP_PROXY.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the dwoc-proxy-settings branch from cc5075c to 5baf24a Compare October 17, 2023 17:39
@amisevsk amisevsk changed the title [WIP] Configure DWO to not process proxy settings for workspaces Che manages Configure DWO to not process proxy settings for workspaces Che manages Oct 17, 2023
@amisevsk
Copy link
Contributor Author

PR is now ready for review

@@ -104,6 +105,13 @@ func updateWorkspaceConfig(cheCluster *chev2.CheCluster, operatorConfig *control

updateWorkspaceImagePullPolicy(devEnvironments.ImagePullPolicy, operatorConfig.Workspace)

if ctx.Proxy.HttpProxy != "" || ctx.Proxy.HttpsProxy != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Could you add some comments explaining why we have to clean up proxy config, please?
  2. There is something that I don't understand. If proxy is set, then we have to clean up to DWO proxy config. If proxy is not set, then we do nothing. What is the point of DWO proxy config then?

Copy link
Contributor Author

@amisevsk amisevsk Oct 18, 2023

Choose a reason for hiding this comment

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

Yup, I agree some comments here would be helpful.

As for why DWO supports configuring the proxy, the answer is twofold: first, it exists as an option because we implemented it (way back in v0.12); second, it's a feature that's still used by other consumers of DevWorkspaces, such as Web Terminal.

Che introduced its own implementation of managing proxy settings for DevWorkspaces, but the two implementations collide -- Che supports the proxy by creating a che-proxy-settings configmap which is mounted as env vars, and so needs to disable built-in DWO proxy configuration/detection.

Basically, what we're doing here is telling DWO "we'll handle setting up proxy environment variables, you shouldn't do it for this workspace".

Co-authored-by: Anatolii Bazko <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the dwoc-proxy-settings branch from a74fc10 to ba1370b Compare October 18, 2023 19:52
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #1741 (ba1370b) into main (af6dd52) will decrease coverage by 0.08%.
Report is 2 commits behind head on main.
The diff coverage is 34.78%.

@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
- Coverage   59.58%   59.50%   -0.08%     
==========================================
  Files          71       71              
  Lines        8571     8592      +21     
==========================================
+ Hits         5107     5113       +6     
- Misses       3115     3129      +14     
- Partials      349      350       +1     
Files Coverage Δ
controllers/usernamespace/controller.go 69.87% <33.33%> (-0.23%) ⬇️
...eploy/dev-workspace-config/dev_workspace_config.go 85.32% <35.00%> (-6.25%) ⬇️

@openshift-ci
Copy link

openshift-ci bot commented Oct 18, 2023

@amisevsk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v12-devworkspace-happy-path a74fc10 link true /test v12-devworkspace-happy-path

Full PR test history. Your PR dashboard.

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.

Copy link
Contributor

@tolusha tolusha left a comment

Choose a reason for hiding this comment

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

Thank you for explanation.
Please merge.

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, tolusha

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

@amisevsk amisevsk merged commit 851f110 into eclipse-che:main Oct 19, 2023
13 of 15 checks passed
@amisevsk amisevsk deleted the dwoc-proxy-settings branch October 19, 2023 17:10
@devstudio-release
Copy link

Build 3.10 :: operator_3.x/304: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: operator_3.x/306: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/4889: FAILURE

devspaces-operator : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/4891: FAILURE

devspaces-operator-bundle : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/4894: FAILURE

devspaces-operator-bundle : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: get-sources-rhpkg-container-build_3.x/4896: FAILURE

devspaces-operator-bundle : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Che proxy settings configmaps are ignored on OpenShift if cluster-wide proxy is enabled.
4 participants