Conversation
|
Skipping CI for Draft Pull Request. |
|
/test ? |
WalkthroughThe PR adds dynamic TLS profile handling to the metrics server by integrating APIServerLister throughout the codebase. The manifest.Include() method signature is extended with an additional parameter, requiring updates across multiple call sites. The metrics server now optionally fetches, caches, and applies TLS profiles from the APIServer resource when the RespectCentralTLSProfile flag is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant TLS as TLS Handshake
participant Metrics as Metrics Server
participant Cache as TLS Profile Cache
participant Lister as APIServerLister
participant APIServer as APIServer Resource
TLS->>Metrics: GetConfigForClient()
alt RespectCentralTLSProfile enabled
Metrics->>Lister: Get APIServer resource
Lister->>APIServer: Fetch current APIServer
APIServer-->>Lister: Return resource
Lister-->>Metrics: Return APIServer spec
Metrics->>Metrics: Extract TLS profile
Metrics->>Cache: Compare with cached profile
alt Profile changed or first time
Metrics->>Metrics: Apply new TLS config
Metrics->>Cache: Store new profile
Metrics-->>TLS: Return updated config
else Profile unchanged
Metrics-->>TLS: Return cached config
end
else Use fallback
Metrics->>Cache: Fetch last valid profile
Cache-->>Metrics: Return cached profile
Metrics-->>TLS: Return config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta 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 |
|
/test e2e-agnostic-operator |
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
| applyTLSProfile, unsupportedCiphers := tlsprofile.NewTLSConfigFromProfile(profile) | ||
| if len(unsupportedCiphers) > 0 { | ||
| klog.Warningf("TLS profile contains unsupported ciphers (will be ignored): %v", unsupportedCiphers) | ||
| } | ||
| klog.Infof("TLS profile changed to: MinTLSVersion=%s, Ciphers=%s", profile.MinTLSVersion, profile.Ciphers) |
There was a problem hiding this comment.
Fix the cipher list log formatting.
Line 180 formats profile.Ciphers with %s, so this log will print %!s(...) instead of the configured cipher list.
Suggested fix
- klog.Infof("TLS profile changed to: MinTLSVersion=%s, Ciphers=%s", profile.MinTLSVersion, profile.Ciphers)
+ klog.Infof("TLS profile changed to: MinTLSVersion=%s, Ciphers=%v", profile.MinTLSVersion, profile.Ciphers)📝 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.
| applyTLSProfile, unsupportedCiphers := tlsprofile.NewTLSConfigFromProfile(profile) | |
| if len(unsupportedCiphers) > 0 { | |
| klog.Warningf("TLS profile contains unsupported ciphers (will be ignored): %v", unsupportedCiphers) | |
| } | |
| klog.Infof("TLS profile changed to: MinTLSVersion=%s, Ciphers=%s", profile.MinTLSVersion, profile.Ciphers) | |
| applyTLSProfile, unsupportedCiphers := tlsprofile.NewTLSConfigFromProfile(profile) | |
| if len(unsupportedCiphers) > 0 { | |
| klog.Warningf("TLS profile contains unsupported ciphers (will be ignored): %v", unsupportedCiphers) | |
| } | |
| klog.Infof("TLS profile changed to: MinTLSVersion=%s, Ciphers=%v", profile.MinTLSVersion, profile.Ciphers) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cvo/metrics.go` around lines 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.
|
@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. |
Summary by CodeRabbit
New Features
Chores