Conversation
|
Hi @nunnatsa. Thanks for your PR. I'm waiting for a kubevirt-ui member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughIntroduces a new config package that parses TLS-related flags into a typed Config (with TLSConfig) and refactors main to load and apply TLS settings from that package; adds unit tests and expands CI test scope to all subpackages. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main
participant Config as Config Package
participant Server as Server
Main->>Config: GetConfig()
Config-->>Main: Config (minVersion, cipherSuites) / error
alt config OK
Main->>Server: set TLSConfig.MinVersion = cfg.GetMinTLSVersion()
Main->>Server: set TLSConfig.CipherSuites = cfg.GetTLSCipherSuites()
Main->>Server: start server
else config error
Main-->>Main: log.Fatal(err)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 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
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 |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: metalice, nunnatsa 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 |
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 24-48: GetConfig currently casts *minTLSVersionFlag to uint16
without bounds checking which can truncate values >65535; add an explicit bounds
check in GetConfig: if *minTLSVersionFlag > math.MaxUint16 return a clear error
(or validate and clamp per project policy) before assigning to
cfg.TLS.minTLSVersion, then safely convert to uint16; reference GetConfig,
minTLSVersionFlag and cfg.TLS.minTLSVersion when making the change.
🧹 Nitpick comments (2)
config/config.go (2)
19-22: Consider documenting expected cipher format.The
tls-cipher-suitesflag expects numeric cipher IDs (e.g.,49195,49199), which may not be intuitive. Users might expect cipher names likeTLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256. Consider clarifying this in the usage string.📝 Suggested improvement
var ( minTLSVersionFlag = flag.Uint("tls-min-version", 0, "The minimum TLS version to use") - tlsCipherSuitesFlag = flag.String("tls-cipher-suites", "", "A comma-separated list of cipher suites to use") + tlsCipherSuitesFlag = flag.String("tls-cipher-suites", "", "A comma-separated list of cipher suite IDs (numeric) to use") )
35-42: Consider trimming whitespace from cipher strings.If users provide input like
"49195, 49199"(with spaces after commas), parsing will fail. Trimming whitespace improves usability.📝 Suggested improvement
for _, cipherStr := range ciphers { + cipherStr = strings.TrimSpace(cipherStr) cipher, err := strconv.ParseUint(cipherStr, 10, 16) if err != nil { return nil, fmt.Errorf("can't parse cipher %q; %w", cipherStr, err) } tlsCipherSuites = append(tlsCipherSuites, uint16(cipher)) }
|
New changes are detected. LGTM label has been removed. |
Move the TLS flags logic to the new config package. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
Move the TLS flags logic to the new
configpackage.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.