Skip to content

Log unknown adherence values and add support for legacy adhering components to ShouldHonorClusterTLSProfile #2308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
richardsonnick wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −1
Open

Log unknown adherence values and add support for legacy adhering components to ShouldHonorClusterTLSProfile #2308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pkg/crypto/tls_adherence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package crypto

import (
configv1 "github.com/openshift/api/config/v1"
"k8s.io/klog/v2"
)

// ShouldHonorClusterTLSProfile returns true if the component should honor the
Expand All @@ -13,10 +14,15 @@ import (
//
// Unknown enum values are treated as StrictAllComponents for forward compatibility
// and to default to the more secure behavior.
func ShouldHonorClusterTLSProfile(tlsAdherence configv1.TLSAdherencePolicy) bool {
func ShouldHonorClusterTLSProfile(tlsAdherence configv1.TLSAdherencePolicy, isLegacyAdheringComponent bool, logger klog.Logger) bool {

@coderabbitai coderabbitai Bot Jun 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Signature change at Line 17 breaks existing callers unless updated in the same PR.

ShouldHonorClusterTLSProfile now requires 3 args, but the provided pkg/crypto/tls_adherence_test.go snippet still calls it with one argument (ShouldHonorClusterTLSProfile(tt.tlsAdherence)), which will fail to compile until call sites are updated (or a compatibility wrapper is added).

🤖 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/crypto/tls_adherence.go` at line 17, The function signature for
ShouldHonorClusterTLSProfile in pkg/crypto/tls_adherence.go has been changed to
accept three parameters (tlsAdherence, isLegacyAdheringComponent, and logger),
but the test file pkg/crypto/tls_adherence_test.go still calls it with only one
argument. Update all call sites to ShouldHonorClusterTLSProfile to pass all
three required arguments: the TLSAdherencePolicy, the boolean flag for legacy
adherence, and the klog.Logger instance. Check both the test file and any other
files that call this function to ensure consistency.

if isLegacyAdheringComponent {
return true
}
switch tlsAdherence {
case configv1.TLSAdherencePolicyNoOpinion, configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly:
return false
case configv1.TLSAdherencePolicyStrictAllComponents:
return true
default:
return true
Comment on lines 26 to 27

@coderabbitai coderabbitai Bot Jun 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unknown policy path still does not log, so the new logger parameter is effectively unused.

Line 26-27 defaults to true for forward compatibility, but it should also emit a log entry for unknown enum values to satisfy the stated observability objective.

Suggested patch 
 default:
+    logger.Info("unknown TLSAdherencePolicy value; defaulting to strict behavior", "tlsAdherence", tlsAdherence)
     return true
📝 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
default:
return true
default:
logger.Info("unknown TLSAdherencePolicy value; defaulting to strict behavior", "tlsAdherence", tlsAdherence)
return true
🤖 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/crypto/tls_adherence.go` around lines 26 - 27, The default case in the
switch statement (lines 26-27) currently returns true for forward compatibility
but does not use the logger parameter to emit an observability log entry for
unknown policy enum values. Add a log statement in the default case using the
logger parameter to record when an unknown policy path is encountered, then
return true. This ensures the new logger parameter is actually utilized and
provides the observability coverage for unrecognized enum values as intended.

}
Expand Down