support running karmada-agent out-of-cluster - #7821
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7821 +/- ##
==========================================
+ Coverage 42.07% 42.22% +0.14%
==========================================
Files 879 880 +1
Lines 54852 54907 +55
==========================================
+ Hits 23081 23182 +101
+ Misses 30026 29978 -48
- Partials 1745 1747 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/assign |
Signed-off-by: driegel1 <driegel1@bloomberg.net>
b1a21bb to
dc21ffb
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a --register-cluster flag to allow karmada-agent to run out-of-cluster without performing self-registration, instead validating a pre-registered Pull-mode cluster.
Changes:
- Introduces
--register-cluster(defaulttrue) and wires it into agent startup logic. - Adds
validateExternallyRegisteredClusterto verify pre-registration invariants when self-registration is disabled. - Adds unit tests for the new validation logic and regenerates the CLI flags documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/command-line-flags/karmada-agent.md | Documents the new --register-cluster flag and its behavior. |
| cmd/agent/app/registration.go | Adds validation logic for externally registered (Pull-mode) clusters. |
| cmd/agent/app/registration_test.go | Adds unit tests for the new external-registration validation. |
| cmd/agent/app/options/options.go | Adds RegisterCluster option and exposes it as a CLI flag. |
| cmd/agent/app/agent.go | Gates self-registration behind opts.RegisterCluster; runs validation when disabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
| fs.DurationVar(&o.CertRotationCheckingInterval, "cert-rotation-checking-interval", 5*time.Minute, "The interval of checking if the certificate need to be rotated. This is only applicable if cert rotation is enabled") | ||
| fs.Float64Var(&o.CertRotationRemainingTimeThreshold, "cert-rotation-remaining-time-threshold", 0.2, "The threshold of remaining time of the valid certificate. This is only applicable if cert rotation is enabled.") | ||
| fs.StringVar(&o.KarmadaKubeconfigNamespace, "karmada-kubeconfig-namespace", "karmada-system", "Namespace of the secret containing karmada-agent certificate. This is only applicable if cert rotation is enabled.") | ||
| fs.BoolVar(&o.RegisterCluster, "register-cluster", true, "Whether to register the member cluster with the Karmada control plane on startup. Set to false when the cluster is pre-registered by an external process; the agent will then only validate the existing Pull-mode registration and run its controllers.") |
There was a problem hiding this comment.
Good that this is backwards compatible.
| return fmt.Errorf("cluster %q has SyncMode %q, expected %q for an externally registered cluster", opts.ClusterName, cluster.Spec.SyncMode, clusterv1alpha1.Pull) | ||
| } | ||
|
|
||
| clusterID, err := util.ObtainClusterID(memberKubeClient) |
There was a problem hiding this comment.
I think we can drop the cluster ID check. The ID is an optional field that may not always be set. Also this util function does not take into account the fact that there's multiple ways in which it can be set.
Ref: https://github.com/karmada-io/karmada/blob/release-1.18/pkg/apis/cluster/types.go#L64
Or maybe we can make the util function more robust.
@RainbowMango , what do you think?
jabellard
left a comment
There was a problem hiding this comment.
Generally looks good. Left a couple of comments.
|
Hey @driegel1. We can perform some e2e tests locally using the local-up script. I think we need to test that:
|
|
/retest |
|
/cc @RainbowMango for another look |
|
@jabellard: GitHub didn't allow me to request PR reviews from the following users: for, another, look. Note that only karmada-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. |
And to test a missing registration failure on startup, we can:
|
|
/assign |
What type of PR is this?*
/kind feature
What this PR does / why we need it:
Addresses #7601. Adds a
--register-clusterboolean flag (defaulttrue) tokarmada-agent. When set tofalse, the agent skips self-registration and instead validates that the member cluster isalready registered with the Karmada control plane before starting its Pull-mode controllers.
This supports environments where cluster registration is managed by an external system (e.g. Bloomberg's
managed Karmada platform) rather than by the agent itself. The agent can already connect to a
member cluster out-of-cluster via the existing
--kubeconfigflag; the only missing piece wasthe ability to decouple registration from the agent's runtime, which this flag provides.
When
--register-cluster=false, the agent verifies the pre-registered cluster satisfied all of the following:spec.syncModeisPullspec.id(if set) matches the member cluster's IDThe default (
--register-cluster=true) is unchanged and fully backward-compatible.Which issue(s) this PR fixes:
Fixes #7601
Special notes for your reviewer:
run()is wrapped in anif opts.RegisterCluster { ... } else { ... }; no controller wiring or config plumbing waschanged. Out-of-cluster member connectivity relies on the pre-existing
--kubeconfigflag.validateExternallyRegisteredClustercovering: happy path, missingcluster, cluster being deleted, wrong sync mode, ID mismatch, and missing
kube-systemnamespace.
docs/command-line-flags/karmada-agent.mdregenerated viahack/update-command-line-flags.sh.go build,go test ./cmd/agent/...,go vet, andhack/verify-command-line-flags.shall pass.Does this PR introduce a user-facing change?: