[wip]Fix empty lister for API server#1397
Open
hongkailiu wants to merge 18 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>
# Conflicts: # pkg/cvo/cvo.go # pkg/start/start.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>
Initializing ProfileManager before the APIServer informer syncs causes the lister to be empty, forcing CVO to use library default TLS settings instead of the cluster's configured TLS profile from the APIServer resource. This creates inconsistency where CVO starts with different TLS settings than what the cluster administrator configured, potentially causing connection issues or security policy violations. Move ProfileManager initialization to after informer sync in start.go to ensure the cluster's actual TLS profile is applied from the start. Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
This reverts commit 97a5bd2.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR implements event-driven TLS profile management for the cluster-version-operator. It adds CLI flags for TLS overrides ( ChangesTLS Profile Management Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (13 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.
🧹 Nitpick comments (3)
pkg/tls/tls_test.go (2)
41-44: ⚡ Quick winAssert that the cache actually synced.
WaitForCacheSync's result is discarded, so if sync fails (e.g. thectxtimeout fires) the helper returns an informer with an empty lister and downstream tests fail in confusing, indirect ways. Given this PR specifically targets the empty-lister problem, verifying sync here makes failures explicit.♻️ Proposed change
informerFactory.Start(ctx.Done()) - informerFactory.WaitForCacheSync(ctx.Done()) + for typ, synced := range informerFactory.WaitForCacheSync(ctx.Done()) { + if !synced { + t.Fatalf("failed to sync informer cache for %v", typ) + } + } return apiServerInformer, fakeClient, cancel🤖 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 41 - 44, The test currently starts informers but ignores informerFactory.WaitForCacheSync's return value; capture the sync result (e.g. syncMap := informerFactory.WaitForCacheSync(ctx.Done()) or syncOK := informerFactory.WaitForCacheSync(ctx.Done()) and assert that the expected informer(s) synced (for example verify all values in the returned map are true, or the single bool is true) and fail the test with t.Fatalf or t.Fatalf-like helper if sync did not complete; also ensure the created ctx cancel is invoked (defer cancel()) so the timeout is properly released. Use the existing symbols ctx, cancel, informerFactory.Start and informerFactory.WaitForCacheSync to locate where to add the checks.
434-435: ⚡ Quick winReplace fixed
time.Sleepwaits with bounded polling to avoid flakiness.The event-driven assertions rely on a fixed
time.Sleep(100 * time.Millisecond)to let the informer process each update/delete. On loaded CI this is a classic source of flaky failures (and, conversely, wastes time when processing is fast). The same pattern repeats at Lines 469, 494, and 511. Prefer polling the manager state with a timeout (e.g.k8s.io/apimachinery/pkg/util/waitPollUntilContextTimeout) until the expectedMinVersionis observed.Note also that the informer's lifetime is bounded by the
5*time.Secondcontext timeout insetupInformerWithClient; if the cumulative sub-test runtime ever exceeds it, later events silently stop propagating. Polling against the manager rather than sleeping makes the intent explicit and decouples correctness from wall-clock timing.🤖 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 434 - 435, Replace the fixed sleeps (time.Sleep(100 * time.Millisecond)) used to wait for informer events with bounded polling against the manager state: instead of sleeping in the tests that call setupInformerWithClient, use a context-aware poll (e.g. k8s.io/apimachinery/pkg/util/wait.PollImmediateWithContext or PollUntilContextTimeout) to repeatedly check the manager/handler's observed MinVersion until it equals the expected value or the timeout elapses; update the four occurrences (including the spots that assert MinVersion after update/delete) to poll with a short interval and a conservative timeout that fits within setupInformerWithClient's 5*time.Second context, and add the required import for wait.pkg/cvo/cvo.go (1)
138-138: 💤 Low valueUnused field
apiServerListeradded to Operator struct.This field is added but never assigned in the constructor. If it's for future use, consider adding it when needed to avoid dead code.
🤖 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/cvo/cvo.go` at line 138, The Operator struct contains an unused field apiServerLister of type configlistersv1.APIServerLister; remove this field from the Operator struct to avoid dead code, or if it was intended to be used, wire it up in the constructor (e.g., in NewOperator) by accepting/obtaining a configlistersv1.APIServerLister and assigning it to o.apiServerLister so the field is actually initialized and used.
🤖 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.
Nitpick comments:
In `@pkg/cvo/cvo.go`:
- Line 138: The Operator struct contains an unused field apiServerLister of type
configlistersv1.APIServerLister; remove this field from the Operator struct to
avoid dead code, or if it was intended to be used, wire it up in the constructor
(e.g., in NewOperator) by accepting/obtaining a configlistersv1.APIServerLister
and assigning it to o.apiServerLister so the field is actually initialized and
used.
In `@pkg/tls/tls_test.go`:
- Around line 41-44: The test currently starts informers but ignores
informerFactory.WaitForCacheSync's return value; capture the sync result (e.g.
syncMap := informerFactory.WaitForCacheSync(ctx.Done()) or syncOK :=
informerFactory.WaitForCacheSync(ctx.Done()) and assert that the expected
informer(s) synced (for example verify all values in the returned map are true,
or the single bool is true) and fail the test with t.Fatalf or t.Fatalf-like
helper if sync did not complete; also ensure the created ctx cancel is invoked
(defer cancel()) so the timeout is properly released. Use the existing symbols
ctx, cancel, informerFactory.Start and informerFactory.WaitForCacheSync to
locate where to add the checks.
- Around line 434-435: Replace the fixed sleeps (time.Sleep(100 *
time.Millisecond)) used to wait for informer events with bounded polling against
the manager state: instead of sleeping in the tests that call
setupInformerWithClient, use a context-aware poll (e.g.
k8s.io/apimachinery/pkg/util/wait.PollImmediateWithContext or
PollUntilContextTimeout) to repeatedly check the manager/handler's observed
MinVersion until it equals the expected value or the timeout elapses; update the
four occurrences (including the spots that assert MinVersion after
update/delete) to poll with a short interval and a conservative timeout that
fits within setupInformerWithClient's 5*time.Second context, and add the
required import for wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c7d6ad8a-f3f0-4d83-8653-6f362d24253d
⛔ 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 (7)
cmd/cluster-version-operator/start.gogo.modpkg/cvo/cvo.gopkg/cvo/metrics.gopkg/start/start.gopkg/tls/tls.gopkg/tls/tls_test.go
Member
Author
|
Repeating the steps in #1338 (comment): |
hongkailiu
force-pushed
the
pr1338-fix-empty-lister-simple
branch
from
82b6d6f to
3a50eca
Compare
Follow up the issue from openshift#1338 (comment): The right way to call things out for `InformerFactory`: e.g., [processInitialFeatureGate](https://github.com/openshift/cluster-version-operator/blob/c1dc3e8f0b7c366d45177af391c4f888059b446d/pkg/start/start.go#L269-L280), * `Lister()` such as `configInformerFactory.Config().V1().FeatureGates().Lister()` * `Start()` such as `configInformerFactory.Start(ctx.Done())` * `WaitForCacheSync*()` such as `configInformerFactory.WaitForCacheSync(ctx.Done())` The lister is ready to retrieve instance, e.g., via `Get()`. ```console $ git --no-pager log --pretty=oneline -1 6a8c607 (HEAD -> pr1338-e2e-6a8c6072b2f4e45fe5ab9269b4efa96d4608b0d0) make update ``` The issue with the commit above: ``` start.go: Line 190 func (o *Options) Run(ctx context.Context) error * Line 216 o.NewControllerContext() * Line 650 cvo, err := cvo.New() where Lister() and cvotls.NewProfileManager(apiServerInformer, tlsOverrides) are called where apiServerInformer.Lister().Get(tlsprofile.APIServerName) is called * Line 221 o.run() * start and sync ``` The issue is that `get()` is called in the wrong order: `Lister()`, `Get()`, `Start()` and `Sync()`. It explains `apiserver.config.openshift.io "cluster" not found` from openshift#1338 (comment). In short, we need the informer factory is ready before initializing the TLS profile manager. The fix is moving `NewProfileManager()` from `cvo.go` to `start.go` and the instance passes to `cvo.go`. Note that the same `configInformerFactory` instance is started and synced in `o.processInitialFeatureGate`. The only thing missing is invoking its `List()` ahead of it. Once it is done, we do not need to do those again in `cvo.go`. I prefer the current fix over the reverted one because the code is simpler this way. `cvo` does not need to have the fields such as the informer and the overrides that actually belong to the TLS profile manager.
hongkailiu
force-pushed
the
pr1338-fix-empty-lister-simple
branch
from
3a50eca to
f89bd0c
Compare
Contributor
|
@hongkailiu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Member
Author
|
Take this job: Still good. |
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.
Will rebase after #1338 gets in
Summary by CodeRabbit
New Features
Tests
Chores