OCPBUGS-29894: Add test verifying that routers without the required CRLs are marked not ready #1053

rfredette · 2024-05-13T19:17:16Z

The new test should fail until openshift/router#595 is merged.
This is part of the fix for OCPBUGS-29894

openshift-ci-robot · 2024-05-13T19:17:23Z

@rfredette: This pull request references Jira Issue OCPBUGS-29894, 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.16.0) matches configured target version for branch (4.16.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

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

In response to this:

This is part of the fix for OCPBUGS-29894

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-robot · 2024-05-13T19:20:35Z

@rfredette: This pull request references Jira Issue OCPBUGS-29894, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @lihongan

In response to this:

The new test should fail until openshift/router#595 is merged.
This is part of the fix for OCPBUGS-29894

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.

Miciah · 2024-06-05T15:22:58Z

/assign

openshift-bot · 2024-09-04T01:00:37Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

lihongan · 2024-09-04T02:01:42Z

/remove-lifecycle stale

openshift-ci · 2024-09-16T18:16:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from miciah. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Miciah · 2024-09-16T21:24:57Z

As noted in openshift/router#595 (review), I think we need an alert. Would it make sense to include that in this PR?

test/e2e/client_tls_test.go

Miciah · 2024-09-16T22:04:39Z

test/e2e/client_tls_test.go

+				Namespace: namespaceName,
+			}
+			// When generating certificates, the CRL distribution points need to be specified by URL
+			crlHostServiceName := names.SimpleNameGenerator.GenerateName("crl-host-service-")


Is it useful to use a new generated name here, as opposed to using crlHostName.Name + "-service" or even just crlHostName.Name? Similarly, I think crlConfigMapName below could be generated as fmt.Sprintf("%s-%s", crlHostName.Name, name), and clientCAConfigmapName and icName could use the same prefix. I imagine tracing the test or diagnosing failures would be easier if it used the same prefix when naming resources related to the same test case.

Then there are clientCertsConfigmap, podName, echoPod, etc., but maybe it makes more sense for some of those to have distinct prefixes. That is, the echo pod, echo route, and echo service can (and do) share a prefix, but a different prefix from crlConfigMapName etc.

If I'm missing something or you find the current approach easier to follow, feel free to ignore this suggestion.

I took the brute force approach to make sure there were no name conflicts when the subtests are run in parallel, but I agree, it'd be better be able to identify which resources are a part of the same subtest.

Miciah · 2024-09-16T22:08:35Z

test/e2e/client_tls_test.go

 			// Generate CRLs. Offset the expiration times by 1 minute each so that we can verify that only the correct CRLs get updated at each expiration.
 			currentCRLs := map[string]*x509.RevocationList{}
 			crlPems := map[string]string{}
 			caBundle := []string{}
 			validTime := 3 * time.Minute
-			//expirations := []time.Time{}


Debug code?

test/e2e/client_tls_test.go

Miciah · 2024-09-16T22:20:40Z

test/e2e/client_tls_test.go

+	for i := range testCases {
+		tc := testCases[i]


What's the point of this change? Did you mean to use tc := &testCases[i] to avoid copying the whole test case? Even that would be an unnecessary micro-optimization for test code, in my opinion, but I don't understand the purpose of writing for i := range testCases / tc := testCases[i] instead of keeping the original for _, tc := range testCases at all.

for _, tc := range testCases isn't thread safe. I initially had it that way, but once t.Parallel() was called, tc in all threads converged on the last test case in the list.

test/e2e/client_tls_test.go

test/e2e/util_test.go

test/e2e/client_tls_test.go

Modify TestMTLSWithCRLs and TestCRLUpdate so that the subtests of each test can be run in parallel.

TestRouterWaitsForCRLs verifies that the router reports not ready until all required CRLs are downloaded This is part of the fix for OCPBUGS-29894

openshift-ci · 2024-09-24T05:45:05Z

@rfredette: The following tests 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/e2e-azure-operator	`258b656`	link	true	`/test e2e-azure-operator`
ci/prow/e2e-gcp-operator	`258b656`	link	true	`/test e2e-gcp-operator`
ci/prow/e2e-aws-operator-techpreview	`258b656`	link	false	`/test e2e-aws-operator-techpreview`
ci/prow/e2e-aws-ovn-single-node	`258b656`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-hypershift	`258b656`	link	true	`/test e2e-hypershift`
ci/prow/e2e-aws-operator	`258b656`	link	true	`/test e2e-aws-operator`

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels May 13, 2024

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label May 13, 2024

openshift-ci bot requested review from lihongan, alebedev87 and Miciah May 13, 2024 19:17

rfredette force-pushed the ocpbugs-29894-crl-tests branch 2 times, most recently from 07a0f76 to e12ad7b Compare June 5, 2024 02:42

openshift-ci bot assigned Miciah Jun 5, 2024

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 4, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 4, 2024

rfredette force-pushed the ocpbugs-29894-crl-tests branch from e12ad7b to 8e39bd5 Compare September 16, 2024 18:15

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2024