OTA-1836: Honor the centralized TLS configuration#1338
Open
DavidHurta wants to merge 16 commits into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/test ? |
|
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:
WalkthroughAdds APIServer-driven TLS profile management for the metrics server: new pkg/tls ProfileManager, CLI overrides, operator informer wiring, metrics integration applying profile+overrides per-handshake, tests, and dependency bumps. ChangesAPIServer-driven TLS Profile for Metrics
Sequence DiagramsequenceDiagram
participant OperatorStart as Operator Startup
participant APIServerInf as APIServer Informer
participant ProfileMgr as TLS ProfileManager
participant MetricsServer as Metrics Server
participant ClientTLS as Client TLS Handshake
OperatorStart->>APIServerInf: create and sync informer
OperatorStart->>ProfileMgr: NewProfileManager(informer, overrides)
ProfileMgr->>APIServerInf: register add/update/delete handlers
ProfileMgr->>ProfileMgr: resolve initial TLS profile
OperatorStart->>MetricsServer: RunMetrics(informer)
ClientTLS->>MetricsServer: initiate connection
MetricsServer->>ProfileMgr: ApplySettings(tls.Config)
ProfileMgr->>ProfileMgr: apply secure defaults -> central profile -> overrides
MetricsServer-->>ClientTLS: TLS config ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/test e2e-agnostic-operator |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/start/start.go (1)
358-363:⚠️ Potential issue | 🟠 MajorWait for the APIServer informer before starting metrics.
RunMetricsnow reads the TLS profile throughcontrollerCtx.CVO.APIServerLister(), but this path still only blocks onClusterVersionInformerFactory.WaitForCacheSyncabove. The APIServer informer is created later inNewControllerContext, so on a fresh leader transition its cache can still be cold here and the first handshakes will race an empty/NotFound profile. Please gate metrics startup on the APIServer informer, or the fullConfigInformerFactory, being synced whenRespectCentralTLSProfileis enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/start/start.go` around lines 358 - 363, The metrics server is started without ensuring the APIServer informer (or the full ConfigInformerFactory) has synced, causing races when RunMetrics reads TLS profiles via controllerCtx.CVO.APIServerLister(); update the metrics startup to, when RespectCentralTLSProfile is enabled, wait for the APIServer informer (or controllerCtx.ConfigInformerFactory) to be synced before spawning the goroutine that calls cvo.RunMetrics (check o.MetricsOptions.ListenAddress and o.RespectCentralTLSProfile, then call the appropriate WaitForCacheSync on the APIServer informer or ConfigInformerFactory from controllerCtx and only start the metrics goroutine after that returns true).pkg/cvo/metrics.go (2)
353-360:⚠️ Potential issue | 🟠 MajorReject missing
apiServerListerat startup.If
RespectCentralTLSProfileis true andapiServerListeris nil, the process won't fail until Line 485, where the first handshake dereferences it. Please turn that into an early configuration error.Suggested fix
func RunMetrics(runContext context.Context, shutdownContext context.Context, restConfig *rest.Config, apiServerLister configlistersv1.APIServerLister, options MetricsOptions) error { if options.ListenAddress == "" { return errors.New("listen address is required to serve metrics") } if options.DisableAuthentication && !options.DisableAuthorization { return errors.New("invalid configuration: cannot enable authorization without authentication") } + if options.RespectCentralTLSProfile && apiServerLister == nil { + return errors.New("apiServerLister is required when RespectCentralTLSProfile is enabled") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/metrics.go` around lines 353 - 360, Add an early validation in RunMetrics to reject a nil apiServerLister when options.RespectCentralTLSProfile is true: after the existing ListenAddress and auth checks, check if options.RespectCentralTLSProfile && apiServerLister == nil and return a clear configuration error (e.g. "apiServerLister is required when RespectCentralTLSProfile is true"). This prevents the later nil dereference in TLS handshake code that relies on apiServerLister.
467-490:⚠️ Potential issue | 🔴 CriticalSynchronize
lastValidProfilein the TLS callback.
lastValidProfileis captured byGetConfigForClientand then read and overwritten on each handshake without synchronization. That is a data race, and concurrent handshakes can also write back an older snapshot after a profile change.Suggested fix
import ( "context" "crypto/tls" "crypto/x509" "errors" "fmt" "net" "net/http" "slices" + "sync" "time" @@ - var lastValidProfile *cachedTLSProfile + var ( + lastValidProfile *cachedTLSProfile + lastValidProfileMu sync.Mutex + ) @@ if options.RespectCentralTLSProfile { + lastValidProfileMu.Lock() profile, err := getAPIServerTLSProfile(apiServerLister, lastValidProfile) + if err == nil { + lastValidProfile = profile + } + lastValidProfileMu.Unlock() if err != nil { return nil, fmt.Errorf("failed to get TLS profile for metrics server: %w", err) } - lastValidProfile = profile profile.apply(config) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/metrics.go` around lines 467 - 490, The TLS callback captures and mutates lastValidProfile unsafely; add synchronization (e.g., a package-local sync.RWMutex like lastValidProfileMu) and use RLock when reading and Lock when updating to prevent data races and stale overwrites in GetConfigForClient; wrap the call to getAPIServerTLSProfile and the assignment lastValidProfile = profile (and the subsequent profile.apply(config) if it relies on the stored state) inside the mutex so readers/writers are serialized and the cachedTLSProfile is updated atomically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cvo/metrics.go`:
- Around line 176-180: The log at the klog.Infof call prints profile.Ciphers
using %s which causes invalid formatting; update the klog.Infof in the TLS
profile change block (after tlsprofile.NewTLSConfigFromProfile and the
unsupportedCiphers check) to format the cipher list correctly by using a verbs
that match the type (e.g., %v) or join the slice into a string (e.g.,
strings.Join(profile.Ciphers, ",")). Ensure you import strings if you choose
Join and keep the rest of the message unchanged.
---
Outside diff comments:
In `@pkg/cvo/metrics.go`:
- Around line 353-360: Add an early validation in RunMetrics to reject a nil
apiServerLister when options.RespectCentralTLSProfile is true: after the
existing ListenAddress and auth checks, check if
options.RespectCentralTLSProfile && apiServerLister == nil and return a clear
configuration error (e.g. "apiServerLister is required when
RespectCentralTLSProfile is true"). This prevents the later nil dereference in
TLS handshake code that relies on apiServerLister.
- Around line 467-490: The TLS callback captures and mutates lastValidProfile
unsafely; add synchronization (e.g., a package-local sync.RWMutex like
lastValidProfileMu) and use RLock when reading and Lock when updating to prevent
data races and stale overwrites in GetConfigForClient; wrap the call to
getAPIServerTLSProfile and the assignment lastValidProfile = profile (and the
subsequent profile.apply(config) if it relies on the stored state) inside the
mutex so readers/writers are serialized and the cachedTLSProfile is updated
atomically.
In `@pkg/start/start.go`:
- Around line 358-363: The metrics server is started without ensuring the
APIServer informer (or the full ConfigInformerFactory) has synced, causing races
when RunMetrics reads TLS profiles via controllerCtx.CVO.APIServerLister();
update the metrics startup to, when RespectCentralTLSProfile is enabled, wait
for the APIServer informer (or controllerCtx.ConfigInformerFactory) to be synced
before spawning the goroutine that calls cvo.RunMetrics (check
o.MetricsOptions.ListenAddress and o.RespectCentralTLSProfile, then call the
appropriate WaitForCacheSync on the APIServer informer or ConfigInformerFactory
from controllerCtx and only start the metrics goroutine after that returns
true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b2ebb59-479e-4c67-b71f-fec6dbcfc90e
⛔ Files ignored due to path filters (291)
go.sumis excluded by!**/*.sumvendor/github.com/evanphx/json-patch/v5/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/decode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/encode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fold.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fuzz.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/indent.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/scanner.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tables.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tags.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/evanphx/json-patch/v5/patch.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!**/vendor/**,!vendor/**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/internal/assertion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.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/have_len_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/match_error_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/match_json_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/manifest/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/api/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/desc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/internal/difflib.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/internal/go_runtime_metrics.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/labels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/metric.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/process_collector_darwin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/process_collector_mem_nocgo_darwin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/process_collector_procfsenabled.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/promhttp/instrument_server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/vec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/client_golang/prometheus/wrap.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/config/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/config/headers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/config/http_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/decode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/encode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/expfmt.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/fuzz.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/openmetrics_create.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/text_create.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/expfmt/text_parse.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/alert.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/labels.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/labelset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/metric.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/time.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value_histogram.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/common/model/value_type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/Makefile.commonis excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/arp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/fs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/fs_statfs_notype.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/fscache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/internal/fs/fs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/internal/util/parse.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/internal/util/sysreadfile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/mountstats.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/net_dev_snmp6.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/net_ip_socket.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/net_protocols.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/net_tcp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/net_unix.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_cgroup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_netstat.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_smaps.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_snmp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_snmp6.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_status.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/proc_sys.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/prometheus/procfs/softirqs.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/bool_func.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/count.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/flag.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/func.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/golangflag.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/ipnet_slice.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/string_to_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/text.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/spf13/pflag/time.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/oauth2/clientcredentials/clientcredentials.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/internal/doc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/internal/oauth2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/internal/token.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/internal/transport.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/oauth2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/pkce.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/token.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/transport.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/google.golang.org/protobuf/encoding/protowire/wire.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/editiondefaults/editions_defaults.binpbis excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/presence.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/api_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_message_opaque.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/message_opaque.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/presence.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/strs/strings_unsafe.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/strs/strings_unsafe_go120.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/merge.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/source_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/value_unsafe.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protoreflect/value_unsafe_go120.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/gofeaturespb/go_features.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/anypb/any.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/durationpb/duration.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/emptypb/empty.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/structpb/struct.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/wrapperspb/wrappers.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/LICENSE.libyamlis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/apic.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/decode.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/yaml.v2/emitterc.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
go.modlib/manifest/manifest.gopkg/cvo/cvo.gopkg/cvo/featuregate_integration_test.gopkg/cvo/metrics.gopkg/cvo/sync_worker.gopkg/payload/payload.gopkg/payload/render.gopkg/start/start.go
openshift-merge-robot
added
the
needs-rebase
DavidHurta
force-pushed
the
central-tls-profile
branch
from
3f595e0 to
1362d3f
Compare
openshift-merge-robot
removed
the
needs-rebase
DavidHurta
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@DavidHurta: This pull request references OTA-1764 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
DavidHurta
force-pushed
the
central-tls-profile
branch
2 times, most recently
from
9990e0a to
f9d748d
Compare
Contributor
Author
|
/test all |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cvo/metrics.go (1)
164-170: Remove or clarify the dead code block.The
if falseblock with the TODO comment is unreachable and will never execute. If this is a placeholder for future API changes, consider using a build tag or feature flag instead, or remove it entirely until the API changes are ready.♻️ Suggested approach
Either remove the dead code block entirely:
- if false { // TODO: Add TLS adherence logic when API changes merge - return &cachedTLSProfile{ - spec: nil, - apply: func(config *tls.Config) {}, // do nothing - generation: apiServer.Generation, - }, nil - } -Or if keeping it as a placeholder, add a clearer skip mechanism:
+ // TODO(OTA-XXXX): Add TLS adherence logic when API changes merge + // When implemented, check apiServer.Spec.TLSSecurityProfile adherence field here🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/metrics.go` around lines 164 - 170, The unreachable "if false" block should be removed or replaced with a proper toggle; delete the dead branch that returns a cachedTLSProfile with spec nil/apply noop (references: cachedTLSProfile, apiServer.Generation, tls.Config) or, if you need a placeholder, replace it with a clear feature flag/build-tag guard (not "if false") and document the intent; ensure any retained placeholder still compiles and preserves the intended return shape for the surrounding function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 29-36: The go.mod shows a mismatch: most k8s modules are v0.35.1
while k8s.io/kube-aggregator remains v0.34.1; verify compatibility and align
versions by either upgrading k8s.io/kube-aggregator to a v0.35.x release if
available, or downgrade the other k8s modules to the kube-aggregator compatible
version (e.g., v0.34.x), or add a go.mod replace to force a tested compatible
commit; update the k8s module entries (k8s.io/kube-aggregator and the other
k8s.io/* lines) so all Kubernetes component modules use a mutually compatible
version set and run go mod tidy and go test to ensure no API/type mismatches.
- Line 3: The go.mod contains an invalid Go version "go 1.25.0"; update the
module's go directive to a valid released version (e.g., "go 1.25.8" or "go
1.26.1") by replacing the current go directive value so the go directive in
go.mod reflects an actual Go release.
---
Nitpick comments:
In `@pkg/cvo/metrics.go`:
- Around line 164-170: The unreachable "if false" block should be removed or
replaced with a proper toggle; delete the dead branch that returns a
cachedTLSProfile with spec nil/apply noop (references: cachedTLSProfile,
apiServer.Generation, tls.Config) or, if you need a placeholder, replace it with
a clear feature flag/build-tag guard (not "if false") and document the intent;
ensure any retained placeholder still compiles and preserves the intended return
shape for the surrounding function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b19eb2ef-b766-4f59-b7e5-9d3710e2fd62
⛔ Files ignored due to path filters (113)
go.sumis excluded by!**/*.sumvendor/github.com/evanphx/json-patch/v5/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/errors.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fold.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/fuzz.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/indent.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/scanner.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tables.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/internal/json/tags.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/merge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/evanphx/json-patch/v5/patch.gois excluded by!vendor/**,!**/vendor/**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/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.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/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_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/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/edge/edge.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
go.modlib/manifest/manifest.gopkg/cvo/cvo.gopkg/cvo/featuregate_integration_test.gopkg/cvo/metrics.gopkg/cvo/sync_worker.gopkg/payload/payload.gopkg/payload/render.gopkg/start/start.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cvo/featuregate_integration_test.go
- lib/manifest/manifest.go
- pkg/payload/render.go
DavidHurta
force-pushed
the
central-tls-profile
branch
from
f9d748d to
8553636
Compare
Contributor
Author
|
/test ? |
Contributor
Author
|
/test tls-scanner |
DavidHurta
force-pushed
the
central-tls-profile
branch
from
8553636 to
7b9c98f
Compare
Contributor
Author
|
/test tls-scanner |
Contributor
Author
|
/test tls-scanner |
DavidHurta
changed the title
DavidHurta
force-pushed
the
central-tls-profile
branch
from
70099c9 to
29e1401
Compare
Contributor
Author
|
/test tls-scanner |
Draft
4 tasks
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>
DavidHurta
force-pushed
the
central-tls-profile
branch
from
b60de52 to
b9518b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cvo/metrics.go (1)
279-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
applySettingsis nil.
GetConfigForClientcalls the callback unconditionally. A nil callback turns the first scrape into a process panic instead of a startup error.Suggested fix
func RunMetrics(runContext context.Context, shutdownContext context.Context, restConfig *rest.Config, applySettings func(config *tls.Config), options MetricsOptions) error { + if applySettings == nil { + return errors.New("applySettings is required") + } + if options.ListenAddress == "" { return errors.New("listen address is required to serve metrics") }Also applies to: 392-405
🤖 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/metrics.go` around lines 279 - 286, RunMetrics must fail fast if applySettings is nil because GetConfigForClient calls that callback unconditionally; add a nil-check at the start of RunMetrics (and the other initialization path noted around lines 392-405) and return an explicit error like "applySettings callback is required" when applySettings == nil so startup errors occur instead of a panic; update both the RunMetrics function and the other initialization site that constructs a client via GetConfigForClient to validate the callback before proceeding, referencing the applySettings parameter and the GetConfigForClient call sites.
🤖 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 178-205: The method CreateOverrides leaves o.settings intact on
the early-return path, causing stale overrides to persist; modify
CreateOverrides (on type Options) to clear the cached overrides before returning
when no overrides are configured (e.g., set o.settings = nil before the early
return) so that a reused Options instance does not retain previous TLS settings;
keep the existing parsing and assignment to o.settings = validated for the
non-empty cases.
---
Outside diff comments:
In `@pkg/cvo/metrics.go`:
- Around line 279-286: RunMetrics must fail fast if applySettings is nil because
GetConfigForClient calls that callback unconditionally; add a nil-check at the
start of RunMetrics (and the other initialization path noted around lines
392-405) and return an explicit error like "applySettings callback is required"
when applySettings == nil so startup errors occur instead of a panic; update
both the RunMetrics function and the other initialization site that constructs a
client via GetConfigForClient to validate the callback before proceeding,
referencing the applySettings parameter and the GetConfigForClient call sites.
🪄 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: b4079a36-1e0c-4307-bbb8-73f1af317b83
📒 Files selected for processing (6)
cmd/cluster-version-operator/start.gopkg/cvo/cvo.gopkg/cvo/metrics.gopkg/start/start.gopkg/tls/tls.gopkg/tls/tls_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cluster-version-operator/start.go
- pkg/tls/tls_test.go
Comment on lines
+178
to
+205
| // CreateOverrides creates the TLS override options and returns parsed values. | ||
| // Returns nil if no overrides are configured. | ||
| 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 reparsing.
CreateOverrides() leaves o.settings untouched on the early-return path, so a reused Options instance keeps applying the previous TLS overrides after the flags are cleared. Reset the cached settings before parsing so “no overrides” really becomes nil.
Suggested 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
}
@@
- o.settings = validated
+ o.settings = validated
return nil
}📝 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
| // CreateOverrides creates the TLS override options and returns parsed values. | |
| // Returns nil if no overrides are configured. | |
| 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 | |
| // CreateOverrides creates the TLS override options and returns parsed values. | |
| // Returns nil if no overrides are configured. | |
| 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 178 - 205, The method CreateOverrides leaves
o.settings intact on the early-return path, causing stale overrides to persist;
modify CreateOverrides (on type Options) to clear the cached overrides before
returning when no overrides are configured (e.g., set o.settings = nil before
the early return) so that a reused Options instance does not retain previous TLS
settings; keep the existing parsing and assignment to o.settings = validated for
the non-empty cases.
Contributor
Author
|
I have tried to make your review more straightforward. I have directly cherry-picked your commits (only minor merge conflict resolution needed for the Furthermore, I have added a new 97a5bd2 commit to resolve that one minor mentioned issue. PTAL. Please let me know if the description is not clear enough or is incorrect. I am happy to follow up. |
Member
Thanks for catching it up. It saved us lots of troubles of fixing it. I should not have made that mistake in the first place given that it took me 2 hours to fix the same issue in test. apiServerInformer.Lister() // we have to call this before Start |
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta, 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 |
Member
|
/retest-required |
Member
The above from 97a5bd2 Yesterday I thought about it and led to #1338 (comment) but now I am thinking maybe the claim "the lister to be empty" is not true. Did you get the conclusion by testing it on a cluster? I am going to do some testing otherwise. This is NOT a blocker to merge the current pull. I can bring this up with a follow up pull if needed. |
Member
|
share some e2e testing result: #1393 (comment) It shows the e2e running against the current pull. |
Contributor
Author
No testing. Static analysis (prone to misunderstanding the code, of course). The wording of the commit could be more technically correct. It is a data race issue. It may or may not occur depending on when informers are started and when the caches sync. The CVO can sometimes make these things (initialization, passing down only the needed structs, etc.) a bit awkward due to its architecture. Thus, why I always double-check my code in this area. Previously, the profile manager wanted to fetch the APIServer before the underlying informer was even started. Start is described as: Thus, here we should get a clear error. I have checked the past CI, and we can see such CVO logs in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1393/pull-ci-openshift-cluster-version-operator-main-e2e-agnostic-operator/2058900598811529216/artifacts/e2e-agnostic-operator/gather-extra/artifacts/pods/openshift-cluster-version_cluster-version-operator-867dc65658-nxxj2_cluster-version-operator.log: That's a CI run for 6a8c607 from #1393, which at the time contained the CVO changes, if I understand correctly. |
Member
Great. You have a solid proof from CI. I will dig a bit about it (still no blockers for this pull). |
Contributor
Author
|
/test tls-scanner-modern-profile |
Contributor
Author
|
/unhold Intentionally not marking as verified at the moment. |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
|
@DavidHurta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
hongkailiu
added a commit
to hongkailiu/cluster-version-operator
that referenced
this pull request
Jun 1, 2026
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`.
hongkailiu
added a commit
to hongkailiu/cluster-version-operator
that referenced
this pull request
Jun 1, 2026
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.
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
4 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.
Honor the centralized TLS configuration.
A follow-up PR openshift/hypershift#8013 after this one merges.
Summary by CodeRabbit
New Features
Tests
Chores