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 |
WalkthroughA new Ginkgo test file is added under ChangesTLSAdherence Integration Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 a valid spec.tlsAdherence value on apiservers/cluster [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| apiServer, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get apiservers/cluster") | ||
|
|
||
| validValues := []configv1.TLSAdherencePolicy{ | ||
| configv1.TLSAdherencePolicyNoOpinion, | ||
| configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly, | ||
| configv1.TLSAdherencePolicyStrictAllComponents, | ||
| } | ||
| o.Expect(apiServer.Spec.TLSAdherence).To( | ||
| o.BeElementOf(validValues), | ||
| "apiservers/cluster spec.tlsAdherence must be one of the defined TLSAdherencePolicy values") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TLSAdherence field type/default semantics and whether empty value is a valid persisted state.
set -euo pipefail
echo "== Locate APIServerSpec TLSAdherence definition =="
rg -n --type=go -C4 '\bTLSAdherence\b'
echo
echo "== Look for defaulting markers or comments tied to TLSAdherence =="
rg -n --type=go -C3 'default|omitempty|TLSAdherencePolicyNoOpinion|tlsAdherence'
echo
echo "== Check if existing tests treat empty/unset tlsAdherence as valid =="
rg -n --type=go -C3 'spec\.tlsAdherence|TLSAdherencePolicy|BeElementOf'Repository: openshift/origin
Length of output: 50375
Add a guard to check if spec.tlsAdherence is set before asserting it matches a valid value.
Line 129 asserts apiServer.Spec.TLSAdherence is one of three explicit enum values, but the field is optional (marked omitempty) and can be unset. The test comment on line 119 says "when set," acknowledging that the field may not always be populated. If the field is unset on the cluster, the BeElementOf assertion will fail even though the state is valid. Guard the assertion with a check for whether the field is set, or include the zero/unset value in the valid values list.
🤖 Prompt for 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.
In `@test/extended/apiserver/tls_adherence.go` around lines 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.
| if isMicroShift || isHyperShift { | ||
| g.Skip("TLSAdherence is not applicable to MicroShift or HyperShift clusters") | ||
| } |
There was a problem hiding this comment.
This is not true, right? I think it is applicable in both, though it may make sense to have a separate test for Hypershift since it has a split apiserver and a HyperShift-specific configuration API for configuring the TLS profile of the hosted cluster.
There was a problem hiding this comment.
oh sorry yes no this is not true. removing.
| 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: |
| // Test 2 – verify the API server accepts and reflects StrictAllComponents via dry-run. | ||
| g.It("[FeatureGate:TLSAdherence] should accept and reflect spec.tlsAdherence StrictAllComponents on apiservers/cluster [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| current, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get apiservers/cluster") | ||
|
|
||
| desired := current.DeepCopy() | ||
| desired.Spec.TLSAdherence = configv1.TLSAdherencePolicyStrictAllComponents | ||
|
|
||
| result, err := oc.AdminConfigClient().ConfigV1().APIServers().Update(ctx, desired, metav1.UpdateOptions{ | ||
| DryRun: []string{metav1.DryRunAll}, | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), | ||
| "apiservers/cluster should accept spec.tlsAdherence=StrictAllComponents") | ||
| o.Expect(result.Spec.TLSAdherence).To( | ||
| o.Equal(configv1.TLSAdherencePolicyStrictAllComponents), | ||
| "apiservers/cluster should reflect spec.tlsAdherence=StrictAllComponents") | ||
| }) | ||
|
|
||
| // Test 3 – verify the API server accepts and reflects LegacyAdheringComponentsOnly via dry-run. | ||
| g.It("[FeatureGate:TLSAdherence] should accept and reflect spec.tlsAdherence LegacyAdheringComponentsOnly on apiservers/cluster [apigroup:config.openshift.io]", func(ctx context.Context) { | ||
| current, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get apiservers/cluster") | ||
|
|
||
| desired := current.DeepCopy() | ||
| desired.Spec.TLSAdherence = configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly | ||
|
|
||
| result, err := oc.AdminConfigClient().ConfigV1().APIServers().Update(ctx, desired, metav1.UpdateOptions{ | ||
| DryRun: []string{metav1.DryRunAll}, | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), | ||
| "apiservers/cluster should accept spec.tlsAdherence=LegacyAdheringComponentsOnly") | ||
| o.Expect(result.Spec.TLSAdherence).To( | ||
| o.Equal(configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly), | ||
| "apiservers/cluster should reflect spec.tlsAdherence=LegacyAdheringComponentsOnly") | ||
| }) |
There was a problem hiding this comment.
I think it's a stretch to make these two separate tests. But I think a single test that verifies that all valid values can be successfully dry-run updated is a reasonable single test to run.
There was a problem hiding this comment.
Is it possible to test to make sure this can only be set by cluster-admins? Is there an RBAC in place to test?
|
@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