WIP: Testing only; no merge#1392
Closed
hongkailiu wants to merge 6 commits into
Closed
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>
openshift-ci
Bot
added
do-not-merge/hold
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
WalkthroughThis PR introduces a central TLS profile manager that reads Kubernetes APIServer TLS configuration, applies it with CLI override precedence, and propagates settings through the CVO operator to its metrics server. New CLI flags allow operators to override TLS minimum version and cipher suites. ChangesTLS Profile Manager Feature
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Flags
participant Startup as Start Options
participant Manager as TLS Manager
participant CVO as CVO Operator
participant Metrics as Metrics Server
participant APIServer as APIServer Resource
CLI->>Startup: Parse --tls-min-version, --tls-cipher-suites
Startup->>Startup: ValidateAndComplete: parse & validate overrides
Startup->>Manager: NewProfileManager(apiServerInformer, overrides)
Manager->>APIServer: Read TLS profile spec
Manager->>Manager: Cache apply function + register event handlers
Startup->>CVO: cvo.New(..., apiServerInformer, overrides)
CVO->>CVO: Initialize profileMgr
Startup->>Metrics: RunMetrics(..., CVO.ApplySettings())
APIServer->>Manager: Emit add/update/delete events on changes
Manager->>Manager: Update cached TLS profile
Metrics->>Manager: GetConfigForClient calls applySettings(config)
Manager->>Metrics: Apply central profile + overrides to TLS config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
⚔️ Resolve merge conflicts
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: 2
🧹 Nitpick comments (2)
go.mod (1)
30-35: 🏗️ Heavy liftVerify Kubernetes direct dependency patch skew is intentional.
Direct
k8s.io/*dependencies are mixed acrossv0.35.1andv0.35.2. If this isn’t deliberate, aligning patch versions usually reduces resolver/compatibility surprises in controller-runtime stacks.Also applies to: 37-37
🤖 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 `@go.mod` around lines 30 - 35, The go.mod has mixed Kubernetes patch versions (some v0.35.1 and some v0.35.2) which can cause resolver/compatibility issues; update the entries for k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/apiserver, k8s.io/client-go, and k8s.io/component-base so they all use the same patch level (e.g., all v0.35.2 or all v0.35.1) to align patch versions consistently across the direct k8s.io/* dependencies, then run go mod tidy to refresh the lock info.pkg/tls/tls_test.go (1)
425-493: 🏗️ Heavy liftBefore enabling this test, stop bypassing the production handler path.
After
NewProfileManagerregisters its own informer handler, this test adds a second handler that callsmgr.updateSettingsdirectly. Oncet.Skipis removed, the assertions can still pass with brokenregisterEventHandlerswiring because the test-owned handler keeps the manager updated. Prefer observing the state changes driven by the manager's existing handler, or testregisterEventHandlersin isolation.If helpful, I can sketch a deterministic fake-client/informer version of this test.
🤖 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 425 - 493, The test bypasses the manager's own event handler by adding a second informer handler that calls mgr.updateSettings directly; remove that AddEventHandler block and instead exercise the manager's production path by either calling the manager's registerEventHandlers (or exposing a method to register handlers) and starting the informer factory (informerFactory.Start(...) and wait for cache sync), then create/update/delete APIServer objects via the fakeClient so the manager's registered handler processes them and pushes updates; keep the existing helpers (waitForEvent, verifyMinVersion) but drive events through fakeClient.ConfigV1().APIServers().Create/Update/Delete so registerEventHandlers, NewProfileManager, and updateSettings are exercised deterministically.
🤖 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 `@go.mod`:
- Around line 11-12: go.mod declares github.com/onsi/ginkgo/v2 v2.28.1 but also
contains a replace directive pointing github.com/onsi/ginkgo/v2 =>
github.com/openshift/onsi-ginkgo/v2 v2.6.1-0... which creates a version
mismatch; either remove the replace to use upstream v2.28.1 or update the
replace target to an openshift/onsi-ginkgo commit that is API-compatible with
v2.28.x so the required and replaced versions align, and similarly reconcile
k8s.io/* versions (e.g., k8s.io/api vs k8s.io/apiserver) so their minor/patch
versions are consistent per project compatibility policy and tests.
In `@pkg/tls/tls.go`:
- Around line 171-196: CreateOverrides currently preserves o.settings from prior
runs when there are no overrides or when validation later fails; to fix, clear
the cached overrides at the start of CreateOverrides by setting o.settings = nil
(or otherwise resetting the cached Settings) so each call recomputes from flags;
keep the rest of the validation logic as-is (references: CreateOverrides method
and the o.settings field, also GetOverrides/ProfileManager which read
o.settings).
---
Nitpick comments:
In `@go.mod`:
- Around line 30-35: The go.mod has mixed Kubernetes patch versions (some
v0.35.1 and some v0.35.2) which can cause resolver/compatibility issues; update
the entries for k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery,
k8s.io/apiserver, k8s.io/client-go, and k8s.io/component-base so they all use
the same patch level (e.g., all v0.35.2 or all v0.35.1) to align patch versions
consistently across the direct k8s.io/* dependencies, then run go mod tidy to
refresh the lock info.
In `@pkg/tls/tls_test.go`:
- Around line 425-493: The test bypasses the manager's own event handler by
adding a second informer handler that calls mgr.updateSettings directly; remove
that AddEventHandler block and instead exercise the manager's production path by
either calling the manager's registerEventHandlers (or exposing a method to
register handlers) and starting the informer factory (informerFactory.Start(...)
and wait for cache sync), then create/update/delete APIServer objects via the
fakeClient so the manager's registered handler processes them and pushes
updates; keep the existing helpers (waitForEvent, verifyMinVersion) but drive
events through fakeClient.ConfigV1().APIServers().Create/Update/Delete so
registerEventHandlers, NewProfileManager, and updateSettings are exercised
deterministically.
🪄 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: 05bceda1-1010-40cc-8afd-cda40eeacddc
⛔ Files ignored due to path filters (198)
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/net/http2/transport.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/http2/writesched_priority_rfc9218.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/trace/events.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/websocket/hybi.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/sys/unix/mkerrors.shis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_386.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_amd64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_loong64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mips64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_mipsle.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_ppc64le.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_riscv64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/zerrors_linux_sparc64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/sys/unix/ztypes_netbsd_arm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/term/terminal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/eucjp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/iso2022jp.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/japanese/shiftjis.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/korean/euckr.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/gbk.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/simplifiedchinese/hzgb2312.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/traditionalchinese/big5.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/text/encoding/unicode/unicode.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/k8s.io/utils/buffer/ring_fixed.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/client/apiutil/apimachinery.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/apiutil/errors.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/dryrun.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/versioned_tracker.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fieldowner.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/fieldvalidation.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor/intercept.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/namespaced_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/options.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/patch.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/typed_client.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/client/unstructured_client.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/log/deleg.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/log.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/controller-runtime/pkg/log/null.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/**vendor/sigs.k8s.io/structured-merge-diff/v6/schema/elements.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/typed/remove.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/value/reflectcache.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
Comment on lines
+11
to
+12
| github.com/onsi/ginkgo/v2 v2.28.1 | ||
| github.com/onsi/gomega v1.39.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show go.mod with line numbers around the cited regions.
echo "=== go.mod lines 1-180 (numbered, but keep it bounded) ==="
nl -ba go.mod | sed -n '1,180p'
# Extract just the relevant require/replace entries (if present).
echo "=== Matching ginkgo/gomega entries ==="
rg -n "github.com/onsi/ginkgo/v2|github.com/onsi/gomega|openshift/onsi-ginkgo/v2" go.mod || true
echo "=== Matching k8s.io/* direct deps and versions ==="
# Print k8s.io.* lines inside require blocks.
rg -n "^\s*k8s\.io/.*\sv0\." go.mod || true
echo "=== Matching all k8s.io/* replace directives (if any) ==="
rg -n "^\s*replace\s+k8s\.io/|replace\s+\(k8s\.io/" go.mod || trueRepository: openshift/cluster-version-operator
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -u
echo "=== go.mod lines 1-180 (numbered) ==="
awk '{printf "%6d\t%s\n", NR, $0}' go.mod | sed -n '1,180p'
echo "=== Matching ginkgo/gomega entries ==="
rg -n "github.com/onsi/ginkgo/v2|github.com/onsi/gomega|openshift/onsi-ginkgo/v2" go.mod || true
echo "=== Matching k8s.io/* require lines (v0.35.*) ==="
rg -n "^\s*k8s\.io/.*\sv0\.35\." go.mod || true
echo "=== Matching k8s.io/* replace directives ==="
rg -n "^\s*replace\s+k8s\.io/|replace\s+\(k8s\.io/" go.mod || trueRepository: openshift/cluster-version-operator
Length of output: 6966
Resolve Ginkgo require vs replace contract mismatch in go.mod.
go.mod requires github.com/onsi/ginkgo/v2 v2.28.1 (lines 11-12) but overrides it with replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12 (line 115), so the effective Ginkgo version differs from the declared one.
-
Prefer removing the
replaceif upstream v2.28.1 is intended, or move thereplacetarget to anopenshift/onsi-ginkgocommit known to be API-compatible with v2.28.x. -
Direct
k8s.io/*deps are skewed betweenv0.35.1andv0.35.2(e.g.,k8s.io/api v0.35.2vsk8s.io/apiserver v0.35.1); keep this only if the repo’s compatibility policy/tests validate it.
Suggested fix
- replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12
+ // Option A: remove replace if upstream v2.28.1 is intended
+ // Option B: move replace target to an openshift/onsi-ginkgo commit that is API-compatible with v2.28.x🤖 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 `@go.mod` around lines 11 - 12, go.mod declares github.com/onsi/ginkgo/v2
v2.28.1 but also contains a replace directive pointing github.com/onsi/ginkgo/v2
=> github.com/openshift/onsi-ginkgo/v2 v2.6.1-0... which creates a version
mismatch; either remove the replace to use upstream v2.28.1 or update the
replace target to an openshift/onsi-ginkgo commit that is API-compatible with
v2.28.x so the required and replaced versions align, and similarly reconcile
k8s.io/* versions (e.g., k8s.io/api vs k8s.io/apiserver) so their minor/patch
versions are consistent per project compatibility policy and tests.
Comment on lines
+171
to
+196
| func (o *Options) CreateOverrides() error { | ||
| // If no overrides, return nil (central profile or defaults will be used) | ||
| if o.MinVersionOverride == "" && len(o.CipherSuitesOverride) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| validated := &Settings{} | ||
|
|
||
| if o.MinVersionOverride != "" { | ||
| minVersion, err := cliflag.TLSVersion(o.MinVersionOverride) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid --tls-min-version %q: %w (valid values: %v)", o.MinVersionOverride, err, cliflag.TLSPossibleVersions()) | ||
| } | ||
| validated.MinVersion = minVersion | ||
| } | ||
|
|
||
| if len(o.CipherSuitesOverride) > 0 { | ||
| cipherSuites, err := cliflag.TLSCipherSuites(o.CipherSuitesOverride) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid --tls-cipher-suites: %w", err) | ||
| } | ||
| validated.CipherSuites = cipherSuites | ||
| } | ||
|
|
||
| o.settings = validated | ||
| return nil |
There was a problem hiding this comment.
Reset cached overrides before each parse.
CreateOverrides preserves o.settings from the last successful call. If the same Options instance is validated again after clearing flags, or after introducing an invalid value, GetOverrides() still returns the old overrides and ProfileManager can apply settings the caller no longer requested.
Proposed fix
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
}
validated := &Settings{}📝 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
| func (o *Options) CreateOverrides() error { | |
| // If no overrides, return nil (central profile or defaults will be used) | |
| if o.MinVersionOverride == "" && len(o.CipherSuitesOverride) == 0 { | |
| return nil | |
| } | |
| validated := &Settings{} | |
| if o.MinVersionOverride != "" { | |
| minVersion, err := cliflag.TLSVersion(o.MinVersionOverride) | |
| if err != nil { | |
| return fmt.Errorf("invalid --tls-min-version %q: %w (valid values: %v)", o.MinVersionOverride, err, cliflag.TLSPossibleVersions()) | |
| } | |
| validated.MinVersion = minVersion | |
| } | |
| if len(o.CipherSuitesOverride) > 0 { | |
| cipherSuites, err := cliflag.TLSCipherSuites(o.CipherSuitesOverride) | |
| if err != nil { | |
| return fmt.Errorf("invalid --tls-cipher-suites: %w", err) | |
| } | |
| validated.CipherSuites = cipherSuites | |
| } | |
| o.settings = validated | |
| return nil | |
| 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 | |
| } | |
| validated := &Settings{} | |
| if o.MinVersionOverride != "" { | |
| minVersion, err := cliflag.TLSVersion(o.MinVersionOverride) | |
| if err != nil { | |
| return fmt.Errorf("invalid --tls-min-version %q: %w (valid values: %v)", o.MinVersionOverride, err, cliflag.TLSPossibleVersions()) | |
| } | |
| validated.MinVersion = minVersion | |
| } | |
| if len(o.CipherSuitesOverride) > 0 { | |
| cipherSuites, err := cliflag.TLSCipherSuites(o.CipherSuitesOverride) | |
| if err != nil { | |
| return fmt.Errorf("invalid --tls-cipher-suites: %w", err) | |
| } | |
| validated.CipherSuites = cipherSuites | |
| } | |
| o.settings = validated | |
| return nil | |
| } |
🤖 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 171 - 196, CreateOverrides currently preserves
o.settings from prior runs when there are no overrides or when validation later
fails; to fix, clear the cached overrides at the start of CreateOverrides by
setting o.settings = nil (or otherwise resetting the cached Settings) so each
call recomputes from flags; keep the rest of the validation logic as-is
(references: CreateOverrides method and the o.settings field, also
GetOverrides/ProfileManager which read o.settings).
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.
/hold
Summary by CodeRabbit
New Features
--tls-min-versionand--tls-cipher-suitesfor configurable TLS overrides.Tests
Chores