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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fcee6f9
Upgrade DWO dependency to v0.23.0
amisevsk Oct 17, 2023
80d722d
Disable DWO's workspace proxy handling when proxy configuration is de…
amisevsk Aug 10, 2023
76662e6
Set lowercase version of proxy env vars in addition to uppercase
amisevsk Aug 10, 2023
5baf24a
Update tests to include both http_proxy and HTTP_proxy
amisevsk Aug 10, 2023
ba1370b
Add documentation on why proxy configuration is disabled in DWO
amisevsk Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DEPENDENCIES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@
| [golang.org/x/mobile@d3739f865fa66d07c1f506505c18aac71a8ead6e](https://cs.opensource.google/go) | BSD-3-Clause | N/A |
| [github.com/devfile/api/v2@0d163445376d4d28898f3fbac4f2ff62863b944b](https://github.com/devfile/api.git) | Apache-2.0 | [clearlydefined](https://clearlydefined.io/definitions/git/github/devfile/api/0d163445376d4d28898f3fbac4f2ff62863b944b) |
| [github.com/che-incubator/kubernetes-image-puller-operator@0128446f5af78587c0427a35d693bbb8d24036bc](https://github.com/che-incubator/kubernetes-image-puller-operator.git) | EPL-2.0 | todo |
| [github.com/devfile/devworkspace-operator@v0.21.0](https://github.com/devfile/devworkspace-operator.git) | Apache-2.0 | [clearlydefined](https://clearlydefined.io/definitions/git/github/devfile/devworkspace-operator/21edf4373322c228ed54a5d4747b0451435a8f08) |
| [github.com/devfile/devworkspace-operator@v0.23.0](https://github.com/devfile/devworkspace-operator.git) | Apache-2.0 | [clearlydefined](https://clearlydefined.io/definitions/git/github/devfile/devworkspace-operator/2d9aa0fa7ba6296c51dd44269bf5c872f5dd41b7) |
| [github.com/gophercloud/[email protected]](https://github.com/gophercloud/gophercloud) | Apache-2.0 | [clearlydefined](https://clearlydefined.io/definitions/go/golang/github.com%2Fgophercloud/gophercloud/v0.1.0) |
| [gopkg.in/imdario/[email protected]](https://github.com/imdario/mergo/) | BSD-3-Clause | [clearlydefined](https://clearlydefined.io/definitions/go/golang/github.com%2Fimdario/mergo/v0.3.7) |
| [github.com/mikefarah/yq/[email protected]](https://github.com/mikefarah/yq) | MIT | [clearlydefined](https://clearlydefined.io/definitions/git/github/mikefarah/yq/b8b2c9de6189471c0cdbd459b5b0b49a57844bd4) |
Expand Down
4 changes: 2 additions & 2 deletions bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ metadata:
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
repository: https://github.com/eclipse-che/che-operator
support: Eclipse Foundation
name: eclipse-che.v7.76.0-811.next
name: eclipse-che.v7.76.0-812.next
namespace: placeholder
spec:
apiservicedefinitions: {}
Expand Down Expand Up @@ -1234,7 +1234,7 @@ spec:
minKubeVersion: 1.19.0
provider:
name: Eclipse Foundation
version: 7.76.0-811.next
version: 7.76.0-812.next
webhookdefinitions:
- admissionReviewVersions:
- v1
Expand Down
3 changes: 3 additions & 0 deletions controllers/usernamespace/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,15 @@
proxySettings := map[string]string{}
if proxyConfig.HttpProxy != "" {
proxySettings["HTTP_PROXY"] = proxyConfig.HttpProxy
proxySettings["http_proxy"] = proxyConfig.HttpProxy

Check warning on line 408 in controllers/usernamespace/controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usernamespace/controller.go#L408 

Added line #L408 was not covered by tests
}
if proxyConfig.HttpsProxy != "" {
proxySettings["HTTPS_PROXY"] = proxyConfig.HttpsProxy
proxySettings["https_proxy"] = proxyConfig.HttpsProxy

Check warning on line 412 in controllers/usernamespace/controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usernamespace/controller.go#L412 

Added line #L412 was not covered by tests
}
if proxyConfig.NoProxy != "" {
proxySettings["NO_PROXY"] = proxyConfig.NoProxy
proxySettings["no_proxy"] = proxyConfig.NoProxy
}

key := client.ObjectKey{Name: prefixedName("proxy-settings"), Namespace: targetNs}
Expand Down
2 changes: 1 addition & 1 deletion controllers/usernamespace/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func TestCreatesDataInNamespace(t *testing.T) {
assert.Equal(t, "true", proxySettings.GetLabels()[dwconstants.DevWorkspaceMountLabel],
"proxy settings should be labeled as mounted")

assert.Equal(t, 1, len(proxySettings.Data), "Expecting just 1 element in the default proxy settings")
assert.Equal(t, 2, len(proxySettings.Data), "Expecting 2 elements in the default proxy settings")

assert.Equal(t, ".svc", proxySettings.Data["NO_PROXY"], "Unexpected proxy settings")

Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ go 1.18
require (
github.com/che-incubator/kubernetes-image-puller-operator v0.0.0-20210929175054-0128446f5af7
github.com/devfile/api/v2 v2.2.1-alpha.0.20230413012049-a6c32fca0dbd
github.com/devfile/devworkspace-operator v0.22.0
github.com/devfile/devworkspace-operator v0.23.0
github.com/go-logr/logr v1.2.3
github.com/golang/mock v1.5.0
github.com/google/go-cmp v0.5.9
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ github.com/deislabs/oras v0.8.1/go.mod h1:Mx0rMSbBNaNfY9hjpccEnxkOqJL6KGjtxNHPLC
github.com/denisenkom/go-mssqldb v0.0.0-20190204142019-df6d76eb9289/go.mod h1:xN/JuLBIz4bjkxNmByTiV1IbhfnYb6oo99phBn4Eqhc=
github.com/devfile/api/v2 v2.2.1-alpha.0.20230413012049-a6c32fca0dbd h1:HpGR728CfB6BB9ZuFtQb0UeTIYNFgpuGsuoMOJNMUTM=
github.com/devfile/api/v2 v2.2.1-alpha.0.20230413012049-a6c32fca0dbd/go.mod h1:qp8jcw12y1JdCsxjK/7LJ7uWaJOxcY1s2LUk5PhbkbM=
github.com/devfile/devworkspace-operator v0.22.0 h1:Q2w419Y4E5yOYxIY6LI4u4fHi5YewKQsRIN1J4Jlh3Q=
github.com/devfile/devworkspace-operator v0.22.0/go.mod h1:42cQKSbE+Zdqez8X5IqlEfdeeA0a/LkOTe2kkekJX6c=
github.com/devfile/devworkspace-operator v0.23.0 h1:QUJXt6OtKILTuQUbSE2r6zyIysxhqx9sgZeMo1l7yJs=
github.com/devfile/devworkspace-operator v0.23.0/go.mod h1:42cQKSbE+Zdqez8X5IqlEfdeeA0a/LkOTe2kkekJX6c=
github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
github.com/dhui/dktest v0.3.2/go.mod h1:l1/ib23a/CmxAe7yixtrYPc8Iy90Zy2udyaHINM5p58=
github.com/docker/cli v0.0.0-20200130152716-5d0cf8839492 h1:FwssHbCDJD025h+BchanCwE1Q8fyMgqDr2mOQAWOLGw=
Expand Down
26 changes: 24 additions & 2 deletions pkg/deploy/dev-workspace-config/dev_workspace_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
dwoc.Config = &controllerv1alpha1.OperatorConfiguration{}
}

if err := updateWorkspaceConfig(ctx.CheCluster, dwoc.Config); err != nil {
if err := updateWorkspaceConfig(ctx, dwoc.Config); err != nil {
return reconcile.Result{}, false, err
}

Expand All @@ -78,7 +78,8 @@
return true
}

func updateWorkspaceConfig(cheCluster *chev2.CheCluster, operatorConfig *controllerv1alpha1.OperatorConfiguration) error {
func updateWorkspaceConfig(ctx *chetypes.DeployContext, operatorConfig *controllerv1alpha1.OperatorConfiguration) error {
cheCluster := ctx.CheCluster
devEnvironments := &cheCluster.Spec.DevEnvironments
if operatorConfig.Workspace == nil {
operatorConfig.Workspace = &controllerv1alpha1.WorkspaceConfig{}
Expand All @@ -104,6 +105,17 @@

updateWorkspaceImagePullPolicy(devEnvironments.ImagePullPolicy, operatorConfig.Workspace)

// If the CheCluster has a configured proxy, or if the Che Operator has detected a proxy configuration,
// we need to disable automatic proxy handling in the DevWorkspace Operator as its implementation collides
// with ours -- they set environment variables the deployment spec explicitly, which overrides the proxy-settings
// automount configmap.
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 •
edited
Loading

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".

if operatorConfig.Routing == nil {
operatorConfig.Routing = &controllerv1alpha1.RoutingConfig{}
}
disableDWOProxy(operatorConfig.Routing)

Check warning on line 116 in pkg/deploy/dev-workspace-config/dev_workspace_config.go

View check run for this annotation

Codecov / codecov/patch

pkg/deploy/dev-workspace-config/dev_workspace_config.go#L113-L116 

Added lines #L113 - L116 were not covered by tests
}

operatorConfig.Workspace.DeploymentStrategy = v1.DeploymentStrategyType(utils.GetValue(string(devEnvironments.DeploymentStrategy), constants.DefaultDeploymentStrategy))
return nil
}
Expand Down Expand Up @@ -213,6 +225,16 @@
workspaceConfig.ProjectCloneConfig.Resources = cheResourcesToCoreV1Resources(container.Resources)
}

func disableDWOProxy(routingConfig *controllerv1alpha1.RoutingConfig) {
// Since we create proxy configmaps to mount proxy settings, we want to disable
// proxy handling in DWO; otherwise the env vars added by DWO will override the env
// vars we intend to mount via configmap.
routingConfig.ProxyConfig = &controllerv1alpha1.Proxy{}
routingConfig.ProxyConfig.HttpProxy = pointer.String("")
routingConfig.ProxyConfig.HttpsProxy = pointer.String("")
routingConfig.ProxyConfig.NoProxy = pointer.String("")

Check warning on line 235 in pkg/deploy/dev-workspace-config/dev_workspace_config.go

View check run for this annotation

Codecov / codecov/patch

pkg/deploy/dev-workspace-config/dev_workspace_config.go#L228-L235 

Added lines #L228 - L235 were not covered by tests
}

// Returns the default container security context required for container builds.
// Returns an error if the default container security context could not be retrieved.
func getDefaultContainerSecurityContext() (*corev1.SecurityContext, error) {
Expand Down
18 changes: 11 additions & 7 deletions ...evfile/devworkspace-operator/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ...b.com/devfile/devworkspace-operator/apis/controller/v1alpha1/devworkspacerouting_types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion ...ithub.com/devfile/devworkspace-operator/apis/controller/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions ...ace-operator/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 34 additions & 12 deletions vendor/github.com/devfile/devworkspace-operator/pkg/config/proxy/openshift.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ github.com/davecgh/go-spew/spew
github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2
github.com/devfile/api/v2/pkg/attributes
github.com/devfile/api/v2/pkg/devfile
# github.com/devfile/devworkspace-operator v0.22.0
# github.com/devfile/devworkspace-operator v0.23.0
## explicit; go 1.18
github.com/devfile/devworkspace-operator/apis/controller/v1alpha1
github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting
Expand Down
Loading