Log unknown adherence values and add support for legacy adhering components to ShouldHonorClusterTLSProfile#2308
Conversation
…ring components to ShouldHonorClusterTLSProfile
Walkthrough
ChangesTLS Adherence Policy Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: richardsonnick The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/crypto/tls_adherence.go`:
- 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.
- Around line 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.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b698934b-9e67-45a4-bb08-66fdfe935fc4
📒 Files selected for processing (1)
pkg/crypto/tls_adherence.go
| // 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 { |
There was a problem hiding this comment.
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.
| default: | ||
| return true |
There was a problem hiding this comment.
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.
| 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.
|
@richardsonnick: 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. |
|
Will fix ci errors |
1 participant
Adds logging for unknown
TLSAdherencePolicyenum values and add support for legacy components toShouldHonorClusterTLSProfileSummary by CodeRabbit