OCPBUGS-19716: Enable proxy cert test to run in CI #1823

saifshaikh48 · 2023-09-22T20:38:20Z

Proxy tests were not actually running due to an issue with the proxy enabled check.
When enabled, a few of the tests were failing so one was patched and the other was
disabled with the fix coming in another PR (#1800).

openshift-ci · 2023-09-22T20:38:24Z

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

saifshaikh48 · 2023-09-22T20:39:00Z

/test vsphere-proxy-e2e-operator

openshift-ci-robot · 2023-09-26T18:42:22Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-19716, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

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

In response to this:

Proxy tests where not actually running due to an issue with the proxy enabled check.
When enabled, a few of the tests were failing so those are patched as well.

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 · 2023-09-26T18:59:34Z

/test vsphere-proxy-e2e-operator

cherry-picked from #1800 which had green CI. let's see if there is a timing issue in the tests

saifshaikh48 · 2023-09-27T14:09:49Z

/test vsphere-proxy-e2e-operator

openshift-ci-robot · 2023-09-27T15:43:34Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-19716, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

In response to this:

Proxy tests where not actually running due to an issue with the proxy enabled check.
When enabled, a few of the tests were failing so one was patched and the other was
disabled with the fix coming in another PR (#1800).

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 · 2023-09-27T15:46:33Z

test/e2e/proxy_test.go

+					// First requires data to be written to a file and then provide the file path the cert constructor
+					"Set-Content C:\\Temp\\cert.pem $certString;"+
+					"$expectedCert=[System.Security.Cryptography.X509Certificates.X509Certificate2]::new(\\\"C:\\Temp\\cert.pem\\\");"+


I also tried with passing the cert byte data directly to the X509Certificate2 constructor and, interestingly enough, reading from a file was consistently speedier.

jrvaldes

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

jrvaldes · 2023-09-27T16:09:13Z

hack/run-ci-e2e-test.sh

@@ -160,7 +160,7 @@ if [[ "$TEST" = "all" || "$TEST" = "basic" ]]; then
  printf "\n####### Testing service reconciliation #######\n" >> "$ARTIFACT_DIR"/wmco.log
  go test ./test/e2e/... -run=TestWMCO/service_reconciliation -v -timeout=20m -args $GO_TEST_ARGS
  printf "\n####### Testing cluster-wide proxy #######\n" >> "$ARTIFACT_DIR"/wmco.log
-  go test ./test/e2e/... -run=TestWMCO/cluster-wide_proxy -v -timeout=10m -args $GO_TEST_ARGS
+  go test ./test/e2e/... -run=TestWMCO/cluster-wide_proxy -v -timeout=45m -args $GO_TEST_ARGS


This is a 350% timeout increase. Did you run this with a dev cluster? Wondering what is the average run time?

Do you know what part of the test is taking a long time to run?

average runtime is 25-30min, cert test is the one that takes a long time as CNO injects 100+ certs into the proxy certs CM. Checking each one on each Win node takes a long while

test/e2e/proxy_test.go

aravindhp · 2023-09-27T16:34:20Z

hack/run-ci-e2e-test.sh

@@ -160,7 +160,7 @@ if [[ "$TEST" = "all" || "$TEST" = "basic" ]]; then
  printf "\n####### Testing service reconciliation #######\n" >> "$ARTIFACT_DIR"/wmco.log
  go test ./test/e2e/... -run=TestWMCO/service_reconciliation -v -timeout=20m -args $GO_TEST_ARGS
  printf "\n####### Testing cluster-wide proxy #######\n" >> "$ARTIFACT_DIR"/wmco.log
-  go test ./test/e2e/... -run=TestWMCO/cluster-wide_proxy -v -timeout=10m -args $GO_TEST_ARGS
+  go test ./test/e2e/... -run=TestWMCO/cluster-wide_proxy -v -timeout=45m -args $GO_TEST_ARGS


Do you know what part of the test is taking a long time to run?

test/e2e/proxy_test.go

openshift-ci-robot · 2023-09-28T20:40:08Z

@saifshaikh48: This pull request references Jira Issue OCPBUGS-19716, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @rrasouli

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

In response to this:

Proxy tests were not actually running due to an issue with the proxy enabled check.
When enabled, a few of the tests were failing so one was patched and the other was
disabled with the fix coming in another PR (#1800).

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 · 2023-09-28T20:55:39Z

/test vsphere-proxy-e2e-operator

Green locally 🚀

aravindhp

[proxy_test] Disable env var removal test

Lets be consistent with commit naming. [tests] Disable proxy env var removal

test/e2e/main_test.go

test/e2e/proxy_test.go

This test is flaking consistently after we fix the way we check for proxy enabled in the e2e tests. This test has been temporarily disabled until we address it (timing issue).

sebsoto · 2023-09-29T19:22:25Z

test/e2e/main_test.go

+	// proxyEnabled indicates whether a cluster-wide proxy is in use in the test environment
+	proxyEnabled bool


Why should this be added to the testContext struct?
Based on the way this is used I suggest just calling testContext.client.IsProxyEnabled() instead.
Caching it in this way doesn't seem useful enough to justify, and could cause problems if the proxy status is changed after testContext is initialized.

sebsoto · 2023-09-29T19:27:58Z

test/e2e/clusterinfo/openshift.go

@@ -93,3 +93,12 @@ func (o *OpenShift) GetInfrastructure() (*config.Infrastructure, error) {
 	}
 	return infra, nil
 }
+
+// IsProxyEnabled queries the Proxy resource to see if a cluster-wide proxy is enabled in this environment
+func (o *OpenShift) IsProxyEnabled() (bool, error) {


nit: consider renaming this to ProxyEnabled, just personal preference as it seems more natural to me:

if o.ProxyEnabled{} vs if o.IsProxyEnabled{}

sebsoto · 2023-09-29T19:33:50Z

test/e2e/proxy_test.go

+	var patches []*patch.JSONPatch
+	patches = append(patches, patch.NewJSONPatch("replace", "/spec/trustedCA/name", userCABundleName))


patches := []*patch.JSONPatch{patch.NewJSONPatch("replace", "/spec/trustedCA/name", userCABundleName)}

sebsoto · 2023-09-29T19:46:49Z

test/e2e/proxy_test.go

-					"Where-Object {$_.Subject -eq '%s'}).Count", string(subjectBytes))
-				out, err := tc.runPowerShellSSHJob(fmt.Sprintf("get-cert-%d", i), command, addr)
+			// Read in one cert at a time and test it exists in the Windows instance's system store
+			scanner := bufio.NewScanner(strings.NewReader(trustedCABundle))


Have you looked at https://pkg.go.dev/encoding/pem#Decode?
It looks like this functionality might be built in.

for block, rest := pem.Decode(data); block != nil; block, rest = pem.Decode(rest) { if block.Type == "CERTIFICATE"{

Bringing this up because this code + splitAtPEMCert() seems to be duplicated from the WICD code. Keeping things simpler here and using stdlib functions might make things easier to maintain.

Not sure if decoding the actual data causes a problem for this. Re-encoding the individual blocks with pem.Encode before doing the comparison is always an option :)

Good point, I initially was using pem.Decode in the source code but had to move away from that approach to avoid reading an arbitrary number of certs into memory. I don't see any issue using it in the test code (where we are controlling the # of certs)

This commit updates the way we are retrieving the proxy object in test cluster. The previous check cluster.isProxyEnabled() looks at the env vars in the environment from where it is called. The test pod doesn't run on the e2e cluster where proxy is enabled and WMCO is running. It runs in an ephemeral namespace on the CI cluster where the check will always return false. Co-authored-by: Mansi Kulkarni <mankulka@redhat.com>

In proxied clusters, this commit configures the proxy to use user-provided additional certs by creating a ConfigMap (that CNO merges with other proxy certs it deems necessary).

This commit fixes the proxy certificate import test. Ensuring certs where present in the Windows store by comparing subjects was not working properly since Windows and Golang extract the subject from the cert data differently, in incompatible formats. Instead, we now compare expected and actual certificates directly through powershell. It moves the cert validation test before the environment variable tests occur since the env var removal test removes the cluster-wide proxy/makes WMCO delete the TrustedCAConfigMap which the cert test relies on. This commit also increases the time limit for running proxy test suite to avoid timeouts now that the fixed tests run.

aravindhp

LGTM from me. I will let @sebsoto give the approval given he had requested changes.

test/e2e/proxy_test.go

saifshaikh48 · 2023-10-02T20:44:50Z

/test vsphere-proxy-e2e-operator

openshift-merge-robot · 2023-10-02T23:34:05Z

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

mansikulkarni96 · 2023-10-03T15:41:43Z

/lgtm

openshift-ci · 2023-10-03T16:09:15Z

[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 · 2023-10-03T16:19:53Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-10-03T16:19:55Z

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

openshift-ci · 2023-10-03T19:17:41Z

@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 · 2023-10-03T19:21:19Z

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

openshift/windows-machine-config-operator#1823

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

In response to this:

Proxy tests were not actually running due to an issue with the proxy enabled check.
When enabled, a few of the tests were failing so one was patched and the other was
disabled with the fix coming in another PR (#1800).

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 · 2023-10-03T19:22:05Z

@saifshaikh48: new pull request created: #1860

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.

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 Sep 22, 2023

saifshaikh48 changed the title ~~Fix proxy tests being skipped~~ WIP: Fix proxy tests being skipped Sep 22, 2023

saifshaikh48 changed the title ~~WIP: Fix proxy tests being skipped~~ [WIP] OCPBUGS-19716: Enable proxy cert test to run in CI Sep 26, 2023

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Sep 26, 2023

openshift-ci bot requested a review from rrasouli September 26, 2023 18:42

saifshaikh48 force-pushed the fixproxycerttest2 branch 3 times, most recently from b307bcd to 0dc1a99 Compare September 26, 2023 18:56

saifshaikh48 force-pushed the fixproxycerttest2 branch 2 times, most recently from 2cb0f8d to d145394 Compare September 27, 2023 14:06

saifshaikh48 changed the title ~~[WIP] OCPBUGS-19716: Enable proxy cert test to run in CI~~ OCPBUGS-19716: Enable proxy cert test to run in CI Sep 27, 2023

saifshaikh48 commented Sep 27, 2023

View reviewed changes

jrvaldes reviewed Sep 27, 2023

View reviewed changes

aravindhp suggested changes Sep 27, 2023

View reviewed changes

saifshaikh48 force-pushed the fixproxycerttest2 branch 6 times, most recently from 1ae9961 to f53040d Compare September 28, 2023 20:14

aravindhp suggested changes Sep 29, 2023

View reviewed changes

test/e2e/main_test.go Outdated Show resolved Hide resolved

test/e2e/proxy_test.go Show resolved Hide resolved

[e2e] Disable proxy env var removal test

c48d893

This test is flaking consistently after we fix the way we check for proxy enabled in the e2e tests. This test has been temporarily disabled until we address it (timing issue).

saifshaikh48 force-pushed the fixproxycerttest2 branch from f53040d to c19ff74 Compare September 29, 2023 15:38

sebsoto requested changes Sep 29, 2023

View reviewed changes

saifshaikh48 force-pushed the fixproxycerttest2 branch from c19ff74 to 53895f8 Compare October 2, 2023 17:41

saifshaikh48 and others added 3 commits October 2, 2023 13:44

[e2e] Use user-provided additional certs

95a70fd

In proxied clusters, this commit configures the proxy to use user-provided additional certs by creating a ConfigMap (that CNO merges with other proxy certs it deems necessary).

saifshaikh48 force-pushed the fixproxycerttest2 branch from 53895f8 to 3bb78e6 Compare October 2, 2023 17:44

aravindhp reviewed Oct 2, 2023

View reviewed changes

test/e2e/proxy_test.go Show resolved Hide resolved

openshift-ci bot assigned mansikulkarni96 Oct 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2023

sebsoto approved these changes Oct 3, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

aravindhp marked this pull request as ready for review October 3, 2023 16:13

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 Oct 3, 2023

openshift-ci bot requested review from mansikulkarni96 and sebsoto October 3, 2023 16:14

openshift-ci bot merged commit 6d7b8c0 into openshift:master Oct 3, 2023
15 checks passed

openshift-cherrypick-robot mentioned this pull request Oct 3, 2023

[release-4.14] OCPBUGS-20054: Enable proxy cert test to run in CI #1860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-19716: Enable proxy cert test to run in CI #1823

OCPBUGS-19716: Enable proxy cert test to run in CI #1823

saifshaikh48 commented Sep 22, 2023 •

edited

openshift-ci bot commented Sep 22, 2023

saifshaikh48 commented Sep 22, 2023

openshift-ci-robot commented Sep 26, 2023

saifshaikh48 commented Sep 26, 2023

saifshaikh48 commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

saifshaikh48 Sep 27, 2023

jrvaldes left a comment

jrvaldes Sep 27, 2023

aravindhp Sep 27, 2023

saifshaikh48 Sep 27, 2023

aravindhp Sep 27, 2023

openshift-ci-robot commented Sep 28, 2023

saifshaikh48 commented Sep 28, 2023

aravindhp left a comment

sebsoto Sep 29, 2023

sebsoto Sep 29, 2023

sebsoto Sep 29, 2023

sebsoto Sep 29, 2023

saifshaikh48 Oct 2, 2023

aravindhp left a comment

saifshaikh48 commented Oct 2, 2023

openshift-merge-robot commented Oct 2, 2023

mansikulkarni96 commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

saifshaikh48 commented Oct 3, 2023

openshift-cherrypick-robot commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

openshift-ci-robot commented Oct 3, 2023

openshift-cherrypick-robot commented Oct 3, 2023

		// proxyEnabled indicates whether a cluster-wide proxy is in use in the test environment
		proxyEnabled bool

		var patches []*patch.JSONPatch
		patches = append(patches, patch.NewJSONPatch("replace", "/spec/trustedCA/name", userCABundleName))

OCPBUGS-19716: Enable proxy cert test to run in CI #1823

OCPBUGS-19716: Enable proxy cert test to run in CI #1823

Conversation

saifshaikh48 commented Sep 22, 2023 • edited

openshift-ci bot commented Sep 22, 2023

saifshaikh48 commented Sep 22, 2023

openshift-ci-robot commented Sep 26, 2023

saifshaikh48 commented Sep 26, 2023

saifshaikh48 commented Sep 27, 2023

openshift-ci-robot commented Sep 27, 2023

Choose a reason for hiding this comment

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

openshift-ci-robot commented Sep 28, 2023

saifshaikh48 commented Sep 28, 2023

aravindhp 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

aravindhp left a comment

Choose a reason for hiding this comment

saifshaikh48 commented Oct 2, 2023

openshift-merge-robot commented Oct 2, 2023

mansikulkarni96 commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

saifshaikh48 commented Oct 3, 2023

openshift-cherrypick-robot commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

openshift-ci-robot commented Oct 3, 2023

openshift-cherrypick-robot commented Oct 3, 2023

saifshaikh48 commented Sep 22, 2023 •

edited