Add an e2e test for TLS profile update#1393
Open
hongkailiu wants to merge 13 commits into
Open
Conversation
Honor the central TLS profile [1] with event-driven dynamic updates. Implementation uses an APIServer informer with event handlers to proactively cache TLS settings, eliminating per-handshake lister calls while maintaining dynamic reconfiguration capability. The cached settings are applied during TLS handshakes via GetConfigForClient. The commit aims to focus on availability over strict consistency on errors, such as an error fetching the API server object. The CVO provides critical metrics and as such, I am inclined towards availability instead of strict TLS configuration consistency. The TLS adherence feature is currently in Tech Preview. Components do not need to check the feature gate explicitly though [2]: > Component Interaction with the Feature Gate: The feature gate controls > whether the tlsAdherence field is accepted by the API server — > components themselves do not need to check the feature gate. > Because the field is optional (+optional, omitempty), components only > need to handle the field's value when unmarshaling the APIServer config > ... > This means components do not need to set up feature gate watching or > add feature-gate-specific code paths. The ShouldHonorClusterTLSProfile > helper in library-go encapsulates all of this logic. The ShouldHonorClusterTLSProfile helper from library-go encapsulates this logic. Configuration precedence: crypto defaults → central profile → overrides (override support added in next commit for HyperShift compatibility). [1]: https://github.com/openshift/enhancements/blob/master/enhancements/security/centralized-tls-config.md [2]: https://github.com/openshift/enhancements/blob/master/enhancements/security/centralized-tls-config.md#feature-gate Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…perShift Add --tls-min-version and --tls-cipher-suites flags based on recommendations from the centralized TLS config enhancement [1] to support HyperShift deployments: > When these flags are set by the CPO, they take precedence over any > value the component would read from > apiservers.config.openshift.io/cluster. When they are not set, the > component falls back to its normal behavior of watching the cluster config. This allows hosted control planes components, which are deployed in the management cluster, to have different TLS setting or for the components to not need to read the management cluster Kubernetes API server. [1]: https://github.com/openshift/enhancements/blob/master/enhancements/security/centralized-tls-config.md Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
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:
WalkthroughThis PR introduces central TLS profile management to the Cluster Version Operator. A new ChangesTLS Profile Management Feature
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
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: 5
🤖 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 `@pkg/tls/tls_test.go`:
- Around line 473-555: Replace the fixed time.Sleep(100 * time.Millisecond)
waits with bounded polling loops that repeatedly call mgr.ApplySettings(config)
and check config.MinVersion until it matches the expected value (or until a
timeout like 1–2s elapses), failing the test on timeout; do this for each case
that currently sleeps (tests referencing
fakeClient.ConfigV1().APIServers().Update/Delete and the Test blocks that assert
config.MinVersion), polling at a short interval (e.g., 10–50ms) and breaking
early when the condition is met so the informer propagation is robust under slow
CI.
- Around line 42-46: Wait for the informer cache to actually sync and fail the
test immediately if any informer did not sync: call
informerFactory.WaitForCacheSync(ctx.Done()) and capture its returned
map/boolean, iterate over the results and if any entry is false call t.Fatalf
(or t.Fatalf with context and the failing keys), and ensure the created
context's cancel() is deferred (defer cancel()) so the timeout is cleaned up;
this replaces the current blind call to
informerFactory.WaitForCacheSync(ctx.Done()) in the test.
In `@pkg/tls/tls.go`:
- Around line 173-197: CreateOverrides currently leaves o.settings set from a
previous run, causing stale TLS overrides to persist; to fix, ensure you clear
stale parsed overrides by setting o.settings = nil at the start of
Options.CreateOverrides (or before any early returns) so that when
MinVersionOverride or CipherSuitesOverride are empty or validation fails the
Options does not retain prior validated Settings; keep the rest of the
validation logic (validated, MinVersionOverride, CipherSuitesOverride) the same
and only assign validated to o.settings on successful validation.
In `@test/cvo/cvo.go`:
- Around line 126-136: Replace the single synchronous events list/assert with a
bounded retry/poll that repeatedly calls
kubeClient.CoreV1().Events(external.DefaultCVONamespace).List(ctx,
metav1.ListOptions{}) until an event with Reason ==
tls.EventReasonUpdateTLSProfile is observed or a timeout elapses; implement this
using a polling helper (e.g., wait.PollImmediate or gomega.Eventually) with a
clear interval and timeout, returning true when found and then assert that the
poll succeeded (instead of the current immediate loop/expect on events and
found).
- Around line 115-124: This test calls configv1Client.APIServers().Get(...)
without first skipping MicroShift environments; add an explicit MicroShift guard
(e.g., call exutil.IsMicroShiftCluster() and skip if true) at the start of the
g.It block for "should update TLS profile if ClusterTLSProfile should be
honored" so the APIServer API is not invoked on MicroShift clusters; place the
check before the API call to apiServer and keep the existing NotFound handling
intact.
🪄 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: 5c1014b2-5e0c-4baa-a6ac-678fd8a2607d
⛔ Files ignored due to path filters (141)
vendor/github.com/google/btree/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/btree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/btree_generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/merge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/profile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/proto.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/prune.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_strictly_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/PATENTSis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!vendor/**,!**/vendor/**vendor/gomodules.xyz/jsonpatch/v2/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/gomodules.xyz/jsonpatch/v2/jsonpatch.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/ciphersuites_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/colon_separated_multimap_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/configuration_map.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/flags.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/langle_separated_map_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/map_string_bool.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/map_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/namedcertkey_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/noop.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/omitempty.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/sectioned.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/string_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/string_slice_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/tracker_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/tristate.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.golangci.ymlis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.gomodcheck.yamlis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/FAQ.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/Makefileis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/OWNERS_ALIASESis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/README.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/RELEASE.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/SECURITY_CONTACTSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/TMP-LOGGING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/VERSIONING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/alias.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/code-of-conduct.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/options.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/delegating_by_gvk_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/informer_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/cache_reader.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/informers.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/selector.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/certwatcher.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cluster/cluster.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cluster/internal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/config/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/controllerutil.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/name.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue/priorityqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/event/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/event/event.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue_mapped.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue_owner.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/eventhandler.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/healthz/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/healthz/healthz.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/httpserver/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/metrics/workqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/source/event_handler.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/source/kind.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/syncs/syncs.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection/leader_election.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/internal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/manager.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal_posix.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal_windows.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/client_go_adapter.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/leaderelection.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/registry.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/server/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/server/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/workqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/predicate/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/predicate/predicate.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/reconcile/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/reconcile/reconcile.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/recorder/recorder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/source/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/source/source.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/defaulter_custom.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/http.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/multi.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/response.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/validator_custom.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/alias.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion_hubspoke.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion_registry.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/decoder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/server.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
cmd/cluster-version-operator/start.gopkg/cvo/cvo.gopkg/cvo/metrics.gopkg/cvo/sync_worker.gopkg/internal/constants.gopkg/start/start.gopkg/tls/tls.gopkg/tls/tls_test.gotest/cvo/cvo.go
Implements comprehensive test coverage for ProfileManager's event handlers including Add, Update, and Delete events. Tests verify: - Initial profile application from Add event - Profile updates via Update events - Error recovery when invalid profiles are received - TLSAdherence policy changes - Fallback to safe defaults on Delete events Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extracts the repeated pattern of creating and starting an APIServer informer into helper functions to reduce code duplication. Changes: - Add setupInformer helper for simple test cases - Add setupInformerWithClient helper for tests that need the fake client (e.g., for updating APIServer resources in event tests) - Both helpers accept variadic apiServers and custom timeout - Update all tests to use the new helpers, reducing ~80 lines of boilerplate code The helpers use t.Helper() for better test failure reporting. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/tls/tls_test.go (2)
44-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when informer cache sync does not complete.
Line 45 ignores the
WaitForCacheSyncresult, so cache startup failures are hidden and test failures become noisy downstream.Suggested patch
informerFactory.Start(ctx.Done()) - informerFactory.WaitForCacheSync(ctx.Done()) + for informer, synced := range informerFactory.WaitForCacheSync(ctx.Done()) { + if !synced { + t.Fatalf("failed to sync informer cache %v: %v", informer, ctx.Err()) + } + }🤖 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 `@pkg/tls/tls_test.go` around lines 44 - 46, The test currently starts informers with informerFactory.Start(ctx.Done()) but ignores the boolean results returned by informerFactory.WaitForCacheSync(ctx.Done()), which hides cache startup failures; update the test to capture the map[string]bool (or the returned sync result) from WaitForCacheSync and assert that all informers are true, failing fast (e.g., t.Fatalf or t.Fatalf with context) if any informer did not sync; refer to informerFactory.Start, informerFactory.WaitForCacheSync, and the test's *testing.T instance to implement the immediate failure on sync failure.
478-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace fixed sleeps with bounded polling in informer-event assertions.
Lines 479, 513, 538, and 555 use fixed
100mssleeps; informer propagation is asynchronous, so these assertions can be flaky under slower CI.Suggested patch
+ waitForConfig := func(expect func(*tls.Config) bool, msg string) { + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + cfg := &tls.Config{} + mgr.ApplySettings(cfg) + if expect(cfg) { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timeout waiting for expected TLS settings: %s", msg) + } + // Give the informer a moment to process the update - time.Sleep(100 * time.Millisecond) + waitForConfig(func(cfg *tls.Config) bool { return cfg.MinVersion == tls.VersionTLS13 }, "modern profile should set TLS 1.3") @@ - time.Sleep(100 * time.Millisecond) + waitForConfig(func(cfg *tls.Config) bool { return cfg.MinVersion == versionBefore }, "invalid update should retain previous profile") @@ - time.Sleep(100 * time.Millisecond) + waitForConfig(func(cfg *tls.Config) bool { return cfg.MinVersion >= tls.VersionTLS12 }, "NoOpinion should keep safe defaults") @@ - time.Sleep(100 * time.Millisecond) + waitForConfig(func(cfg *tls.Config) bool { return cfg.MinVersion >= tls.VersionTLS12 }, "delete should fall back to safe defaults")Also applies to: 513-513, 538-538, 555-555
🤖 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 `@pkg/tls/tls_test.go` around lines 478 - 480, Tests in pkg/tls/tls_test.go use fixed 100ms time.Sleep calls to wait for informer propagation (occurrences around the time.Sleep calls at lines shown), which makes assertions flaky; replace each fixed sleep with a bounded polling loop (e.g., wait.PollImmediate or a for loop with time.After timeout) that repeatedly checks the informer state or the expected condition (using the same assertion helpers like require/assert) until success or a reasonable timeout (e.g., 2s), and fail the test if the condition never becomes true; update the checks surrounding the existing time.Sleep calls so they actively probe the informer cache/returned value (instead of sleeping) and break early when the expected state is observed.test/cvo/cvo.go (2)
117-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a MicroShift skip guard before the APIServer call.
Line 118 invokes the OpenShift Config API before any MicroShift check. In MicroShift, this API is unsupported and can fail before the intended skip logic.
Suggested patch
g.It("should update TLS profile if ClusterTLSProfile should be honored", func() { + err := util.SkipIfMicroshift(ctx, restCfg) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is MicroShift") + apiServer, err := configv1Client.APIServers().Get(ctx, tlsprofile.APIServerName, metav1.GetOptions{}) if kerrors.IsNotFound(err) { g.Skip(fmt.Sprintf("This test is skipped because APIServer/%s doesn't exist", tlsprofile.APIServerName))As per coding guidelines, "When adding new Ginkgo e2e tests, check whether they use APIs or features unavailable on MicroShift. MicroShift only supports Route and SecurityContextConstraints APIs; all other OpenShift-specific APIs ... are unavailable."
🤖 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/cvo/cvo.go` around lines 117 - 124, This test calls configv1Client.APIServers().Get (using tlsprofile.APIServerName) before checking for MicroShift; add a MicroShift skip guard at the start of the Ginkgo It block so the test bails out on MicroShift platforms before invoking the OpenShift Config API. Specifically, detect MicroShift (the same check pattern used elsewhere in tests) and call g.Skip(...) if running on MicroShift, then proceed to call configv1Client.APIServers().Get and the subsequent crypto.ShouldHonorClusterTLSProfile logic only when not MicroShift.
128-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse bounded polling for event detection instead of a one-shot list.
Line 128 performs a single event list/read; event emission is asynchronous, so this can intermittently fail. Poll with timeout/interval, then assert. (Also fix the assertion text typo:
TCP→TLS.)Suggested patch
- events, err := kubeClient.CoreV1().Events(external.DefaultCVONamespace).List(ctx, metav1.ListOptions{}) - o.Expect(err).NotTo(o.HaveOccurred()) - var found bool - for _, event := range events.Items { - if event.Reason == tls.EventReasonUpdateTLSProfile { - found = true - break - } - } - o.Expect(found).To(o.BeTrue(), "Failed to any event about updating TCP profile") + o.Eventually(func() bool { + events, err := kubeClient.CoreV1().Events(external.DefaultCVONamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + return false + } + for _, event := range events.Items { + if event.Reason == tls.EventReasonUpdateTLSProfile { + return true + } + } + return false + }, "2m", "5s").Should(o.BeTrue(), "Failed to find any event about updating TLS profile")As per coding guidelines, "Ginkgo e2e tests should follow quality requirements: ... (3) timeouts on cluster interactions ... Eventually/Consistently calls."
🤖 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/cvo/cvo.go` around lines 128 - 137, Replace the one-shot call to kubeClient.CoreV1().Events(...).List(...) with a bounded poll (e.g., Gomega's Eventually or wait.PollImmediate) that repeatedly lists events until tls.EventReasonUpdateTLSProfile is observed or a timeout is reached; inside the poll check for the event (current loop that sets found) and assert success after the poll completes, and fix the expectation message to "Failed to find any event about updating TLS profile" (referencing kubeClient.CoreV1().Events, tls.EventReasonUpdateTLSProfile, the found boolean, and the final o.Expect call).
🤖 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 `@pkg/tls/tls.go`:
- Around line 63-69: The APIServer informer UpdateFunc calls mgr.updateSettings
on every resync which triggers ProfileManager.updateSettings to emit
UpdateTLSProfile events even when nothing TLS-relevant changed; modify the
UpdateFunc to cast oldObj and newObj to *configv1.APIServer and short-circuit
unless the TLS-relevant fields actually changed (e.g., compare the relevant
fields or whole TLS-related sub-structure rather than blind invocation), so only
call mgr.updateSettings(apiServer) when those TLS-profile-affecting fields
differ between old and new; ensure you reference UpdateFunc and
mgr.updateSettings/ProfileManager.updateSettings/UpdateTLSProfile when locating
and changing the logic.
---
Duplicate comments:
In `@pkg/tls/tls_test.go`:
- Around line 44-46: The test currently starts informers with
informerFactory.Start(ctx.Done()) but ignores the boolean results returned by
informerFactory.WaitForCacheSync(ctx.Done()), which hides cache startup
failures; update the test to capture the map[string]bool (or the returned sync
result) from WaitForCacheSync and assert that all informers are true, failing
fast (e.g., t.Fatalf or t.Fatalf with context) if any informer did not sync;
refer to informerFactory.Start, informerFactory.WaitForCacheSync, and the test's
*testing.T instance to implement the immediate failure on sync failure.
- Around line 478-480: Tests in pkg/tls/tls_test.go use fixed 100ms time.Sleep
calls to wait for informer propagation (occurrences around the time.Sleep calls
at lines shown), which makes assertions flaky; replace each fixed sleep with a
bounded polling loop (e.g., wait.PollImmediate or a for loop with time.After
timeout) that repeatedly checks the informer state or the expected condition
(using the same assertion helpers like require/assert) until success or a
reasonable timeout (e.g., 2s), and fail the test if the condition never becomes
true; update the checks surrounding the existing time.Sleep calls so they
actively probe the informer cache/returned value (instead of sleeping) and break
early when the expected state is observed.
In `@test/cvo/cvo.go`:
- Around line 117-124: This test calls configv1Client.APIServers().Get (using
tlsprofile.APIServerName) before checking for MicroShift; add a MicroShift skip
guard at the start of the Ginkgo It block so the test bails out on MicroShift
platforms before invoking the OpenShift Config API. Specifically, detect
MicroShift (the same check pattern used elsewhere in tests) and call g.Skip(...)
if running on MicroShift, then proceed to call configv1Client.APIServers().Get
and the subsequent crypto.ShouldHonorClusterTLSProfile logic only when not
MicroShift.
- Around line 128-137: Replace the one-shot call to
kubeClient.CoreV1().Events(...).List(...) with a bounded poll (e.g., Gomega's
Eventually or wait.PollImmediate) that repeatedly lists events until
tls.EventReasonUpdateTLSProfile is observed or a timeout is reached; inside the
poll check for the event (current loop that sets found) and assert success after
the poll completes, and fix the expectation message to "Failed to find any event
about updating TLS profile" (referencing kubeClient.CoreV1().Events,
tls.EventReasonUpdateTLSProfile, the found boolean, and the final o.Expect
call).
🪄 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: 93685e51-fe33-4e03-bac3-215fb5b79dd6
⛔ Files ignored due to path filters (143)
go.sumis excluded by!**/*.sum,!go.sumvendor/github.com/google/btree/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/btree.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/btree/btree_generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/merge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/profile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/proto.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/pprof/profile/prune.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_strictly_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/PATENTSis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sync/errgroup/errgroup.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/tools/go/ast/inspector/cursor.gois excluded by!vendor/**,!**/vendor/**vendor/gomodules.xyz/jsonpatch/v2/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/gomodules.xyz/jsonpatch/v2/jsonpatch.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/ciphersuites_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/colon_separated_multimap_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/configuration_map.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/flags.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/langle_separated_map_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/map_string_bool.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/map_string_string.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/namedcertkey_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/noop.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/omitempty.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/sectioned.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/string_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/string_slice_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/tracker_flag.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/component-base/cli/flag/tristate.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.golangci.ymlis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/.gomodcheck.yamlis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/FAQ.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/Makefileis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/OWNERS_ALIASESis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/README.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/RELEASE.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/SECURITY_CONTACTSis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/TMP-LOGGING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/VERSIONING.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/alias.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/code-of-conduct.mdis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/options.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/builder/webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/delegating_by_gvk_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/informer_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/cache_reader.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/informers.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/selector.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/certwatcher.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/certwatcher/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cluster/cluster.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/cluster/internal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/config/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/controllerutil.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/name.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue/priorityqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/event/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/event/event.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue_mapped.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/enqueue_owner.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/handler/eventhandler.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/healthz/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/healthz/healthz.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/httpserver/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/metrics/workqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/source/event_handler.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/source/kind.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/internal/syncs/syncs.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection/leader_election.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/internal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/manager.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal_posix.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/manager/signals/signal_windows.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/client_go_adapter.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/leaderelection.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/registry.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/server/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/server/server.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/metrics/workqueue.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/predicate/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/predicate/predicate.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/reconcile/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/reconcile/reconcile.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/recorder/recorder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/source/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/source/source.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/defaulter_custom.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/http.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/multi.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/response.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/validator_custom.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/admission/webhook.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/alias.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion_hubspoke.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/conversion_registry.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/decoder.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/conversion/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/doc.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics/metrics.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/webhook/server.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (11)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsoncmd/cluster-version-operator/start.gogo.modpkg/cvo/cvo.gopkg/cvo/metrics.gopkg/cvo/sync_worker.gopkg/internal/constants.gopkg/start/start.gopkg/tls/tls.gopkg/tls/tls_test.gotest/cvo/cvo.go
Comment on lines
+63
to
+69
| UpdateFunc: func(oldObj, newObj interface{}) { | ||
| if apiServer, ok := newObj.(*configv1.APIServer); ok { | ||
| if err := mgr.updateSettings(apiServer); err != nil { | ||
| klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err) | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does client-go SharedInformer call UpdateFunc during periodic resyncs even when an object has not materially changed?
💡 Result:
Yes—during a resync period, client-go SharedInformer will call a handler’s UpdateFunc for every object in its local cache even if the object has not materially changed (i.e., it has not received an actual watch event or list delta). The shared informer’s resync mechanism is explicitly described as “delivering to the handler an update notification for every object in the informer's local cache” and it “does not add any interactions with the authoritative storage” [1]. In the shared informer implementation, resync notifications are routed through the informer’s event distribution as update notifications (Sync/Replaced/Updated/Added all funnel to the Update pathway) [2]. However, the handler will receive oldObj and newObj representing the cached state; many controllers treat resync as an “insurance” rescan and may internally ignore it if resourceVersion (or other fields) indicate no real change. But the UpdateFunc invocation itself is driven by the resync, not by whether the object materially changed [1]. Related nuance: Reflector can also periodically invoke Store.Resync based on ShouldResync; when that happens it triggers in-memory resync processing that results in update notifications to handlers [3].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/master/tools/cache/shared_informer.go
- 2: https://dev.to/jamesli/client-go-deep-dive-deltafifo-the-event-queue-behind-informer-38l
- 3: https://github.com/kubernetes/client-go/blob/master/tools/cache/reflector.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read targeted portion of pkg/tls/tls.go around the UpdateFunc
sed -n '1,220p' pkg/tls/tls.go | nl -ba | sed -n '40,140p' || true
# 2) Find updateSettings and UpdateTLSProfile usage in pkg/tls/tls.go
rg -n "updateSettings|UpdateTLSProfile|TLSSecurityProfile|TLSAdherence" pkg/tls/tls.go || true
# 3) Inspect where informer resync period is set/created (pkg/start/start.go)
rg -n "NewSharedInformerFactory|resync|Resync|configInformerFactory" pkg/start/start.go || true
sed -n '1,260p' pkg/start/start.go | nl -ba | sed -n '1,220p' || trueRepository: openshift/cluster-version-operator
Length of output: 3569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact handler code around lines ~50-90 in pkg/tls/tls.go
sed -n '40,105p' pkg/tls/tls.go
# Show the full updateSettings function body (approximate range based on discovered line numbers)
sed -n '80,160p' pkg/tls/tls.go
# Show some context around where updateSettings(nil) is called (around ~60-90)
sed -n '55,95p' pkg/tls/tls.go
# Search within pkg/tls/tls.go for any caching fields used by updateSettings
rg -n "cache|cached|last|previous|appl|profile|TLSAdherence|TLSSecurityProfile|ShouldHonorClusterTLSProfile" pkg/tls/tls.go || trueRepository: openshift/cluster-version-operator
Length of output: 9214
Gate the APIServer informer UpdateFunc to avoid resync-triggered TLS event spam
ProfileManager.updateSettings() emits the UpdateTLSProfile event on every invocation (including when it resolves to a nil apply function when TLS adherence disables the profile). Because the APIServer informer is configured with a non-zero resync period in pkg/start/start.go, this UpdateFunc will run during periodic resyncs and for unrelated APIServer changes, producing repeated false-positive TLS update events.
Suggested change
+import "reflect"
+
UpdateFunc: func(oldObj, newObj interface{}) {
- if apiServer, ok := newObj.(*configv1.APIServer); ok {
- if err := mgr.updateSettings(apiServer); err != nil {
+ oldAPIServer, oldOK := oldObj.(*configv1.APIServer)
+ newAPIServer, newOK := newObj.(*configv1.APIServer)
+ if !oldOK || !newOK {
+ return
+ }
+ if reflect.DeepEqual(oldAPIServer.Spec.TLSSecurityProfile, newAPIServer.Spec.TLSSecurityProfile) &&
+ reflect.DeepEqual(oldAPIServer.Spec.TLSAdherence, newAPIServer.Spec.TLSAdherence) {
+ return
+ }
+ if err := mgr.updateSettings(newAPIServer); err != nil {
klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err)
- }
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| UpdateFunc: func(oldObj, newObj interface{}) { | |
| if apiServer, ok := newObj.(*configv1.APIServer); ok { | |
| if err := mgr.updateSettings(apiServer); err != nil { | |
| klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err) | |
| } | |
| } | |
| }, | |
| UpdateFunc: func(oldObj, newObj interface{}) { | |
| oldAPIServer, oldOK := oldObj.(*configv1.APIServer) | |
| newAPIServer, newOK := newObj.(*configv1.APIServer) | |
| if !oldOK || !newOK { | |
| return | |
| } | |
| if reflect.DeepEqual(oldAPIServer.Spec.TLSSecurityProfile, newAPIServer.Spec.TLSSecurityProfile) && | |
| reflect.DeepEqual(oldAPIServer.Spec.TLSAdherence, newAPIServer.Spec.TLSAdherence) { | |
| return | |
| } | |
| if err := mgr.updateSettings(newAPIServer); err != nil { | |
| klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err) | |
| } | |
| }, |
🤖 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 `@pkg/tls/tls.go` around lines 63 - 69, The APIServer informer UpdateFunc calls
mgr.updateSettings on every resync which triggers ProfileManager.updateSettings
to emit UpdateTLSProfile events even when nothing TLS-relevant changed; modify
the UpdateFunc to cast oldObj and newObj to *configv1.APIServer and
short-circuit unless the TLS-relevant fields actually changed (e.g., compare the
relevant fields or whole TLS-related sub-structure rather than blind
invocation), so only call mgr.updateSettings(apiServer) when those
TLS-profile-affecting fields differ between old and new; ensure you reference
UpdateFunc and mgr.updateSettings/ProfileManager.updateSettings/UpdateTLSProfile
when locating and changing the logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/tls/tls.go (2)
66-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate APIServer update handling to TLS-relevant field changes.
UpdateFuncstill callsupdateSettingson every APIServer update/resync, so TLS update processing can run even when TLS profile/adherence did not change. Please short-circuit unless TLS-relevant fields differ betweenoldObjandnewObj.Proposed patch
UpdateFunc: func(oldObj, newObj interface{}) { - if apiServer, ok := newObj.(*configv1.APIServer); ok { - if err := mgr.updateSettings(apiServer); err != nil { - klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err) - } - } + oldAPIServer, oldOK := oldObj.(*configv1.APIServer) + newAPIServer, newOK := newObj.(*configv1.APIServer) + if !oldOK || !newOK { + return + } + if oldAPIServer.Spec.TLSAdherence == newAPIServer.Spec.TLSAdherence && + oldAPIServer.Spec.TLSSecurityProfile == newAPIServer.Spec.TLSSecurityProfile { + return + } + if err := mgr.updateSettings(newAPIServer); err != nil { + klog.Errorf("Failed to apply TLS settings on APIServer update: %v", err) + } },🤖 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 `@pkg/tls/tls.go` around lines 66 - 72, The UpdateFunc currently calls mgr.updateSettings on every APIServer update; change it to first type-assert oldObj and newObj to *configv1.APIServer and compare only TLS-relevant fields (e.g. Spec.TLSSecurityProfile, Spec.ServingInfo.TLS/cipher suite settings, any TLS profile/adherence flags) using a deep-equality check (or field-by-field comparison) and return early if they are equal; only call mgr.updateSettings(apiServer) when those TLS-related fields differ, keeping the existing error logging behavior.
189-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear cached overrides before parsing new CLI override state.
CreateOverrides()can retain staleo.settingswhen called again with empty/invalid inputs after a prior valid parse. Reseto.settingsat function start.Proposed patch
func (o *Options) CreateOverrides() error { + o.settings = nil + // If no overrides, return nil (central profile or defaults will be used) if o.MinVersionOverride == "" && len(o.CipherSuitesOverride) == 0 { return nil }Also applies to: 213-214
🤖 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 `@pkg/tls/tls.go` around lines 189 - 193, Reset cached override state before parsing: at the start of Options.CreateOverrides() clear o.settings (e.g., set to nil or a zero-value) so stale settings from a prior successful parse can't be reused when new inputs are empty/invalid; apply the same reset in the other override-parsing routine in this file where o.settings is populated (the later function that sets o.settings around the other override parsing code) so both code paths always start from a clean state before parsing new CLI override values.test/cvo/cvo.go (2)
117-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit MicroShift guard before APIServer calls.
This test should skip MicroShift before invoking
configv1Client.APIServers().Get(...)to avoid environment-specific failures.Proposed patch
g.It("should update TLS profile if ClusterTLSProfile should be honored", func() { + err := util.SkipIfMicroshift(ctx, restCfg) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is MicroShift") + apiServer, err := configv1Client.APIServers().Get(ctx, tlsprofile.APIServerName, metav1.GetOptions{}) if kerrors.IsNotFound(err) { g.Skip(fmt.Sprintf("This test is skipped because APIServer/%s doesn't exist", tlsprofile.APIServerName))🤖 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/cvo/cvo.go` around lines 117 - 123, Add an explicit MicroShift environment guard at the start of the test "should update TLS profile if ClusterTLSProfile should be honored" before calling configv1Client.APIServers().Get(...): detect MicroShift (use the existing platform check helper or add a small isMicroShift() check) and call g.Skip(...) with a brief message when on MicroShift, then proceed with the APIServer get call and existing error handling; update the test in test/cvo/cvo.go around the g.It block to place this guard immediately before invoking configv1Client.APIServers().Get so the call is never made in MicroShift environments.
128-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse bounded polling for event detection to avoid flakes.
Event recording is asynchronous; a one-time list/assert can race. Poll until timeout for
Reason == tls.EventReasonUpdateTLSProfile.Proposed patch
- events, err := kubeClient.CoreV1().Events(external.DefaultCVONamespace).List(ctx, metav1.ListOptions{}) - o.Expect(err).NotTo(o.HaveOccurred()) - var found bool - for _, event := range events.Items { - if event.Reason == tls.EventReasonUpdateTLSProfile { - found = true - break - } - } - o.Expect(found).To(o.BeTrue(), "Failed to any event about updating TCP profile") + o.Eventually(func() bool { + events, err := kubeClient.CoreV1().Events(external.DefaultCVONamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + return false + } + for _, event := range events.Items { + if event.Reason == tls.EventReasonUpdateTLSProfile { + return true + } + } + return false + }, "2m", "5s").Should(o.BeTrue(), "Failed to find any event about updating TLS profile")🤖 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/cvo/cvo.go` around lines 128 - 137, The current one-time list of events can race because event recording is asynchronous; replace the direct List+assert with bounded polling until a timeout to wait for an event with Reason == tls.EventReasonUpdateTLSProfile. Use the existing kubeClient.CoreV1().Events(...).List(...) call inside a retry loop (e.g., wait.PollImmediate or Gomega's Eventually) that re-lists events and checks event.Reason against tls.EventReasonUpdateTLSProfile, returning success when found or failing the test after the timeout; keep the same namespace (external.DefaultCVONamespace) and context (ctx) and preserve the final assertion message.
🤖 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/cvo/cvo.go`:
- Line 137: Update the assertion message in the Expect call that checks the
boolean variable found (the line with o.Expect(found).To(o.BeTrue(), ...)) to
replace the incorrect "TCP profile" text with "TLS profile" so the failure
message correctly reflects TLS-profile validation.
---
Duplicate comments:
In `@pkg/tls/tls.go`:
- Around line 66-72: The UpdateFunc currently calls mgr.updateSettings on every
APIServer update; change it to first type-assert oldObj and newObj to
*configv1.APIServer and compare only TLS-relevant fields (e.g.
Spec.TLSSecurityProfile, Spec.ServingInfo.TLS/cipher suite settings, any TLS
profile/adherence flags) using a deep-equality check (or field-by-field
comparison) and return early if they are equal; only call
mgr.updateSettings(apiServer) when those TLS-related fields differ, keeping the
existing error logging behavior.
- Around line 189-193: Reset cached override state before parsing: at the start
of Options.CreateOverrides() clear o.settings (e.g., set to nil or a zero-value)
so stale settings from a prior successful parse can't be reused when new inputs
are empty/invalid; apply the same reset in the other override-parsing routine in
this file where o.settings is populated (the later function that sets o.settings
around the other override parsing code) so both code paths always start from a
clean state before parsing new CLI override values.
In `@test/cvo/cvo.go`:
- Around line 117-123: Add an explicit MicroShift environment guard at the start
of the test "should update TLS profile if ClusterTLSProfile should be honored"
before calling configv1Client.APIServers().Get(...): detect MicroShift (use the
existing platform check helper or add a small isMicroShift() check) and call
g.Skip(...) with a brief message when on MicroShift, then proceed with the
APIServer get call and existing error handling; update the test in
test/cvo/cvo.go around the g.It block to place this guard immediately before
invoking configv1Client.APIServers().Get so the call is never made in MicroShift
environments.
- Around line 128-137: The current one-time list of events can race because
event recording is asynchronous; replace the direct List+assert with bounded
polling until a timeout to wait for an event with Reason ==
tls.EventReasonUpdateTLSProfile. Use the existing
kubeClient.CoreV1().Events(...).List(...) call inside a retry loop (e.g.,
wait.PollImmediate or Gomega's Eventually) that re-lists events and checks
event.Reason against tls.EventReasonUpdateTLSProfile, returning success when
found or failing the test after the timeout; keep the same namespace
(external.DefaultCVONamespace) and context (ctx) and preserve the final
assertion message.
🪄 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: 51587912-9e80-4737-ae46-dfc2aa770cf4
📒 Files selected for processing (3)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsonpkg/tls/tls.gotest/cvo/cvo.go
Member
Author
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/cvo/cvo.go (2)
120-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit environment skip guard before the APIServer API call.
This test calls OpenShift config APIs immediately; in unsupported environments that can fail instead of skip cleanly.
Suggested patch
g.It("should update TLS profile", func() { + err := util.SkipIfMicroshift(ctx, restCfg) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is MicroShift") + apiServer, err := configV1Client.APIServers().Get(ctx, tlsprofile.APIServerName, metav1.GetOptions{}) if kerrors.IsNotFound(err) { g.Skip(fmt.Sprintf("This test is skipped because APIServer/%s doesn't exist", tlsprofile.APIServerName)) } else { o.Expect(err).NotTo(o.HaveOccurred()) }As per coding guidelines, "When skipping tests for certain environments, document the reason".
🤖 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/cvo/cvo.go` around lines 120 - 126, Before calling configV1Client.APIServers().Get in the It("should update TLS profile") test, add an explicit environment skip guard that verifies the test is running in a supported OpenShift/config API environment (e.g. a helper like ensureConfigAPISupported or isOpenShiftEnvironment) and call g.Skip with a clear reason string if not supported; move or wrap the existing kerrors.IsNotFound handling after this guard so the Get is only attempted in supported environments and document the skip reason in the g.Skip message referencing APIServer/config API availability and tlsprofile.APIServerName.
154-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the assertion message to match what is actually asserted.
This assertion checks TLS log content, but the failure text currently references an event and “TCP profile”.
Suggested patch
- o.Expect(strings.Contains(buf.String(), tls.LogMessageUpdatedTLSProfile)).To(o.BeTrue(), "Failed to any event about updating TCP profile when ShouldHonorClusterTLSProfile=%t", crypto.ShouldHonorClusterTLSProfile(apiServer.Spec.TLSAdherence)) + o.Expect(strings.Contains(buf.String(), tls.LogMessageUpdatedTLSProfile)).To(o.BeTrue(), "Failed to find TLS profile update log when ShouldHonorClusterTLSProfile=%t", crypto.ShouldHonorClusterTLSProfile(apiServer.Spec.TLSAdherence))🤖 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/cvo/cvo.go` at line 154, The failure message in the Expect assertion mismatches the check: update the assertion's failure text to describe the TLS log check rather than an event or "TCP profile". Modify the Expect call that checks strings.Contains(buf.String(), tls.LogMessageUpdatedTLSProfile) to use a message that references verifying the TLS log message (e.g., mention tls.LogMessageUpdatedTLSProfile and ShouldHonorClusterTLSProfile(apiServer.Spec.TLSAdherence)) so the failure text accurately reflects the asserted condition.
🤖 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/cvo/cvo.go`:
- Around line 128-131: The pod list call uses the wrong namespace: replace the
use of external.DefaultClusterVersionName in kubeClient.CoreV1().Pods(...).List
with the CVO namespace variable (i.e. the package-level CVO namespace constant
used elsewhere) so the call targets the Cluster Version Operator namespace;
update the Pods(...).List invocation to pass that CVO namespace (rather than
external.DefaultClusterVersionName) to fix the test path.
---
Duplicate comments:
In `@test/cvo/cvo.go`:
- Around line 120-126: Before calling configV1Client.APIServers().Get in the
It("should update TLS profile") test, add an explicit environment skip guard
that verifies the test is running in a supported OpenShift/config API
environment (e.g. a helper like ensureConfigAPISupported or
isOpenShiftEnvironment) and call g.Skip with a clear reason string if not
supported; move or wrap the existing kerrors.IsNotFound handling after this
guard so the Get is only attempted in supported environments and document the
skip reason in the g.Skip message referencing APIServer/config API availability
and tlsprofile.APIServerName.
- Line 154: The failure message in the Expect assertion mismatches the check:
update the assertion's failure text to describe the TLS log check rather than an
event or "TCP profile". Modify the Expect call that checks
strings.Contains(buf.String(), tls.LogMessageUpdatedTLSProfile) to use a message
that references verifying the TLS log message (e.g., mention
tls.LogMessageUpdatedTLSProfile and
ShouldHonorClusterTLSProfile(apiServer.Spec.TLSAdherence)) so the failure text
accurately reflects the asserted condition.
🪄 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: a9721c2c-efd9-4a72-9824-857cca100719
📒 Files selected for processing (3)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsonpkg/tls/tls.gotest/cvo/cvo.go
Replace fixed time.Sleep calls with wait.PollUntilContextTimeout in Test_tlsProfileManager_EventHandlers to make tests more robust and faster. Tests now poll for expected conditions with 50ms intervals and 5 second timeout, completing as soon as the condition is met rather than always waiting 100ms. This reduces flakiness from timing variations and improves test execution time. Changes: - Add import for k8s.io/apimachinery/pkg/util/wait - Replace sleep in "update event changes profile" test with polling - Replace sleep in "update event disables profile" test with polling - Replace sleep in "delete event falls back" test with polling - Keep sleep for invalid profile test (negative assertion) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
@hongkailiu: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work from this pull:
Test_tlsProfileManager_EventHandlersSummary by CodeRabbit
New Features
--tls-min-versionand--tls-cipher-suitesenable TLS configuration overridesTests