CM-1223: Add extensive e2e automation for TLS profile test - #449
CM-1223: Add extensive e2e automation for TLS profile test#449chiragkyal wants to merge 1 commit into
Conversation
Signed-off-by: chiragkyal <ckyal@redhat.com>
WalkthroughThe TLS profile e2e test now saves and restores cluster apiserver TLS state, covers legacy and strict adherence paths across cert-manager operands, checks webhook override precedence, and verifies live transitions from Intermediate to Modern. ChangesTLS profile e2e coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@chiragkyal: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandkuma77, chiragkyal The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-1223 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
|
/cc @bharath-b-rh @mytreya-rh |
| // ───────────────────────────────────────────────────────────────────────── | ||
| // Scenario 1: LegacyAdheringComponentsOnly — operands must be untouched | ||
| // ───────────────────────────────────────────────────────────────────────── | ||
| It("S1: should not inject TLS args when tlsAdherence is LegacyAdheringComponentsOnly", func() { |
There was a problem hiding this comment.
I was just comparing scenario 1 and scenario 6. And looks like S6 is a superset of S1.
So if S6 passes (no args injected with Modern + Legacy), S1 should always pass too.
S1 only adds the nil profile case, which does not exercise a different branch today.
I think even if we drop scenario 1 in this case, we should be good. Let me know what do you think.
There was a problem hiding this comment.
Thanks for the review. I see the logic, but I think S1 is worth keeping for a few reasons:
They test different API-level guarantees, not just different code paths.
S1 asserts: "when no TLS profile has ever been set, cert-manager operands are untouched." This is the day-0 / fresh-install state of any cluster.
S6 asserts: "when a profile is explicitly set to Modern but adherence is Legacy, operands are still untouched."
Both pass through the same early-return today, but they protect against independently plausible regression e.g:
- A future change that adds "apply a default profile when tlsSecurityProfile is nil" — this would break S1 but leave S6 green, because S6 has an explicit non-nil profile.
There is also a subtle documentation value: S1 makes it explicit that cert-manager's TLS hook is opt-in — zero configuration means zero injection. S6 alone doesn't communicate that; it looks like the adherence policy is the only gate, whereas S1 shows the nil profile is also a valid no-op state.
Also since it's the first test in the Ordered suite, it also serves as a clean-slate baseline before all the mutation tests that follow. Happy to discuss further, but I'd prefer to keep both.
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
All the tests added here are getting skipped. Please check https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cert-manager-operator/449/pull-ci-openshift-cert-manager-operator-master-e2e-operator-tech-preview/2070425619958599680/artifacts/e2e-operator-tech-preview/test/artifacts/junit.xml for the reason.
| return restoreClusterAPIServerTLSConfig(ctx, original) | ||
| }, lowTimeout, fastPollInterval).Should(Succeed()) | ||
|
|
||
| By("clearing any unsupportedConfigOverrides left by scenario 8") |
There was a problem hiding this comment.
| By("clearing any unsupportedConfigOverrides left by scenario 8") | |
| By("clearing unsupportedConfigOverrides left by any test scenarios") |
| updated := cm.DeepCopy() | ||
| if args == nil { | ||
| // Clear the whole unsupportedConfigOverrides field | ||
| updated.Spec.OperatorSpec.UnsupportedConfigOverrides = runtime.RawExtension{} |
There was a problem hiding this comment.
nit: This method is specifically for webhook, but cleanup is done for all components.
|
|
||
| By("verifying all operands match the Modern profile") | ||
| expectedSpec, err := tlsprofile.EffectiveSpec(tlsProfileModernProfile) | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Could we add some message here to make debugging easier. Same at other places too.
I see the issue. The techpreview jobs are running in a HyperShift Cluster, where the So to run these tests need would need a standalone OCP cluster with @bharath-b-rh, should we migrate the job to a standalone OCP instead of HyperShift, or do we have some alternative approach here? |
5 participants
Summary
The cert-manager operator propagates the cluster-wide TLS security profile and adherence policy to cert-manager operand deployments. Existing e2e coverage only validated a single happy-path case, leaving profile types, adherence transitions, rollback, and override behavior untested.
Change
Rework the TLS profile e2e suite to add broad coverage across:
All TLS profile types (Modern, Intermediate, Old, Custom) under strict adherence
Legacy adherence behavior (no args injected)
Rollback from strict to legacy adherence
unsupportedConfigOverridesprecedence over cluster-enforced TLS settingsLive profile switching between profiles
Summary by CodeRabbit
New Features
Bug Fixes
Tests