✨ ✨ feature feat: add labels to managedcluster resource if present #1123
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,11 +89,14 @@ func (c *managedClusterCreatingController) sync(ctx context.Context, syncCtx fac | |
| managedCluster = decorator(managedCluster) | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. the reason we only compare size is because the configs can be set on the hub with different cabundle or endpoint. And the agent should trust setting of hub since hub would have more accurate info on how to access spoke apiserver. @zhujian7 I wonder whether to update the cluster should be returned by the decorator rather than compare in the controller.
Member
There was a problem hiding this comment. Yes, make sense, I think we can let the decorator interface return an additional |
||
| // Check if any updates are needed (ClientConfigs, Labels, or Annotations) | ||
| if equalClientConfigs(existingCluster.Spec.ManagedClusterClientConfigs, managedCluster.Spec.ManagedClusterClientConfigs) && | ||
| equalLabels(existingCluster.Labels, managedCluster.Labels) && | ||
| equalAnnotations(existingCluster.Annotations, managedCluster.Annotations) { | ||
| return nil | ||
| } | ||
|
|
||
| // update ManagedCluster (ClientConfigs, Labels, and Annotations) | ||
| _, err = c.hubClusterClient.ClusterV1().ManagedClusters().Update(ctx, managedCluster, metav1.UpdateOptions{}) | ||
|
Comment on lines
+93
to
101
There was a problem hiding this comment. 🛠️ Refactor suggestion Equality gate may churn on ordering; make client-config comparison order-insensitive The current equalClientConfigs is order-sensitive; different ordering will trigger unnecessary updates. Sort by URL (and CABundle as tiebreaker) before comparing. -// equalClientConfigs compares two ClientConfig slices for equality
-func equalClientConfigs(configs1, configs2 []clusterv1.ClientConfig) bool {
- if len(configs1) != len(configs2) {
- return false
- }
- for i, config1 := range configs1 {
- config2 := configs2[i]
- if config1.URL != config2.URL {
- return false
- }
- if string(config1.CABundle) != string(config2.CABundle) {
- return false
- }
- }
- return true
-}
+// equalClientConfigs compares two ClientConfig slices for equality (order-insensitive).
+func equalClientConfigs(configs1, configs2 []clusterv1.ClientConfig) bool {
+ if len(configs1) != len(configs2) {
+ return false
+ }
+ s1 := append([]clusterv1.ClientConfig(nil), configs1...)
+ s2 := append([]clusterv1.ClientConfig(nil), configs2...)
+ sort.Slice(s1, func(i, j int) bool {
+ if s1[i].URL == s1[j].URL {
+ return string(s1[i].CABundle) < string(s1[j].CABundle)
+ }
+ return s1[i].URL < s1[j].URL
+ })
+ sort.Slice(s2, func(i, j int) bool {
+ if s2[i].URL == s2[j].URL {
+ return string(s2[i].CABundle) < string(s2[j].CABundle)
+ }
+ return s2[i].URL < s2[j].URL
+ })
+ for i := range s1 {
+ if s1[i].URL != s2[i].URL || string(s1[i].CABundle) != string(s2[i].CABundle) {
+ return false
+ }
+ }
+ return true
+}Note: add the following import in this file: import "sort"🤖 Prompt for AI Agents |
||
| // ManagedClusterClientConfigs in ManagedCluster is only allowed updated during bootstrap. | ||
| // After bootstrap secret expired, an unauthorized error will be got, skip it | ||
|
|
@@ -112,15 +115,33 @@ func skipUnauthorizedError(err error) error { | |
| return err | ||
| } | ||
|
|
||
| // AnnotationDecorator set annotations from annotation map to ManagedCluster | ||
| func AnnotationDecorator(annotations map[string]string) ManagedClusterDecorator { | ||
| return func(cluster *clusterv1.ManagedCluster) *clusterv1.ManagedCluster { | ||
| if cluster.Annotations == nil { | ||
| cluster.Annotations = make(map[string]string) | ||
| } | ||
|
|
||
| filteredAnnotations := commonhelpers.FilterClusterAnnotations(annotations) | ||
| for k, v := range filteredAnnotations { | ||
| cluster.Annotations[k] = v | ||
| } | ||
|
|
||
| return cluster | ||
| } | ||
| } | ||
|
|
||
| // LabelDecorator set labels from label map to ManagedCluster | ||
| func LabelDecorator(labels map[string]string) ManagedClusterDecorator { | ||
| return func(cluster *clusterv1.ManagedCluster) *clusterv1.ManagedCluster { | ||
| if cluster.Labels == nil { | ||
| cluster.Labels = make(map[string]string) | ||
| } | ||
|
|
||
| for k, v := range labels { | ||
|
Member
There was a problem hiding this comment. note we should not allowing set ANY label from the spoke. For instance, open-cluster-management.io/clusterset label is to define the label on the managedcluster, it should not be set by agent. |
||
| cluster.Labels[k] = v | ||
| } | ||
|
|
||
| return cluster | ||
| } | ||
| } | ||
|
|
@@ -148,3 +169,46 @@ func ClientConfigDecorator(externalServerURLs []string, caBundle []byte) Managed | |
| return cluster | ||
| } | ||
| } | ||
|
|
||
| // equalClientConfigs compares two ClientConfig slices for equality | ||
| func equalClientConfigs(configs1, configs2 []clusterv1.ClientConfig) bool { | ||
|
Member
There was a problem hiding this comment. same here, clientconfigs can also be updated by user, so we cannot update clientConfigs based on equality. |
||
| if len(configs1) != len(configs2) { | ||
| return false | ||
| } | ||
| for i, config1 := range configs1 { | ||
| config2 := configs2[i] | ||
| if config1.URL != config2.URL { | ||
| return false | ||
| } | ||
| if string(config1.CABundle) != string(config2.CABundle) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // equalLabels compares two label maps for equality | ||
| func equalLabels(labels1, labels2 map[string]string) bool { | ||
|
Member
There was a problem hiding this comment. we cannot do such equal since the label of cluster can also be set by user on hub. So such label setting can only happen when creating the cluster. If the label exists already on the cluster, the controller cannot update the label or annotation since it might be updated by user on purpose. |
||
| if len(labels1) != len(labels2) { | ||
| return false | ||
| } | ||
| for k, v := range labels1 { | ||
| if labels2[k] != v { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // equalAnnotations compares two annotation maps for equality | ||
| func equalAnnotations(annotations1, annotations2 map[string]string) bool { | ||
| if len(annotations1) != len(annotations2) { | ||
| return false | ||
| } | ||
| for k, v := range annotations1 { | ||
| if annotations2[k] != v { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Confirm registration agent supports --cluster-labels and values flow from Helm.
Validate that:
Run this script:
🏁 Script executed:
Length of output: 5146
🏁 Script executed:
Length of output: 2493
Ensure Helm chart exposes and injects
clusterLabelsWe’ve confirmed that:
--cluster‐labelsflag.- "--cluster-labels={{ .ClusterLabelsString }}"when.ClusterLabelsStringis set.clusterLabelssection invalues.yamland provides no helper to build.ClusterLabelsString.To complete the feature, please:
In
deploy/klusterlet/chart/klusterlet/values.yaml, uncomment or add a top‐levelclusterLabels: {}map.In
deploy/klusterlet/chart/klusterlet/templates/_helpers.tpl, define a helper, e.g.:In both
klusterlet-registration-deployment.yamlandklusterlet-agent-deployment.yaml, set:This will ensure
Values.clusterLabelsflows through to.ClusterLabelsStringand into both deployments.🤖 Prompt for AI Agents