feat(rotate-ca): add auto-discovery and post-processing #104
Conversation
- Auto-discover control plane and worker nodes from Kubernetes API - Auto-set --control-plane-nodes and --worker-nodes flags - Auto-set --output to project talosconfig path - Auto-set --k8s-endpoint from --endpoints flag - Post-processing after rotation: - Update secrets.yaml with new CA from cluster - Update talosconfig.encrypted if exists - Update kubeconfig via talm kubeconfig - Validate only one endpoint/node is provided - Use endpoint as target node for COSI requests - Disable --with-docs and --with-examples by default - Update help text and examples Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReworks the rotate-ca wrapper to accept and invoke the original RunE, adds pre-rotation auto-discovery (project root, kubeconfig, endpoints, cluster nodes), performs the original rotation, and runs post-rotation updates (secrets, optional re-encryption, talosconfig and kubeconfig refresh). Tightens talosconfig init error handling. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as rotate-ca CLI
participant Wrapper as Pre/Post Wrapper
participant TalosAPI as Talos API
participant KubeAPI as Kubernetes API
participant Original as Original rotate-ca Logic
participant FS as Filesystem
User->>CLI: run rotate-ca
CLI->>Wrapper: enter wrapper (with originalRunE)
Wrapper->>FS: detect project root & output path
Wrapper->>TalosAPI: fetch kubeconfig
TalosAPI-->>Wrapper: kubeconfig
Wrapper->>KubeAPI: list nodes / derive endpoint
KubeAPI-->>Wrapper: node list (control-plane / workers)
Wrapper->>Original: invoke originalRunE (perform CA rotation)
Original-->>Wrapper: rotation complete
Wrapper->>KubeAPI: fetch cluster CA certs
KubeAPI-->>Wrapper: CA certs
Wrapper->>FS: update secrets.yaml and optional encrypted files
Wrapper->>CLI: optionally run kubeconfig refresh command
FS-->>User: updated configs and secrets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Summary of ChangesHello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the rotate-ca command by adding auto-discovery of cluster nodes and post-rotation processing to update local configuration files. The changes are well-structured and add valuable automation. My review focuses on a few key areas to improve correctness and maintainability: ensuring the correct node is targeted for API calls, improving context propagation, and refactoring for code clarity and robustness. I've identified a critical issue where a specific control-plane node is not being used for COSI requests, which could lead to failures when a VIP is in use.
![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
🤖 Fix all issues with AI agents
In `@pkg/commands/rotate_ca_handler.go`:
- Around line 172-182: The targetNode value extracted in rotate_ca_handler.go is
never actually used inside updateSecretsFromCluster (which currently calls
WithClientNoNodes); either remove the unused parameter from
updateSecretsFromCluster and its call site (delete targetNode variable and stop
passing it), or modify updateSecretsFromCluster to accept and use targetNode by
replacing WithClientNoNodes with a client initializer that sets the node(s)
(e.g., create the Talos/Kubernetes/COSI client with the targetNode in its
context) so the COSI requests are directed to the specified node; update all
references to the updateSecretsFromCluster signature accordingly.
- Around line 131-134: The StringSlice flags are being set with
cmd.Flags().Set(...) which stores the whole comma-separated string as one
element (affecting later GetStringSlice() use and cpNodes[0]); change the flag
population to use the FlagSet.SetSlice API so each element is set separately —
obtain the pflag.FlagSet via cmd.Flags().(*pflag.FlagSet) and call
SetSlice("control-plane-nodes", cpNodes) and, when len(wNodes) > 0,
SetSlice("worker-nodes", wNodes); alternatively, call cmd.Flags().Set(...) once
per node string if SetSlice is not available.
🧹 Nitpick comments (3)
pkg/commands/rotate_ca_handler.go (3)
148-158: Consider reusingnormalizeEndpointfrom talosctl_wrapper.go.This endpoint normalization logic (stripping protocol, extracting host, adding
https://and:6443) is duplicated fromnormalizeEndpoint()intalosctl_wrapper.go. Consider extracting to a shared helper to reduce duplication.♻️ Suggested refactor
// Set --k8s-endpoint from GlobalArgs.Endpoints if !cmd.Flags().Changed("k8s-endpoint") && len(GlobalArgs.Endpoints) > 0 { - host := GlobalArgs.Endpoints[0] - host = strings.TrimPrefix(host, "https://") - host = strings.TrimPrefix(host, "http://") - if h, _, err := net.SplitHostPort(host); err == nil { - host = h - } - k8sEndpoint := fmt.Sprintf("https://%s:6443", host) + k8sEndpoint := normalizeEndpoint(GlobalArgs.Endpoints[0]) cmd.Flags().Set("k8s-endpoint", k8sEndpoint) }
282-305: Additional duplication of endpoint normalization logic.This is the third occurrence of the endpoint normalization pattern (lines 289-296). Strongly consider extracting this to a shared helper function.
♻️ Extract shared helper
Create a helper function that can be reused across all three locations:
// extractHost extracts the host from an endpoint, stripping protocol and port. func extractHost(endpoint string) string { host := endpoint host = strings.TrimPrefix(host, "https://") host = strings.TrimPrefix(host, "http://") if h, _, err := net.SplitHostPort(host); err == nil { host = h } return host }Then
normalizeEndpointand these functions can useextractHost.
394-404: Potential state mutation on shared command object.Setting
cmd.Flags().Set("force", "true")modifies the globalCommandsslice entry. If this command is run multiple times or in tests, the flag will remain set. This is likely fine for CLI usage but worth noting.Additionally, the loop could break early after finding the command to avoid unnecessary iterations.
♻️ Minor optimization
func runKubeconfigCmd() error { for _, cmd := range Commands { if cmd.Name() == "kubeconfig" { // Set --force to avoid interactive prompt cmd.Flags().Set("force", "true") return cmd.RunE(cmd, []string{}) } } return fmt.Errorf("kubeconfig command not found") }The current implementation is actually fine since
returnexits the loop. No change needed.
- Check total endpoints + nodes count in rotate-ca validation - Propagate decryption errors in init.go instead of ignoring them - Check flag existence with Lookup() before calling Set() - Handle all Set() errors properly Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
feat/rotate-ca-auto-discovery
branch
from
595e6b0 to
f68e14b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/commands/rotate_ca_handler.go`:
- Around line 401-415: runKubeconfigCmd currently calls cmd.RunE directly which
bypasses the command's PreRunE setup (e.g., wrapTalosCommand’s initialization);
change the invocation to use the command lifecycle so PreRunE runs first —
either call cmd.Execute() on the located "kubeconfig" command (preferred) or
explicitly invoke cmd.PreRunE(cmd, []string{}) and handle its error before
calling cmd.RunE(cmd, []string{}); keep the existing logic that sets the "force"
flag and propagate any errors from PreRunE/Execute/RunE.
♻️ Duplicate comments (4)
pkg/commands/rotate_ca_handler.go (4)
238-242: Use command context instead ofcontext.Background().Using
context.Background()prevents proper cancellation and timeout handling. The command's context should be propagated from the RunE function.♻️ Propagate context from command
Update
discoverClusterNodessignature to accept context:-func discoverClusterNodes() (controlPlane []string, workers []string, err error) { +func discoverClusterNodes(ctx context.Context) (controlPlane []string, workers []string, err error) {Update the call site in RunE:
- cpNodes, wNodes, err := discoverClusterNodes() + cpNodes, wNodes, err := discoverClusterNodes(cmd.Context())Use the passed context:
- nodes, err := clientset.CoreV1().Nodes().List(context.Background(), metav1.ListOptions{}) + nodes, err := clientset.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
403-413: Command lookup by iteration is inefficient.Searching for the kubeconfig command by iterating through the
Commandsslice on each call is inefficient. Consider creating a lookup map keyed by command name during initialization.
130-137: StringSlice flag setting with comma-joined strings does not work as intended.Cobra's
Set()method treats the entire comma-separated string as a single value rather than splitting it. WhencpNodesis["10.0.0.1", "10.0.0.2"], callingSet("control-plane-nodes", "10.0.0.1,10.0.0.2")stores it as a single element["10.0.0.1,10.0.0.2"].This causes
cpNodes[0]on line 183 to be"10.0.0.1,10.0.0.2"instead of"10.0.0.1", breaking the targetNode logic.🐛 Recommended fix: Call Set() for each node individually
- if err := cmd.Flags().Set("control-plane-nodes", strings.Join(cpNodes, ",")); err != nil { - return fmt.Errorf("failed to set control-plane-nodes: %w", err) - } - if len(wNodes) > 0 { - if err := cmd.Flags().Set("worker-nodes", strings.Join(wNodes, ",")); err != nil { - return fmt.Errorf("failed to set worker-nodes: %w", err) - } - } + for _, node := range cpNodes { + if err := cmd.Flags().Set("control-plane-nodes", node); err != nil { + return fmt.Errorf("failed to set control-plane-nodes: %w", err) + } + } + for _, node := range wNodes { + if err := cmd.Flags().Set("worker-nodes", node); err != nil { + return fmt.Errorf("failed to set worker-nodes: %w", err) + } + }
179-189: ThetargetNodeparameter is extracted but unused inupdateSecretsFromCluster.
targetNodeis passed toupdateSecretsFromCluster(line 187) but inside that function,WithClientNoNodesis used (line 326) which doesn't configure the client to use the target node. The COSI requests may fail or connect to the wrong node if the endpoint is a VIP.🔧 Option: Set endpoint to targetNode before COSI calls
func updateSecretsFromCluster(updateTalos, updateKubernetes bool, targetNode string) error { secretsPath := ResolveSecretsPath(Config.TemplateOptions.WithSecrets) // Load existing secrets bundle, err := secrets.LoadBundle(secretsPath) if err != nil { return fmt.Errorf("failed to load secrets bundle: %w", err) } + // Temporarily set endpoint to the target node for COSI requests + originalEndpoints := GlobalArgs.Endpoints + if targetNode != "" { + GlobalArgs.Endpoints = []string{targetNode} + } + defer func() { GlobalArgs.Endpoints = originalEndpoints }() + // Use WithClientNoNodes to avoid automatic node setting err = WithClientNoNodes(func(ctx context.Context, c *client.Client) error {
🧹 Nitpick comments (1)
pkg/commands/rotate_ca_handler.go (1)
74-82: Consider handling Set() errors consistently.The errors from
Set()for--with-docsand--with-examplesare ignored with blank identifiers. While unlikely to fail for these optional boolean flags, this is inconsistent with the careful error handling elsewhere in this PR.🔧 Suggested improvement
if f := wrappedCmd.Flags().Lookup("with-docs"); f != nil { f.DefValue = "false" - _ = wrappedCmd.Flags().Set("with-docs", "false") + if err := wrappedCmd.Flags().Set("with-docs", "false"); err != nil { + // Log or ignore - these are optional flags + } } if f := wrappedCmd.Flags().Lookup("with-examples"); f != nil { f.DefValue = "false" - _ = wrappedCmd.Flags().Set("with-examples", "false") + if err := wrappedCmd.Flags().Set("with-examples", "false"); err != nil { + // Log or ignore - these are optional flags + } }
Summary
Extends the
rotate-cacommand with automatic cluster node discovery and post-rotation configuration updates:--control-plane-nodes,--worker-nodes,--output, and--k8s-endpointflagssecrets.yamlwith new CA certificates from clustersecrets.encrypted.yamlif existstalosconfig.encryptedif existskubeconfigviatalm kubeconfigChanges
WithClientNoNodesfor COSI requests to avoid multi-node proxying issues--with-docsand--with-examplesby defaultTest plan
Summary by CodeRabbit
Enhancements
Bug Fixes
Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.