WINC-1152, OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list #1998

saifshaikh48 · 2024-01-04T17:25:40Z

Reverts an erroneous sanitization step in WMCO that replaced commas with semicolons in the user's cluster-wide proxy configuration. This was causing Windows to ignore the values set in the noProxy environment variable, incorrectly sending traffic through the proxy.

From the Azure PowerShell docs:
NO_PROXY: a comma-separated list of hostnames that should be excluded from proxying. [1]
noProxy list should not contain semicolon delimiters (;). Use (,) to delimit the noProxy domains. [2]

openshift-ci · 2024-01-04T17:27:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Reverts an erroneous sanitization step in WMCO that replaced commas with semicolons in the user's cluster-wide proxy configuration. This was causing Windows to ignore the values set in the noProxy environment variable, incorrectly sending traffic through the proxy. From the Azure PowerShell docs: `NO_PROXY: a comma-separated list of hostnames that should be excluded from proxying.` `noProxy list should not contain semicolon delimiters (;). Use (,) to delimit the noProxy domains.`

saifshaikh48 · 2024-01-04T17:35:13Z

/test vsphere-proxy-e2e-operator
/test wicd-unit-vsphere

saifshaikh48 · 2024-01-04T20:23:11Z

/test vsphere-proxy-e2e-operator

jrvaldes

@saifshaikh48 thanks for working on this. PTAL at my comments.

jrvaldes · 2024-01-05T16:46:59Z

pkg/cluster/config.go

-			// on Windows, hostname lists are separated by semicolons rather than the Linux default of commas
-			sanitizedVal := strings.ReplaceAll(value, ",", ";")
-			clusterWideProxyVars[envVar] = sanitizedVal
+			clusterWideProxyVars[envVar] = value


Not sure about unquestioningly trusting the content of value. Does this mertis a check/validation here?

What do you think about trimming spaces? How does Windows react to localhost, 127.0.0.2?

Managing entities (CVO and/or OLM) do basic validations around this to ensure the URLs/domains are well-formed. No further checks needed from our side

I was thinking more about the token separator, i.e. ; or ,

jrvaldes · 2024-01-05T16:52:50Z

pkg/daemon/controller/controller_test.go

-			existingEnvVars:              map[string]string{"NO_PROXY": "localhost;127.0.0.1", "HTTP_PROXY": "http://example.com"},
-			configMapEnvVars:             map[string]string{"NO_PROXY": "localhost;127.0.0.1"},
+			existingEnvVars:              map[string]string{"NO_PROXY": "localhost,127.0.0.1", "HTTP_PROXY": "http://example.com"},
+			configMapEnvVars:             map[string]string{"NO_PROXY": "localhost,127.0.0.1"},


If a check is introduced, consider adding a test with an invalid value. e.g localhost;27.0.0.2

semicolon-separated value is still valid from WICD unit test/Windows OS perspective. The WMCO e2e integration tests would fail/signal to handle this

jrvaldes · 2024-01-05T16:55:41Z

pkg/cluster/config.go

@@ -366,9 +366,7 @@ func GetProxyVars() map[string]string {
 	for _, envVar := range WatchedEnvironmentVars {


clusterWideProxyVars = make(map[string]string, 3)

It might be out of scope, but consider avoiding the magic constant (3) with:

clusterWideProxyVars = make(map[string]string, len(WatchedEnvironmentVars))

saifshaikh48 · 2024-01-08T15:04:43Z

I was wondering why I thought semicolons were used in PS. Turns out the manual testing we did was using netsh, one of the only tools (along with Microsoft Edge CLI) that uses a bypass-list

To list multiple addresses and host names, separate them with blank spaces or semicolons in the proxy bypass string

mansikulkarni96 · 2024-01-08T20:09:28Z

/lgtm

saifshaikh48 · 2024-01-08T23:01:44Z

/test vsphere-proxy-e2e-operator
/test aws-e2e-upgrade

sebsoto · 2024-01-09T16:53:59Z

test/e2e/proxy_test.go

+	if err != nil {
+		require.NoError(t, err)
+	}


there should be no if statement
just require.NoError

test/e2e/proxy_test.go

sebsoto · 2024-01-09T17:02:59Z

test/e2e/proxy_test.go

+			command := "$response = $(Invoke-WebRequest -UseBasicParsing https://raw.githubusercontent.com/openshift/windows-machine-config-operator/master/README.md); $response.Headers['Via']"
+			out, err := tc.runPowerShellSSHJob("get-webrequest-via-header", command, addr)
+			require.NoErrorf(t, err, "unable to retrieve response 'Via' header")
+
+			viaHeader := strings.TrimSpace(out)


Please make a helper function for this.
such as func (tc *testContext) getOutboundProxy or getOutboundHTTPViaHeaders
A documented helper will help with code readability.

An example of the returned string would be good here.

it also stops the duplicated code thats occuring in the noproxy test

100%, will remove the code duplication in the final draft

sebsoto · 2024-01-09T17:04:18Z

test/e2e/proxy_test.go

+			assert.Truef(t, viaHeader == clusterProxy.Status.HTTPProxy || viaHeader == clusterProxy.Status.HTTPSProxy,
+				"expected response header 'Via' to be either '%s' or '%s' but instead held value: '%s'",
+				clusterProxy.Status.HTTPProxy, clusterProxy.Status.HTTPSProxy, viaHeader)
+		})


Can there be multiple via headers?
Seems that way looking at MDN docs.
How is that being handled?

Good catch. I'll change this to search for a via reader whose value is the cluster-wide proxy
along the lines of
$response.Headers["Via"] == clusterProxy.Status.HTTPProxy || $response.Headers["Via"] == clusterProxy.Status.HTTPSProxy

sebsoto · 2024-01-09T17:07:23Z

test/e2e/proxy_test.go

+// TODO: circumvent the proxy if the target address is specified in the NO_PROXY list
+func (tc *testContext) testNoProxyRequest(t *testing.T) {
+	clusterProxy, err := tc.client.Config.ConfigV1().Proxies().Get(context.TODO(), "cluster", meta.GetOptions{})
+	if err != nil {


drop the if

test/e2e/proxy_test.go

sebsoto · 2024-01-09T17:09:50Z

test/e2e/proxy_test.go

+			addr, err := controllers.GetAddress(node.Status.Addresses)
+			require.NoError(t, err, "unable to get node address")
+
+			command := "$response = $(Invoke-WebRequest -UseBasicParsing https://raw.githubusercontent.com/openshift/windows-machine-config-operator/master/README.md); $response.Headers['Via']"


remind me, is this only for external connections?
can we launch a service and query that via DNS so we arent relying on google and github?

I had thought of that initially but, yes external requests only

jrvaldes · 2024-01-09T23:22:00Z

test/e2e/proxy_test.go

@@ -39,6 +39,9 @@ const (
 	// userCABundleName is the name of the ConfigMap that holds additional user-provided proxy certs
 	userCABundleName      = "user-ca-bundle"
 	userCABundleNamespace = "openshift-config"
+
+	// whitelist is a list of domains that are not to be proxied
+	whitelist = "static.redhat.com,redhat.io,google.com"


s/whitelist/noProxyList

jrvaldes · 2024-01-09T23:24:28Z

test/e2e/proxy_test.go

@@ -39,6 +39,9 @@ const (
 	// userCABundleName is the name of the ConfigMap that holds additional user-provided proxy certs
 	userCABundleName      = "user-ca-bundle"
 	userCABundleNamespace = "openshift-config"
+
+	// whitelist is a list of domains that are not to be proxied


noProxy is a comma-separated list of hostnames and/or CIDRs and/or IPs for
which the proxy should not be used.

As per oc explain proxies.spec.noProxy.

jrvaldes · 2024-01-09T23:25:34Z

test/e2e/proxy_test.go

@@ -351,6 +405,21 @@ func (tc *testContext) getEnvVar(addr, name, command string) (map[string]string,
 	return parseWindowsEnvVars(out), nil
 }

+// configureNoProxy configures the cluster-wide proxy's bypass list with specific URLs for testing purposes
+func (tc *testContext) configureNoProxy() error {


Consider passing the no proxy list as a parameter of the function.

jrvaldes · 2024-01-09T23:27:19Z

test/e2e/proxy_test.go

+	if err != nil {
+		return fmt.Errorf("unable to patch %s: %w", string(patchData), err)
+	}
+	return nil


Consider returning just err instead.

prefer more context here IMO. Having the patchData in the error makes it much easier to debug if something goes wrong

saifshaikh48 · 2024-01-10T20:32:27Z

/test vsphere-proxy-e2e-operator

saifshaikh48 · 2024-01-11T15:28:15Z

/test vsphere-proxy-e2e-operator

mansikulkarni96

Thanks for working on this @saifshaikh48 . This looks much clear and concise.

mansikulkarni96 · 2024-01-11T16:41:41Z

test/e2e/proxy_test.go

+			})
+
+			t.Run("NO_PROXY request", func(t *testing.T) {
+				whitelistedUrl := "google.com"


how about permitted/nonproxied URL

openshift-ci-robot · 2024-01-11T18:33:02Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-19997, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Reverts an erroneous sanitization step in WMCO that replaced commas with semicolons in the user's cluster-wide proxy configuration. This was causing Windows to ignore the values set in the noProxy environment variable, incorrectly sending traffic through the proxy.

From the Azure PowerShell docs:
NO_PROXY: a comma-separated list of hostnames that should be excluded from proxying. [1]
noProxy list should not contain semicolon delimiters (;). Use (,) to delimit the noProxy domains. [2]

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 openshift-eng/jira-lifecycle-plugin repository.

alinaryan · 2024-01-15T20:47:59Z

/lgtm

saifshaikh48 · 2024-01-15T22:22:19Z

/test vsphere-proxy-e2e-operator

sebsoto · 2024-01-15T22:26:52Z

test/e2e/proxy_test.go

+}
+
+// verifyCurlOutput parses the output of a curl request searching for any proxy variables in use and proxy headers
+func verifyCurlOutput(output, httpProxyURL, noProxyList string) (bool, bool, bool) {


should this be named noProxyList, or noProxyURL?
if a comma separated list is passed in would that work?

A list works (that’s what the test does now) and a single URL would also work (if the noProxy value in the cluster wide proxy was just a single URL)

openshift-ci · 2024-01-15T22:27:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saifshaikh48, sebsoto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sebsoto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saifshaikh48 · 2024-01-15T22:42:32Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-01-15T22:42:34Z

@saifshaikh48: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.15

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.

saifshaikh48 · 2024-01-15T22:42:41Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2024-01-15T22:42:44Z

@saifshaikh48: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14

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.

saifshaikh48 · 2024-01-16T15:11:02Z

/retest-required

Not sure why none of the e2e tests are kicking off automatically.

saifshaikh48 · 2024-01-16T15:39:10Z

/test aws-e2e-operator
/test aws-e2e-upgrade
/test azure-e2e-operator
/test azure-e2e-upgrade
/test gcp-e2e-operator
/test nutanix-e2e-operator
/test platform-none-vsphere-e2e-operator
/test vsphere-e2e-operator
/test wicd-unit-vsphere

openshift-ci-robot · 2024-01-16T16:44:33Z

/retest-required

Remaining retests: 0 against base HEAD 74fd810 and 2 for PR HEAD aa5e824 in total

saifshaikh48 · 2024-01-16T18:29:54Z

/test aws-e2e-upgrade

openshift-ci · 2024-01-16T20:34:36Z

@saifshaikh48: all tests passed!

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.

openshift-ci-robot · 2024-01-16T20:38:49Z

@saifshaikh48: Jira Issue OCPBUGS-24264: All pull requests linked via external trackers have merged:

openshift/windows-machine-config-operator#1998

Jira Issue OCPBUGS-24264 has been moved to the MODIFIED state.

In response to this:

Reverts an erroneous sanitization step in WMCO that replaced commas with semicolons in the user's cluster-wide proxy configuration. This was causing Windows to ignore the values set in the noProxy environment variable, incorrectly sending traffic through the proxy.

From the Azure PowerShell docs:
NO_PROXY: a comma-separated list of hostnames that should be excluded from proxying. [1]
noProxy list should not contain semicolon delimiters (;). Use (,) to delimit the noProxy domains. [2]

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-cherrypick-robot · 2024-01-16T20:39:37Z

@saifshaikh48: new pull request created: #2015

In response to this:

/cherry-pick release-4.15

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.

openshift-cherrypick-robot · 2024-01-16T20:40:18Z

@saifshaikh48: new pull request created: #2016

In response to this:

/cherry-pick release-4.14

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.

jrvaldes · 2024-04-23T01:59:37Z

/retitle WINC-1152, OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list

jrvaldes · 2024-04-23T02:00:07Z

/jira refresh

openshift-ci-robot · 2024-04-23T02:00:09Z

@jrvaldes: Jira Issue OCPBUGS-24264 is in an unrecognized state (Verified) and will not be moved to the MODIFIED state.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2024

saifshaikh48 force-pushed the commas-in-noproxy branch from 4679c38 to 9ac4b07 Compare January 4, 2024 17:32

jrvaldes reviewed Jan 5, 2024

View reviewed changes

openshift-ci bot assigned mansikulkarni96 Jan 8, 2024

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 8, 2024

[cluster] Remove magic number for env vars

af9b891

saifshaikh48 force-pushed the commas-in-noproxy branch from d55ed02 to 8d4ac6e Compare January 8, 2024 23:00

sebsoto reviewed Jan 9, 2024

View reviewed changes

sebsoto requested changes Jan 9, 2024

View reviewed changes

saifshaikh48 force-pushed the commas-in-noproxy branch 2 times, most recently from c443b9e to 352653a Compare January 9, 2024 18:00

jrvaldes reviewed Jan 9, 2024

View reviewed changes

mansikulkarni96 reviewed Jan 11, 2024

View reviewed changes

saifshaikh48 force-pushed the commas-in-noproxy branch 2 times, most recently from 236d1d3 to 615e880 Compare January 11, 2024 18:32

saifshaikh48 changed the title ~~WIP: [cluster] Keep comma as delimiter in noProxy list~~ OCPBUGS-24264, OCPBUGS-19997: [cluster] Keep comma as delimiter in noProxy list Jan 11, 2024

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2024

sebsoto approved these changes Jan 15, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2024

saifshaikh48 marked this pull request as ready for review January 15, 2024 22:41

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2024

openshift-ci bot requested review from mansikulkarni96 and sebsoto January 15, 2024 22:41

openshift-merge-bot bot merged commit 626d09c into openshift:master Jan 16, 2024
17 checks passed

openshift-cherrypick-robot mentioned this pull request Jan 16, 2024

[release-4.15] OCPBUGS-27239: [cluster] Keep comma as delimiter in noProxy list #2015

Merged

openshift-cherrypick-robot mentioned this pull request Jan 16, 2024

[release-4.14] OCPBUGS-27240: [cluster] Keep comma as delimiter in noProxy list #2016

Closed

openshift-ci bot changed the title ~~OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list~~ WINC-1152, OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list Apr 23, 2024

		@@ -366,9 +366,7 @@ func GetProxyVars() map[string]string {
		for _, envVar := range WatchedEnvironmentVars {

WINC-1152, OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list #1998

WINC-1152, OCPBUGS-24264: [cluster] Keep comma as delimiter in noProxy list #1998

Conversation

saifshaikh48 commented Jan 4, 2024

openshift-ci bot commented Jan 4, 2024

saifshaikh48 commented Jan 4, 2024

saifshaikh48 commented Jan 4, 2024

jrvaldes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saifshaikh48 commented Jan 8, 2024

mansikulkarni96 commented Jan 8, 2024

saifshaikh48 commented Jan 8, 2024

sebsoto Jan 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saifshaikh48 commented Jan 10, 2024

saifshaikh48 commented Jan 11, 2024

mansikulkarni96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 11, 2024

alinaryan commented Jan 15, 2024

saifshaikh48 commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 15, 2024

saifshaikh48 commented Jan 15, 2024

openshift-cherrypick-robot commented Jan 15, 2024

saifshaikh48 commented Jan 15, 2024

openshift-cherrypick-robot commented Jan 15, 2024

saifshaikh48 commented Jan 16, 2024

saifshaikh48 commented Jan 16, 2024

openshift-ci-robot commented Jan 16, 2024

saifshaikh48 commented Jan 16, 2024

openshift-ci bot commented Jan 16, 2024

openshift-ci-robot commented Jan 16, 2024

openshift-cherrypick-robot commented Jan 16, 2024

openshift-cherrypick-robot commented Jan 16, 2024

jrvaldes commented Apr 23, 2024

jrvaldes commented Apr 23, 2024

openshift-ci-robot commented Apr 23, 2024

sebsoto Jan 9, 2024 •

edited