Refactor TLS cases #31077
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces a single polymorphic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
the
approved
![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 the current code and only fix it if needed.
Inline comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 897-899: The test currently only checks for the presence of the
injectTLSAnnotation key
(o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation))) which allows a
"false" value to pass; update both places where that key is asserted (the call
sites using cm.Annotations and injectTLSAnnotation) to assert the annotation
value instead of just the key—for example replace the HaveKey check with an
assertion that cm.Annotations[injectTLSAnnotation] equals "true" (or use
HaveKeyWithValue(injectTLSAnnotation, "true")) and update the failure message
accordingly so the test fails when the annotation is present but set to "false".
🪄 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: edee0025-9055-4d34-b53f-b5422803ec72
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
|
/lgtm |
The "config.openshift.io/inject-tls" annotation key was duplicated 9 times across test functions. Extract it into a package-level constant to improve maintainability and reduce copy-paste errors.
gangwgr
force-pushed
the
refactor-tls-code
branch
from
f64a461 to
d9f2092
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/tls/tls_observed_config.go (1)
897-898:⚠️ Potential issue | 🟡 MinorStill unresolved: assert annotation value, not only key presence.
At Line 897 and Line 1594,
HaveKey(injectTLSAnnotation)still passes when the value is"false", which can mask injection regressions in profile-switch verification.Suggested fix
- o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), - fmt.Sprintf("ConfigMap %s/%s is missing %s annotation", cmNamespace, t.configMapName, injectTLSAnnotation)) + o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true"), + fmt.Sprintf("ConfigMap %s/%s must have %s=true", cmNamespace, t.configMapName, injectTLSAnnotation))- o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), - fmt.Sprintf("ConfigMap %s/%s is missing %s annotation", cmNamespace, t.configMapName, injectTLSAnnotation)) + o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true"), + fmt.Sprintf("ConfigMap %s/%s must have %s=true", cmNamespace, t.configMapName, injectTLSAnnotation))#!/bin/bash # Verify any remaining key-only checks for injectTLSAnnotation. # Expected after fix: no matches. rg -n -C2 'HaveKey\(injectTLSAnnotation\)' --type=goAlso applies to: 1594-1595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 897 - 898, Replace the key-only assertion that uses o.HaveKey(injectTLSAnnotation) on cm.Annotations with an assertion that checks the annotation value as well: use o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true")) (or o.Expect(cm.Annotations).To(o.And(o.HaveKey(injectTLSAnnotation), o.HaveKeyWithValue(injectTLSAnnotation, "true")))) so the test fails when the annotation exists but is "false"; update both occurrences that currently call o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), ...) to this value-aware check referencing cm.Annotations and injectTLSAnnotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 897-898: Replace the key-only assertion that uses
o.HaveKey(injectTLSAnnotation) on cm.Annotations with an assertion that checks
the annotation value as well: use
o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true")) (or
o.Expect(cm.Annotations).To(o.And(o.HaveKey(injectTLSAnnotation),
o.HaveKeyWithValue(injectTLSAnnotation, "true")))) so the test fails when the
annotation exists but is "false"; update both occurrences that currently call
o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), ...) to this
value-aware check referencing cm.Annotations and injectTLSAnnotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6cbc469d-4206-4546-8f21-4faae9d8e8e6
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/extended/tls/tls_observed_config.go (1)
695-697:⚠️ Potential issue | 🟡 MinorRequire
inject-tls=truein the switch verifiers.These assertions still pass when the annotation exists with value
"false", so the Modern/Custom profile checks can report success while injection is actually disabled. Please assert the value here as well.Also applies to: 1249-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 695 - 697, The current assertion only checks the presence of the injectTLSAnnotation key but not its value, allowing "false" to pass; update the switch verifiers (e.g., in the block using cm, configData := cm.Data[t.resolvedKey()], and the Expect call that references injectTLSAnnotation, and the duplicate at lines 1249-1251) to assert that cm.Annotations[injectTLSAnnotation] == "true" (or use the matcher that checks the annotation value equals "true") so the test fails when injection is explicitly disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 103-108: Restore a controlPlane flag on the
deploymentRolloutTarget struct and use it to filter out management-cluster-only
(control plane) deployments in guestSideDeploymentRolloutTargets: add a bool
field named controlPlane to deploymentRolloutTarget and update
guestSideDeploymentRolloutTargets to only return targets where controlPlane is
false (or explicitly exclude controlPlane==true); adjust any construction sites
of deploymentRolloutTarget to set the flag accordingly so existing callers
(including the HyperShift flow) regain the previous filtering behavior.
- Around line 688-694: The current loop over configMapTargets treats any error
from AdminKubeClient().CoreV1().ConfigMaps(...).Get as a "not found" skip;
change the error handling so you only continue when apierrors.IsNotFound(err) is
true and otherwise fail the test (or return the error) so
auth/transport/apiserver errors are surfaced; update the Get call handling in
the loop that uses configMapTargets and resolvedNamespace() (and its second
occurrence later in the file) to check apierrors.IsNotFound(err) before calling
e2e.Logf and continue, and for any other err call t.Fatalf/return the error (or
fail the test) so failures are not masked.
- Around line 153-163: The current wait logic iterates over clusterOperatorNames
even after computing the deduplicated seen set and the guest-specific guestCOs,
making the HyperShift guest/control-plane split a no-op; modify the waiting loop
to only wait on entries in guestCOs (or the filtered subset derived from seen)
instead of clusterOperatorNames and ensure the functions/variables
clusterOperatorNames, seen, and guestCOs are used to drive that filtered
iteration so operators not relevant to the guest side are not waited on (apply
same fix in the analogous block around lines 1703-1725).
---
Duplicate comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 695-697: The current assertion only checks the presence of the
injectTLSAnnotation key but not its value, allowing "false" to pass; update the
switch verifiers (e.g., in the block using cm, configData :=
cm.Data[t.resolvedKey()], and the Expect call that references
injectTLSAnnotation, and the duplicate at lines 1249-1251) to assert that
cm.Annotations[injectTLSAnnotation] == "true" (or use the matcher that checks
the annotation value equals "true") so the test fails when injection is
explicitly disabled.
🪄 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: 0f9c9dd8-3a02-44ee-9d2a-cb63dce96b83
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
| // deploymentRolloutTarget identifies a Deployment that must complete | ||
| // rollout after a TLS profile change. | ||
| type deploymentRolloutTarget struct { | ||
| namespace string | ||
| deploymentName string | ||
| } |
There was a problem hiding this comment.
Keep guest rollout targets filterable.
guestSideDeploymentRolloutTargets() now returns the full list because deploymentRolloutTarget no longer carries any controlPlane metadata. In the HyperShift flow that reintroduces waits on management-cluster-only deployments, which can turn the profile-switch tests into false timeouts on guest clusters.
Also applies to: 165-170, 215-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/tls/tls_observed_config.go` around lines 103 - 108, Restore a
controlPlane flag on the deploymentRolloutTarget struct and use it to filter out
management-cluster-only (control plane) deployments in
guestSideDeploymentRolloutTargets: add a bool field named controlPlane to
deploymentRolloutTarget and update guestSideDeploymentRolloutTargets to only
return targets where controlPlane is false (or explicitly exclude
controlPlane==true); adjust any construction sites of deploymentRolloutTarget to
set the flag accordingly so existing callers (including the HyperShift flow)
regain the previous filtering behavior.
| // clusterOperatorNames is the deduplicated list of ClusterOperator names. | ||
| var clusterOperatorNames = []string{ | ||
| "image-registry", | ||
| "openshift-controller-manager", | ||
| "kube-apiserver", | ||
| "openshift-apiserver", | ||
| "etcd", | ||
| "kube-controller-manager", | ||
| "kube-scheduler", | ||
| "openshift-samples", | ||
| } |
There was a problem hiding this comment.
The HyperShift ClusterOperator split is currently a no-op.
seen and guestCOs are populated, but the code still waits on every entry in clusterOperatorNames. That defeats the guest/control-plane split and can block on operators that should not be part of guest-side stabilization.
Also applies to: 1703-1725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/tls/tls_observed_config.go` around lines 153 - 163, The current
wait logic iterates over clusterOperatorNames even after computing the
deduplicated seen set and the guest-specific guestCOs, making the HyperShift
guest/control-plane split a no-op; modify the waiting loop to only wait on
entries in guestCOs (or the filtered subset derived from seen) instead of
clusterOperatorNames and ensure the functions/variables clusterOperatorNames,
seen, and guestCOs are used to drive that filtered iteration so operators not
relevant to the guest side are not waited on (apply same fix in the analogous
block around lines 1703-1725).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| } | ||
| configKey := t.configMapKey | ||
| if configKey == "" { | ||
| configKey = "config.yaml" |
There was a problem hiding this comment.
There should not be any defaulting for keys or for namespaces. It's safer to make it always explicit. Also, instead of de-duplicating this part of the code I suggest to first define the specific target types. It will remove the need for many checks and defaults. Also, specifying configMapContext is an artificial step that can be dropped by defining the specific target type.
gangwgr
force-pushed
the
refactor-tls-code
branch
from
6001380 to
ae5667f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/extended/tls/tls_observed_config.go (4)
704-705:⚠️ Potential issue | 🟡 MinorAssert
inject-tls=true, not just key presence.Checking only for the annotation key lets
"false"pass here, which can mask an injection regression during the profile-switch assertions.Suggested fix
- o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), - fmt.Sprintf("ConfigMap %s/%s is missing %s annotation", cmNamespace, t.configMapName, injectTLSAnnotation)) + o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true"), + fmt.Sprintf("ConfigMap %s/%s must have %s=true", cmNamespace, t.configMapName, injectTLSAnnotation))Also applies to: 1258-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 704 - 705, The test currently only asserts the presence of the injectTLSAnnotation key in cm.Annotations (using o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation)...)), which allows a value of "false" to pass; change the assertion to verify the annotation value is exactly "true" instead (e.g. assert cm.Annotations[injectTLSAnnotation] == "true" or use a Gomega matcher like HaveKeyWithValue) so injections are correctly validated; apply the same fix to the other occurrence referenced around the profile-switch assertion (injectTLSAnnotation check at the later lines).
106-109:⚠️ Potential issue | 🟠 MajorRestore control-plane metadata on rollout targets.
deploymentRolloutTargetno longer carries enough information to exclude management-cluster-only deployments, soguestSideDeploymentRolloutTargets()now returns the full list. In the HyperShift flow that reintroduces waits on non-guest rollouts and can turn the profile-switch tests into false timeouts.Suggested direction
type deploymentRolloutTarget struct { namespace string deploymentName string + controlPlane bool } func guestSideDeploymentRolloutTargets() []deploymentRolloutTarget { var result []deploymentRolloutTarget for _, t := range deploymentRolloutTargets { - result = append(result, t) + if !t.controlPlane { + result = append(result, t) + } } return result }Also mark the management-cluster deployments accordingly in
deploymentRolloutTargets.Also applies to: 216-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 106 - 109, The struct deploymentRolloutTarget currently only holds namespace and deploymentName which loses control-plane/management-cluster metadata; update deploymentRolloutTarget to include a boolean or enum (e.g., isManagement bool or controlPlaneRole string) and populate that field in deploymentRolloutTargets() so management-cluster deployments are marked, and change guestSideDeploymentRolloutTargets() to filter based on that new field (returning only guest targets) rather than assuming full list; ensure all call sites that construct or consume deploymentRolloutTarget in tls_observed_config.go and related functions handle the new field.
1711-1733:⚠️ Potential issue | 🟠 MajorUse the filtered guest ClusterOperator list here.
This split is still effectively a no-op:
seenandguestCOsare populated, but the wait loop still iterates every entry inclusterOperatorNames. That makes the HyperShift guest-side stabilization path block on management-only operators again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 1711 - 1733, The loop currently iterates all clusterOperatorNames but should only wait on guest-visible operators; change the for loop that calls waitForClusterOperatorStable to skip entries not present in the seen or guestCOs maps (use the existing seen and guestCOs lookups) so only guest-side ClusterOperators are waited on (keep references to clusterOperatorNames, seen, guestCOs, profileLabel, and waitForClusterOperatorStable).
696-701:⚠️ Potential issue | 🟠 MajorOnly skip on
NotFoundwhen reading ConfigMaps.Both paths currently
continueon anyConfigMaps(...).Geterror and log it as “not found”. That masks auth, transport, and apiserver failures as passing skips and can hide real regressions.Safer error handling
cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps(cmNamespace).Get(ctx, t.configMapName, metav1.GetOptions{}) - if err != nil { + if apierrors.IsNotFound(err) { e2e.Logf("SKIP: ConfigMap %s/%s not found: %v", cmNamespace, t.configMapName, err) continue } + o.Expect(err).NotTo(o.HaveOccurred(), + fmt.Sprintf("failed to get ConfigMap %s/%s", cmNamespace, t.configMapName))Also applies to: 1252-1255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 696 - 701, The loop that calls oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get currently treats all errors as "not found" and continues; change the error handling in the block that calls ConfigMaps(...).Get (and the duplicate at the other occurrence around lines 1252-1255) to check metav1.IsNotFound(err) and only log/continue for NotFound errors, while for any other error return/fail the test or propagate the error (e.g., t.Fatalf/t.Fatalf-like behavior or return the error) so auth/transport/apiserver failures aren’t masked; keep the existing e2e.Logf message for the NotFound case and use a clear error log/failed assertion for other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 945-947: Before mutating shared ConfigMap state capture the
original annotation/data and register a cleanup to restore it using
g.DeferCleanup: read and store the original cm.Annotations (and any other fields
you will change), call g.DeferCleanup with a closure that sets the saved
annotations back and calls cmCtx.update to restore, and only then perform the
delete and cmCtx.update; apply the same pattern for the other mutation sites
referenced (around the uses of injectTLSAnnotation, cm.Annotations, and
cmCtx.update at the other ranges). Ensure the closure uses the saved snapshot
(not a pointer to the mutated object) so restoration reliably re-applies the
original state.
---
Duplicate comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 704-705: The test currently only asserts the presence of the
injectTLSAnnotation key in cm.Annotations (using
o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation)...)), which allows a
value of "false" to pass; change the assertion to verify the annotation value is
exactly "true" instead (e.g. assert cm.Annotations[injectTLSAnnotation] ==
"true" or use a Gomega matcher like HaveKeyWithValue) so injections are
correctly validated; apply the same fix to the other occurrence referenced
around the profile-switch assertion (injectTLSAnnotation check at the later
lines).
- Around line 106-109: The struct deploymentRolloutTarget currently only holds
namespace and deploymentName which loses control-plane/management-cluster
metadata; update deploymentRolloutTarget to include a boolean or enum (e.g.,
isManagement bool or controlPlaneRole string) and populate that field in
deploymentRolloutTargets() so management-cluster deployments are marked, and
change guestSideDeploymentRolloutTargets() to filter based on that new field
(returning only guest targets) rather than assuming full list; ensure all call
sites that construct or consume deploymentRolloutTarget in
tls_observed_config.go and related functions handle the new field.
- Around line 1711-1733: The loop currently iterates all clusterOperatorNames
but should only wait on guest-visible operators; change the for loop that calls
waitForClusterOperatorStable to skip entries not present in the seen or guestCOs
maps (use the existing seen and guestCOs lookups) so only guest-side
ClusterOperators are waited on (keep references to clusterOperatorNames, seen,
guestCOs, profileLabel, and waitForClusterOperatorStable).
- Around line 696-701: The loop that calls
oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get currently treats all errors as
"not found" and continues; change the error handling in the block that calls
ConfigMaps(...).Get (and the duplicate at the other occurrence around lines
1252-1255) to check metav1.IsNotFound(err) and only log/continue for NotFound
errors, while for any other error return/fail the test or propagate the error
(e.g., t.Fatalf/t.Fatalf-like behavior or return the error) so
auth/transport/apiserver failures aren’t masked; keep the existing e2e.Logf
message for the NotFound case and use a clear error log/failed assertion for
other errors.
🪄 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: c89a4d07-9277-4cbd-818e-5fdc9be9f061
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
gangwgr
force-pushed
the
refactor-tls-code
branch
from
ae5667f to
75e6f83
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/tls/tls_observed_config.go (1)
696-705:⚠️ Potential issue | 🟡 MinorError handling masks non-"not found" failures; annotation check is inconsistent.
Error handling (lines 699-701): Any error is logged as "not found" and skipped. Auth, transport, or API server errors will be masked. Only
apierrors.IsNotFound(err)should trigger a skip.Annotation check (line 704): Uses
HaveKeybuttestConfigMapTLSInjectionat line 816 correctly uses value comparison. For consistency and to catch"false"values:Suggested fix
cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps(cmNamespace).Get(configChangeCtx, t.configMapName, metav1.GetOptions{}) - if err != nil { - e2e.Logf("SKIP: ConfigMap %s/%s not found: %v", cmNamespace, t.configMapName, err) - continue + if apierrors.IsNotFound(err) { + e2e.Logf("SKIP: ConfigMap %s/%s not found", cmNamespace, t.configMapName) + continue + } + if err != nil { + o.Expect(err).NotTo(o.HaveOccurred(), + fmt.Sprintf("failed to get ConfigMap %s/%s", cmNamespace, t.configMapName)) } configData := cm.Data[t.resolvedKey()] - o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation), - fmt.Sprintf("ConfigMap %s/%s is missing %s annotation", cmNamespace, t.configMapName, injectTLSAnnotation)) + o.Expect(cm.Annotations).To(o.HaveKeyWithValue(injectTLSAnnotation, "true"), + fmt.Sprintf("ConfigMap %s/%s must have %s=true", cmNamespace, t.configMapName, injectTLSAnnotation)),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 696 - 705, The loop fetching ConfigMaps should only treat missing resources as skippable: replace the blanket error skip around oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get by checking apierrors.IsNotFound(err) and skipping only then, otherwise surface/return the error (don't log every error as "not found"). Also make the annotation assertion consistent with testConfigMapTLSInjection by replacing o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation)) with an explicit value check that the injectTLSAnnotation key equals the expected truthy value (e.g., "true") so `"false"` is caught.
🧹 Nitpick comments (1)
test/extended/tls/tls_observed_config.go (1)
125-134: VerifycontrolPlaneflags onconfigMapTargets.Only the etcd entry (line 130) has
controlPlane: true. Other operators likekube-apiserver,kube-controller-manager, andkube-schedulerConfigMaps are not marked despite being control-plane components.If this is intentional (e.g., these ConfigMaps exist in separate
*-operatornamespaces on guest clusters), consider adding a brief comment explaining the rationale to prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/tls/tls_observed_config.go` around lines 125 - 134, The configMapTargets slice currently only marks the etcd entry with controlPlane: true (the literal entry for namespace "openshift-etcd" / configMapName "etcd-operator-config"); update the other control-plane operator entries (for example the entries with namespace "openshift-kube-apiserver", "openshift-kube-controller-manager", and "openshift-kube-scheduler") to include controlPlane: true if they are indeed control-plane ConfigMaps, or if this omission is intentional leave the entries unchanged but add a concise inline comment next to configMapTargets (or above the slice) explaining why only etcd is marked (e.g., those operator ConfigMaps live in separate operator namespaces on guest clusters) so future reviewers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 696-705: The loop fetching ConfigMaps should only treat missing
resources as skippable: replace the blanket error skip around
oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get by checking
apierrors.IsNotFound(err) and skipping only then, otherwise surface/return the
error (don't log every error as "not found"). Also make the annotation assertion
consistent with testConfigMapTLSInjection by replacing
o.Expect(cm.Annotations).To(o.HaveKey(injectTLSAnnotation)) with an explicit
value check that the injectTLSAnnotation key equals the expected truthy value
(e.g., "true") so `"false"` is caught.
---
Nitpick comments:
In `@test/extended/tls/tls_observed_config.go`:
- Around line 125-134: The configMapTargets slice currently only marks the etcd
entry with controlPlane: true (the literal entry for namespace "openshift-etcd"
/ configMapName "etcd-operator-config"); update the other control-plane operator
entries (for example the entries with namespace "openshift-kube-apiserver",
"openshift-kube-controller-manager", and "openshift-kube-scheduler") to include
controlPlane: true if they are indeed control-plane ConfigMaps, or if this
omission is intentional leave the entries unchanged but add a concise inline
comment next to configMapTargets (or above the slice) explaining why only etcd
is marked (e.g., those operator ConfigMaps live in separate operator namespaces
on guest clusters) so future reviewers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9d6193d8-3a17-4efc-8df2-3506db49de05
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
|
Scheduling required tests: |
1 similar comment
|
Scheduling required tests: |
gangwgr
force-pushed
the
refactor-tls-code
branch
from
d226b6b to
c4d1caa
Compare
|
Scheduling required tests: |
|
Job Failure Risk Analysis for sha: c4d1caa
|
| } | ||
|
|
||
| // requireAnnotation asserts the inject-tls annotation is present on the ConfigMap. | ||
| func requireAnnotation(cm *corev1.ConfigMap, namespace, name string) { |
There was a problem hiding this comment.
Worth extending requireAnnotation with annotationKey string argument so the function can be reused. Also, to make it more explicit which annotation is checked.
| } | ||
|
|
||
| // waitForAnnotation polls until the inject-tls annotation is restored to "true". | ||
| func waitForAnnotation(oc *exutil.CLI, ctx context.Context, namespace, name string) { |
There was a problem hiding this comment.
ditto about annotationKey string argument
| return false, nil | ||
| } | ||
| val, found := cm.Annotations[injectTLSAnnotation] | ||
| if found && val == "true" { |
There was a problem hiding this comment.
the annotation value as an argument to waitForAnnotation
| // The config should have a structure like: | ||
| // servingInfo: | ||
| // minTLSVersion: VersionTLS12 | ||
| // cipherSuites: [...] |
There was a problem hiding this comment.
The comment got deleted by accident?
| fmt.Sprintf("ConfigMap %s/%s config does not contain minTLSVersion", cmNamespace, t.configMapName)) | ||
| fmt.Sprintf("ConfigMap %s/%s config does not contain minTLSVersion", t.configMapNamespace, t.configMapName)) | ||
|
|
||
| // Extract actual minTLSVersion for logging. |
There was a problem hiding this comment.
The comment got deleted by accident?
| } | ||
|
|
||
| // Store original minTLSVersion to verify restoration. | ||
| originalMinTLS := "" |
There was a problem hiding this comment.
The code got deleted by accident?
| } | ||
|
|
||
| // updateConfigMap writes the ConfigMap back to the API server. | ||
| func updateConfigMap(oc *exutil.CLI, ctx context.Context, namespace string, cm *corev1.ConfigMap) { |
There was a problem hiding this comment.
updateConfigMap can read the namespace from cm object? So namespace argument can be dropped.
| return result | ||
| } | ||
|
|
||
| func guestSideDeploymentRolloutTargets() []deploymentRolloutTarget { |
There was a problem hiding this comment.
Is this producing the same list?
| // ─── Test implementations ────────────────────────────────────────────────── | ||
|
|
||
| // validateNamespace checks that the namespace exists, skipping the test if not. | ||
| func validateNamespace(oc *exutil.CLI, ctx context.Context, namespace string) { |
There was a problem hiding this comment.
Why moving the code around? Why not move it to the right place in the commit all these functions are introduced?
| var hostedClusterName string | ||
| var hostedClusterNS string | ||
|
|
||
| setupHyperShiftManagement := func() { |
There was a problem hiding this comment.
How is this test related to "migrate all test functions and loops to narrow target types" commit? Perhaps move it into a separate commit?
| namespace string | ||
| operatorConfigGVR schema.GroupVersionResource | ||
| operatorConfigName string | ||
| controlPlane bool |
There was a problem hiding this comment.
s/controlPlane/hostedControlPlane to make it more explicit.
|
|
||
| // ─── Guest-side filters for HyperShift ───────────────────────────────────── | ||
|
|
||
| func guestSideObservedConfigTargets() []observedConfigTarget { |
There was a problem hiding this comment.
Worth inlining all the guestXXX functions in the top g.Describe("[sig-api-machinery][Feature:TLSObservedConfig][Serial][Suite:openshift/tls-observed-config]" as local variables instead of invoking the functions multiple times with the same input producing the same output.
|
Scheduling required tests: |
|
/test e2e-aws-ovn-serial-1of2 |
|
/test e2e-gcp-ovn |
1 similar comment
|
/test e2e-gcp-ovn |
| ) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), | ||
| fmt.Sprintf("%s annotation was not restored on ConfigMap %s/%s within timeout", injectTLSAnnotation, cmNamespace, t.configMapName)) | ||
| fmt.Sprintf("%s annotation was not restored on ConfigMap %s/%s within timeout", |
There was a problem hiding this comment.
nit: keeping this on the same line will make the diff look easier to read
| wrongValue := "VersionTLS10" // An obviously wrong/old TLS version | ||
| wrongValue := "VersionTLS10" | ||
| if strings.Contains(configData, "VersionTLS10") { | ||
| wrongValue = "VersionTLS99" // Use invalid version if TLS10 is somehow present |
There was a problem hiding this comment.
nit: the comment removed on purpose?
| } | ||
|
|
||
| // Determine a wrong value to set (opposite of expected). | ||
| wrongValue := "VersionTLS10" // An obviously wrong/old TLS version |
There was a problem hiding this comment.
nit: the comment removed on purpose?
|
|
||
| e2e.Logf("PASS: %s annotation was restored to 'true' after being set to 'false' on ConfigMap %s/%s", injectTLSAnnotation, cmNamespace, t.configMapName) | ||
| waitForAnnotation(oc, ctx, t.configMapNamespace, t.configMapName, injectTLSAnnotation, "true") | ||
| e2e.Logf("PASS: %s annotation was restored to 'true' after being set to 'false' on ConfigMap %s/%s", |
There was a problem hiding this comment.
nit: keeping it on a single line will make the diff easier to read
Extract validateNamespace, getConfigMap, requireAnnotation, waitForAnnotation, and updateConfigMap as standalone functions. All callers now use these helpers instead of inline logic. The requireAnnotation and waitForAnnotation functions accept an annotation key (and expected value for wait) as arguments, making them reusable for any annotation. The updateConfigMap function reads the namespace from the ConfigMap object itself rather than requiring a separate argument. Fields configMapNamespace and configMapKey are now always specified explicitly in the targets slice. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce observedConfigTarget, configMapTarget, deploymentEnvVarTarget, serviceTarget, and deploymentRolloutTarget types, each carrying only the fields their respective test function reads. Add typed target lists and guest-side filter functions for HyperShift. The monolithic tlsTarget struct and targets slice are retained for now; the next commit migrates all loops and functions to the narrow types. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the monolithic tlsTarget struct and unified targets slice. All test function signatures now accept the narrow types introduced in the previous commit: - testObservedConfig -> observedConfigTarget - testConfigMapTLSInjection -> configMapTarget - testAnnotationRestoration* -> configMapTarget - testServingInfoRestoration* -> configMapTarget - testDeploymentTLSEnvVars -> deploymentEnvVarTarget - testServiceTLS -> serviceTarget All Ginkgo It loops and helper functions (verifyObservedConfig*, verifyConfigMaps*) iterate over the typed lists. HyperShift guest-side filters are computed once as local variables and passed to waitForGuestOperatorsAfterTLSChange. Co-authored-by: Cursor <cursoragent@cursor.com>
…are missing Move HyperShift management cluster setup from BeforeEach into a lazy setupHyperShiftManagement helper that only runs for config-change tests. If HYPERSHIFT_MANAGEMENT_CLUSTER_KUBECONFIG or HYPERSHIFT_MANAGEMENT_CLUSTER_NAMESPACE are not set, the test is skipped instead of failing. Annotation and servingInfo restoration tests do not need management cluster access and continue to work without these environment variables. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Scheduling required tests: |
| if configKey == "" { | ||
| configKey = "config.yaml" | ||
| } | ||
| fmt.Sprintf("ConfigMap %s/%s has inject-tls annotation but value is not 'true': %s", |
There was a problem hiding this comment.
Nit: Another line breaking. Is this the cursor making these extra changes randomly?
| // running on a HyperShift guest cluster. | ||
| // Pre-compute guest-side target lists so the filter functions are | ||
| // called once rather than on every config-change verification. | ||
| guestObservedCfg := guestSideObservedConfigTargets() |
There was a problem hiding this comment.
guestSideObservedConfigTargets() and others can be inlined here since they are invoked only once.
|
@gangwgr: The following test 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. |
2 participants
Summary by CodeRabbit
Tests
Refactor