Add E2E test for the TLSAdherence openshift/api field#31309
Add E2E test for the TLSAdherence openshift/api field#31309richardsonnick wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughNew extended apiserver tests add TLSAdherence feature-gate gating, validate ChangesTLSAdherence Integration Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: richardsonnick 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/apiserver/tls_adherence.go`:
- Around line 120-131: The test assertion in the It block does not account for
the optional spec.tlsAdherence field being unset. Since the field is marked as
optional with omitempty and the test comment acknowledges "when set", the
apiServer.Spec.TLSAdherence value may be nil, causing the BeElementOf assertion
to fail even when the state is valid. Either guard the assertion with a nil
check that conditionally validates only when the field is set, or add the
nil/zero value to the validValues slice so that an unset field is considered a
valid state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 328d6dfb-fe5e-4dfc-a5f3-811d6c04d0e2
📒 Files selected for processing (1)
test/extended/apiserver/tls_adherence.go
| g.It("[FeatureGate:TLSAdherence] should have TLSAdherence listed as enabled in featuregate/cluster status [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| fg, err := oc.AdminConfigClient().ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get featuregate/cluster") | ||
|
|
||
| found := false | ||
| for _, featureGateValues := range fg.Status.FeatureGates { | ||
| for _, enabledGate := range featureGateValues.Enabled { | ||
| if enabledGate.Name == tlsAdherenceFeatureGateName { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if found { | ||
| break | ||
| } | ||
| } | ||
| o.Expect(found).To(o.BeTrue(), | ||
| "TLSAdherence must appear in featuregate/cluster .status.featureGates[].enabled[].name") | ||
| }) |
There was a problem hiding this comment.
Can't hurt to verify this, but I don't think this should count toward the 5 tests, the intent of which are more functional rather than verifying assumptions.
| // Test 5 – verify that no cluster operator is degraded when the TLSAdherence feature gate is active. | ||
| g.It("[FeatureGate:TLSAdherence] should not have any degraded cluster operators [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| coList, err := oc.AdminConfigClient().ConfigV1().ClusterOperators().List(ctx, metav1.ListOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to list clusteroperators") | ||
|
|
||
| for _, co := range coList.Items { | ||
| for _, condition := range co.Status.Conditions { | ||
| if condition.Type == configv1.OperatorDegraded && condition.Status == configv1.ConditionTrue { | ||
| g.Fail(fmt.Sprintf("cluster operator %q is degraded: %s: %s", | ||
| co.Name, condition.Reason, condition.Message)) | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
I think there is a different kind of test that we can implement that essentially continuously monitors the cluster to verify an invariant is never violated. The way this test is implemented, you'll only get a point in time verification, which would likely result in false negatives.
|
Scheduling required tests: |
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
Tests that apiservers/cluster spec.tlsAdherence: - rejects invalid values (422 Invalid via dry-run) - accepts and reflects all valid values (dry-run) - is only writable by cluster-admins (SubjectAccessReview) - does not leave cluster operators degraded Also labels the existing TestTLSMinimumVersions and TestTLSDefaults wire-level TLS tests with [FeatureGate:TLSAdherence] so they count toward the openshift/api verify-feature-promotion Sippy check. Co-authored-by: Cursor <cursoragent@cursor.com>
richardsonnick
force-pushed
the
tls-adherence-e2e
branch
from
e65c04c to
14d023a
Compare
|
Scheduling required tests: |
|
@richardsonnick: The following tests failed, say
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. |
3 participants
Adds E2E tests for the TLSAdherence openshift/api field. This is a requirement for GA.
Summary by CodeRabbit
spec.tlsAdherencevalidationspec.tlsAdherencevalues and acceptsStrictAllComponentsandLegacyAdheringComponentsOnlyOperatorDegraded=Trueduring updates