🐛 Expose serving option to flags#366
🐛 Expose serving option to flags#366qiujian16 wants to merge 1 commit intoopen-cluster-management-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 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 |
WalkthroughController factory updated to accept external SecureServingOptions: a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/cmd/factory/factory.go (2)
283-283: Self-signed cert only covers localhost access.The self-signed certificate is generated only for
"localhost"and127.0.0.1. If the server needs to be accessed via the pod IP, service DNS name, or external hostname, TLS verification will fail for those clients.Consider documenting this limitation or using the machine's actual hostname/IP when available.
272-274: Port defaulting changes behavior for--secure-port=0.Previously, setting port to 0 would let the OS assign a random available port (useful for testing or avoiding conflicts). Now it silently defaults to 8443, which could conflict with other services or surprise users expecting the standard "port 0 = random" behavior.
Consider either:
- Keeping the random port behavior when explicitly set to 0
- Documenting this default clearly in the flag help text
💡 Alternative: Preserve random port behavior
If the intent is to have a sensible default only when the flag is not explicitly provided, consider tracking whether the flag was explicitly set:
- if servingOptions.BindPort == 0 { - servingOptions.BindPort = 8443 - } + // Only default to 8443 if no port was configured at all. + // Port 0 explicitly set should still allow random assignment.Or update the flag default in
NewSecureServingOptions()customization rather than here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/factory/factory.go` around lines 272 - 274, The change forces a default of 8443 when servingOptions.BindPort == 0 which breaks the conventional "0 = assign random port" behavior; modify the logic so that you only set servingOptions.BindPort = 8443 when the flag was not explicitly provided (i.e., when the value is the flag's default), otherwise preserve an explicit 0 to allow the OS to pick a random port. Locate the BindPort handling in factory.go (servingOptions.BindPort) and either: a) detect whether the secure-port flag was explicitly set and only apply 8443 when not set, or b) move the default into NewSecureServingOptions() so the flag default reflects the desired value while an explicit 0 remains honored.
🤖 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/cmd/factory/factory.go`:
- Around line 276-286: The temp directory created when
servingOptions.ServerCert.CertDirectory is empty is never removed; add a cleanup
hook to remove servingOptions.ServerCert.CertDirectory on shutdown: after
creating temporaryCertDir and setting
servingOptions.ServerCert.CertDirectory/PairName, register a cleanup callback
tied to the server lifecycle or provided context (e.g., via a defer-style
cleanup list, shutdown handler, or context cancellation) that
os.RemoveAll(servingOptions.ServerCert.CertDirectory) when the server stops or
context is done; ensure the cleanup is registered in the same scope that creates
the dir (where MaybeDefaultWithSelfSignedCerts is called) and handle/remove
errors appropriately.
---
Nitpick comments:
In `@pkg/cmd/factory/factory.go`:
- Around line 272-274: The change forces a default of 8443 when
servingOptions.BindPort == 0 which breaks the conventional "0 = assign random
port" behavior; modify the logic so that you only set servingOptions.BindPort =
8443 when the flag was not explicitly provided (i.e., when the value is the
flag's default), otherwise preserve an explicit 0 to allow the OS to pick a
random port. Locate the BindPort handling in factory.go
(servingOptions.BindPort) and either: a) detect whether the secure-port flag was
explicitly set and only apply 8443 when not set, or b) move the default into
NewSecureServingOptions() so the flag default reflects the desired value while
an explicit 0 remains honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82e258e1-8f67-43bd-847d-3c37b8a41ca2
📒 Files selected for processing (1)
pkg/cmd/factory/factory.go
| if servingOptions.ServerCert.CertDirectory == "" { | ||
| temporaryCertDir, err := os.MkdirTemp("", "serving-cert") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| servingOptions.ServerCert.CertDirectory = temporaryCertDir | ||
| servingOptions.ServerCert.PairName = "tls" | ||
| if err := servingOptions.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Temporary certificate directory is never cleaned up.
The temp directory created at line 277 is not removed when the server shuts down, leading to accumulation of orphaned directories over time on systems with frequent restarts.
Consider registering cleanup via the context or a shutdown hook:
🧹 Proposed fix to register cleanup
func toServerConfig(servingOptions *genericapiserveroptions.SecureServingOptions) (*genericapiserver.Config, error) {
scheme := runtime.NewScheme()
metav1.AddToGroupVersion(scheme, metav1.SchemeGroupVersion)
config := genericapiserver.NewConfig(serializer.NewCodecFactory(scheme))
if servingOptions.BindPort == 0 {
servingOptions.BindPort = 8443
}
if servingOptions.ServerCert.CertDirectory == "" {
temporaryCertDir, err := os.MkdirTemp("", "serving-cert")
if err != nil {
return nil, err
}
+ // Note: Consider adding cleanup logic via config.AddPostStartHook or
+ // documenting that callers should clean up the cert directory on shutdown
servingOptions.ServerCert.CertDirectory = temporaryCertDir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/factory/factory.go` around lines 276 - 286, The temp directory
created when servingOptions.ServerCert.CertDirectory is empty is never removed;
add a cleanup hook to remove servingOptions.ServerCert.CertDirectory on
shutdown: after creating temporaryCertDir and setting
servingOptions.ServerCert.CertDirectory/PairName, register a cleanup callback
tied to the server lifecycle or provided context (e.g., via a defer-style
cleanup list, shutdown handler, or context cancellation) that
os.RemoveAll(servingOptions.ServerCert.CertDirectory) when the server stops or
context is done; ensure the cleanup is registered in the same scope that creates
the dir (where MaybeDefaultWithSelfSignedCerts is called) and handle/remove
errors appropriately.
Signed-off-by: Jian Qiu <jqiu@redhat.com>
qiujian16
force-pushed
the
expose-serving-flag
branch
from
d70f9ac to
e995924
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/factory/factory.go (1)
283-283: PairName is unconditionally overwritten.Line 283 always sets
PairName = "tls", which could override a user-provided value via flags. While this likely doesn't affect users who provide explicit cert/key file paths (sincePairNameis primarily used for auto-generated cert file naming), it may cause unexpected behavior if a user configures only theCertDirectoryexpecting a different naming convention.Consider preserving user-provided values:
💡 Proposed fix to preserve user-provided PairName
- servingOptions.ServerCert.PairName = "tls" + if servingOptions.ServerCert.PairName == "" { + servingOptions.ServerCert.PairName = "tls" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/factory/factory.go` at line 283, The code unconditionally sets servingOptions.ServerCert.PairName = "tls", which overwrites any user-provided value; change the logic in the factory where PairName is set (referencing servingOptions.ServerCert.PairName and servingOptions.ServerCert.CertDirectory) so that you only assign "tls" when PairName is empty (e.g., check if servingOptions.ServerCert.PairName == "" before assigning), thereby preserving user-specified PairName while still providing the default when not set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/factory/factory.go`:
- Line 283: The code unconditionally sets servingOptions.ServerCert.PairName =
"tls", which overwrites any user-provided value; change the logic in the factory
where PairName is set (referencing servingOptions.ServerCert.PairName and
servingOptions.ServerCert.CertDirectory) so that you only assign "tls" when
PairName is empty (e.g., check if servingOptions.ServerCert.PairName == ""
before assigning), thereby preserving user-specified PairName while still
providing the default when not set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ceabce3-bd87-4732-976a-fc787074ff16
📒 Files selected for processing (1)
pkg/cmd/factory/factory.go
1 participant
Summary
Related issue(s)
Fixes #362
Summary by CodeRabbit
New Features
Refactor