⚠️ refactor(grpc): change TLSMinVersion/TLSMaxVersion from uint16 to string - #220
Conversation
…string Change TLSMinVersion and TLSMaxVersion from uint16 to string in GRPCServerOptions, so config files use human-readable names like "VersionTLS12" instead of raw numeric constants like 771. The string values are parsed to uint16 by Validate() using pkgtls.ParseTLSVersion, keeping the runtime behavior identical. ApplyTLSFlags is simplified to just set the string directly. Signed-off-by: Jia Zhu <jiazhu@redhat.com> Signed-off-by: zhujian <jiazhu@redhat.com>
WalkthroughTLS version fields in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@zhujian7: GitHub didn't allow me to request PR reviews from the following users: ncr38. Note that only open-cluster-management-io members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/server/grpc/options_test.go (1)
59-60: Please cover the max-version/config-file parse path too.The updated assertions only lock down
tlsMinVersion, and the load test still gets the TLS versions from defaults rather than YAML. Add one case with explicittls_min_version/tls_max_versionin the config and assert bothtlsMinVersionandtlsMaxVersionso the new parser-backed path is exercised.Also applies to: 323-325, 349-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/grpc/options_test.go` around lines 59 - 60, Add a test case that exercises the parser-backed config path by providing a YAML config with explicit tls_min_version and tls_max_version and asserting both TLSMinVersion and TLSMaxVersion on the resulting options; locate the existing tests that assert TLSMinVersion/TLSMaxVersion (the entries using TLSMinVersion and TLSMaxVersion) and add a parallel case which loads the YAML, calls the same config-load function used elsewhere in options_test.go, and verifies both fields match the provided values (this should also be mirrored for the other failing locations noted around the other test cases).
🤖 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/server/grpc/options.go`:
- Around line 38-40: The GRPCServerOptions constructor leaves the parsed TLS
cache fields tlsMinVersion and tlsMaxVersion at zero, but Validate() and runtime
use those fields as the source of truth; update NewGRPCServerOptions to seed
tlsMinVersion and tlsMaxVersion from the default MinVersion/MaxVersion (or
appropriate TLS constants) so the returned GRPCServerOptions is fully
initialized. Locate NewGRPCServerOptions and GRPCServerOptions, assign parsed
tlsMinVersion/tlsMaxVersion to their default parsed values (matching whatever
MinVersion/MaxVersion defaults you already set) and mirror the same change for
the other parsed cache fields noted around lines 72-73 so all parsed caches are
initialized consistently before any Validate() call.
- Around line 21-22: The TLSMinVersion and TLSMaxVersion fields were changed to
strings but existing configs may supply numeric uint16 values; update the config
loading path to accept numeric inputs and convert them to the expected string
form (or to the internal numeric form ParseTLSVersion consumes) to preserve
backward compatibility. Specifically, modify the options
unmarshalling/validation logic that reads TLSMinVersion/TLSMaxVersion (the
struct fields TLSMinVersion and TLSMaxVersion and the function ParseTLSVersion)
to detect numeric values (e.g., json/yaml numbers or strings containing digits),
convert them to the equivalent named/version string or pass the numeric value
through to ParseTLSVersion, and add unit tests for numeric and named inputs;
alternatively add clear migration error messages recommending the new string
format if conversion is not possible.
---
Nitpick comments:
In `@pkg/server/grpc/options_test.go`:
- Around line 59-60: Add a test case that exercises the parser-backed config
path by providing a YAML config with explicit tls_min_version and
tls_max_version and asserting both TLSMinVersion and TLSMaxVersion on the
resulting options; locate the existing tests that assert
TLSMinVersion/TLSMaxVersion (the entries using TLSMinVersion and TLSMaxVersion)
and add a parallel case which loads the YAML, calls the same config-load
function used elsewhere in options_test.go, and verifies both fields match the
provided values (this should also be mirrored for the other failing locations
noted around the other test cases).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ce67e64-640d-40fb-9bcf-0abc43cb7094
📒 Files selected for processing (3)
pkg/server/grpc/options.gopkg/server/grpc/options_test.gopkg/server/grpc/server.go
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhujian7 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 |
883108b
into
open-cluster-management-io:main
3 participants
Summary
TLSMinVersionandTLSMaxVersionfromuint16tostringinGRPCServerOptions, so config files use human-readable names (e.g."VersionTLS12") instead of raw numeric constants (e.g.771)uint16internally byValidate()usingpkgtls.ParseTLSVersion, keeping runtime behavior identicalApplyTLSFlagssimplified — just sets the string directly, validation deferred toValidate()Related issue(s)
Follow-up to #219 (comment: #219 (comment))
Summary by CodeRabbit
Release Notes