✨ [WIP] Adding managed namespace field in the managedclusterset #390
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds ManagedNamespaceConfig and ClusterSetManagedNamespaceConfig types to cluster/v1, exposes managedNamespaces in ManagedClusterSetSpec (v1beta2) and ManagedCluster status (v1), updates generated deep-copy code, updates CRD schemas, and removes one debug echo from a maintenance script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/hold |
![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
🧹 Nitpick comments (1)
cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml (1)
130-147: Tighten wording and fix minor grammar in descriptions.The descriptions use “namespace” (singular) and “clusterset”. Recommend pluralizing and using “ManagedClusterSet” consistently.
Apply this wording update:
- description: |- - managedNamespaces defines the list of namespace on the managedclusters - across the clusterset to be managed. + description: |- + managedNamespaces defines the list of namespaces on the managed clusters + across the ManagedClusterSet to be managed. items: - description: |- - managedNamespaces defines a namespace on the managedclusters across the - clusterset to be managed by this clusterset. + description: |- + A managed namespace on the managed clusters across the ManagedClusterSet + to be managed by this ManagedClusterSet. properties: name: - description: name is the name of the namespace. + description: Name of the namespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cluster/v1/types.go(1 hunks)cluster/v1/zz_generated.deepcopy.go(2 hunks)cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml(1 hunks)cluster/v1beta2/types_managedclusterset.go(2 hunks)cluster/v1beta2/zz_generated.deepcopy.go(2 hunks)hack/update-copyright.sh(0 hunks)
💤 Files with no reviewable changes (1)
- hack/update-copyright.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- cluster/v1beta2/types_managedclusterset.go
- cluster/v1/types.go
- cluster/v1beta2/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cluster/v1/zz_generated.deepcopy.go (1)
cluster/v1/types.go (2)
ClusterSetManagedNamespaceConfig(201-206)ManagedNamespaceConfig(193-199)
🔇 Additional comments (1)
cluster/v1/zz_generated.deepcopy.go (1)
218-233: LGTM: DeepCopy for ManagedNamespaceConfigAll fields are primitives; a shallow copy is correct.
cluster/v1/zz_generated.deepcopy.go
Outdated
| // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. | ||
| func (in *ClusterSetManagedNamespaceConfig) DeepCopyInto(out *ClusterSetManagedNamespaceConfig) { | ||
| *out = *in | ||
| out.ManagedNamespaceConfig = in.ManagedNamespaceConfig | ||
| return | ||
| } | ||
|
|
||
| // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterSetManagedNamespaceConfig. | ||
| func (in *ClusterSetManagedNamespaceConfig) DeepCopy() *ClusterSetManagedNamespaceConfig { | ||
| if in == nil { | ||
| return nil | ||
| } | ||
| out := new(ClusterSetManagedNamespaceConfig) | ||
| in.DeepCopyInto(out) | ||
| return out | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
DeepCopy for ClusterSetManagedNamespaceConfig is correct; shallow copy is sufficient.
Both fields are value types, so the shallow copy is fine. Note that the explicit assignment of ManagedNamespaceConfig after *out = *in is redundant but harmless in generated code.
One cross-file check: In cluster/v1/types.go, the embedded ManagedNamespaceConfig should use the JSON inline tag form json:",inline" to actually inline fields in JSON. If it's json:"inline", it will serialize under an "inline" subfield instead of flattening.
[suggest_nitpick]
Run this to confirm the embed tag and avoid unintended wire format:
If you see json:"inline", update it to:
// in cluster/v1/types.go
type ClusterSetManagedNamespaceConfig struct {
// clusterSet represents the name of the cluster set.
// +required
ClusterSet string `json:"clusterSet"`
ManagedNamespaceConfig `json:",inline"`
}🏁 Script executed:
#!/bin/bash
# Validate the embed tag on ClusterSetManagedNamespaceConfig
rg -n -C2 -P 'type\s+ClusterSetManagedNamespaceConfig\s+struct\s*\{' cluster/v1/types.go
rg -n -C1 -P 'ManagedNamespaceConfig\s+`json:"[^`]*"`' cluster/v1/types.goLength of output: 411
Fix JSON inline tag on ManagedNamespaceConfig embed
The autogenerated DeepCopy methods for ClusterSetManagedNamespaceConfig are correct (a shallow copy is sufficient and the extra assignment is harmless). However, the JSON struct tag on the embedded ManagedNamespaceConfig is currently wrong and will serialize under an “inline” field rather than flattening its fields. Please update it as follows:
- File: cluster/v1/types.go
- Location: struct ClusterSetManagedNamespaceConfig, field ManagedNamespaceConfig
Diff:
--- a/cluster/v1/types.go
+++ b/cluster/v1/types.go
@@ -203,7 +203,7 @@ type ClusterSetManagedNamespaceConfig struct {
ClusterSet string `json:"clusterSet"`
- ManagedNamespaceConfig `json:"inline"`
+ ManagedNamespaceConfig `json:",inline"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. | |
| func (in *ClusterSetManagedNamespaceConfig) DeepCopyInto(out *ClusterSetManagedNamespaceConfig) { | |
| *out = *in | |
| out.ManagedNamespaceConfig = in.ManagedNamespaceConfig | |
| return | |
| } | |
| // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterSetManagedNamespaceConfig. | |
| func (in *ClusterSetManagedNamespaceConfig) DeepCopy() *ClusterSetManagedNamespaceConfig { | |
| if in == nil { | |
| return nil | |
| } | |
| out := new(ClusterSetManagedNamespaceConfig) | |
| in.DeepCopyInto(out) | |
| return out | |
| } | |
| // File: cluster/v1/types.go | |
| type ClusterSetManagedNamespaceConfig struct { | |
| ClusterSet string `json:"clusterSet"` | |
| ManagedNamespaceConfig `json:",inline"` | |
| } |
🤖 Prompt for AI Agents
In cluster/v1/zz_generated.deepcopy.go around lines 35-50 we see DeepCopy for
ClusterSetManagedNamespaceConfig; the real issue is in cluster/v1/types.go on
the ClusterSetManagedNamespaceConfig struct where the ManagedNamespaceConfig
embed has an incorrect JSON tag that serializes as "inline" instead of
flattening; update the struct field tag for ManagedNamespaceConfig from
json:"inline" (or similar) to json:",inline" so the embedded fields are inlined
during JSON/YAML marshal/unmarshal, then run codegen/regenerate deepcopy files
if your workflow requires it.
cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml
Show resolved
Hide resolved
| // +required | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| // +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Is there any restriction on the name? for example, it cannot be one of 'kube-system', 'kube-public' and 'default'
| Name string `json:"name"` | ||
| } | ||
|
|
||
| type ClusterSetManagedNamespaceConfig struct { |
There was a problem hiding this comment.
Has this struct been used anywhere?
There was a problem hiding this comment.
updated, should be in the cluster status
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cluster/v1/types.go (2)
205-205: Name restrictions question already raised earlierPrior review asked about restricting names like kube-system/kube-public/default. See my suggestion above to add CEL validation to enforce this at the API level.
208-208: Usage confirmationThis struct is now used by ManagedClusterStatus.ManagedNamespaces, addressing the previous “used anywhere?” concern.
🧹 Nitpick comments (3)
cluster/v1/types.go (1)
201-206: Consider blocking reserved/system namespacesIf these namespaces are to be created/managed by the product, it’s safer to preclude well-known system namespaces (e.g., default, kube-system, kube-public). You can enforce this via CEL XValidation.
Proposed change:
type ManagedNamespaceConfig struct { // name is the name of the namespace. // +required // +kubebuilder:validation:MaxLength=63 // +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + // +kubebuilder:validation:XValidation:rule="self != 'default' && self != 'kube-system' && self != 'kube-public'",message="reserved namespaces are not allowed" Name string `json:"name"` }If blocking more system namespaces (e.g., kube-, openshift-) is desired, say the word and I’ll craft a stricter rule.
cluster/v1beta2/types_managedclusterset.go (1)
53-59: Minor doc tweak for clarityNit: pluralization and wording.
Apply this diff to the comment (no functional change):
- // managedNamespaces defines the list of namespace on the managedclusters - // across the clusterset to be managed. + // managedNamespaces defines the list of namespaces on managed clusters + // across the clusterset to be managed.cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml (1)
131-134: Nit: grammar in descriptionGenerated from Go comments. If you accept the wording tweak in the Go type, re-generate to reflect it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml(4 hunks)cluster/v1/types.go(2 hunks)cluster/v1/zz_generated.deepcopy.go(3 hunks)cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml(1 hunks)cluster/v1beta2/types_managedclusterset.go(2 hunks)cluster/v1beta2/zz_generated.deepcopy.go(2 hunks)hack/update-copyright.sh(0 hunks)
💤 Files with no reviewable changes (1)
- hack/update-copyright.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- cluster/v1beta2/zz_generated.deepcopy.go
- cluster/v1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cluster/v1beta2/types_managedclusterset.go (1)
cluster/v1/types.go (1)
ManagedNamespaceConfig(200-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
🔇 Additional comments (4)
cluster/v1/types.go (1)
208-217: LGTM on the inline embedding and conditionsEmbedding ManagedNamespaceConfig inline and exposing clusterSet plus conditions is clean and aligns with metav1.Condition usage.
cluster/v1beta2/types_managedclusterset.go (1)
6-7: Appropriate dependency on v1 typeImporting v1 and reusing ManagedNamespaceConfig avoids duplication and keeps validations centralized.
cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml (1)
130-151: CRD list semantics look correctx-kubernetes-list-type: map with x-kubernetes-list-map-keys: [name] matches the Go markers and enforces uniqueness/merge-by-name as intended.
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (1)
266-341: Validation coverage looks goodThe item schema includes required clusterSet and name with DNS-1123 constraints and Conditions use metav1.Condition schema. Once the compound keys change is in, this block is solid.
If you add the CEL validation restricting reserved namespaces in Go, re-run generation and ensure an x-kubernetes-validations entry appears under properties.name here.
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml
Outdated
Show resolved
Hide resolved
| // cluster belongs to. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=clusterSet |
There was a problem hiding this comment.
Since the 'clusterSet' is the key of the map, does that mean for each clusterset only one item is allowed in the list? in another world, only one managed namespace is allowed for a clusterset in the 'managedNamespaces' list?
There was a problem hiding this comment.
correct, add name is another key
Signed-off-by: Jian Qiu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (1)
343-346: List map keys include both clusterSet and name — resolvedThis addresses the earlier feedback about enabling multiple namespaces per clusterset on a single ManagedCluster.
🧹 Nitpick comments (1)
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (1)
265-346: New status.managedNamespaces shape looks right; consider validating clusterSet as a DNS-1123 labelThe list/map semantics, required fields, and condition schema look correct. To align clusterSet with typical k8s resource name constraints (same as name), consider adding maxLength and pattern validations to clusterSet.
Apply this diff within the clusterSet property to add validation:
properties: clusterSet: - description: clusterSet represents the name of the cluster set. - type: string + description: clusterSet represents the name of the cluster set. + maxLength: 63 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml(4 hunks)cluster/v1/types.go(2 hunks)cluster/v1/zz_generated.deepcopy.go(3 hunks)cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml(1 hunks)cluster/v1beta2/types_managedclusterset.go(2 hunks)cluster/v1beta2/zz_generated.deepcopy.go(2 hunks)hack/update-copyright.sh(0 hunks)
💤 Files with no reviewable changes (1)
- hack/update-copyright.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- cluster/v1beta2/types_managedclusterset.go
- cluster/v1beta2/zz_generated.deepcopy.go
- cluster/v1/types.go
- cluster/v1/zz_generated.deepcopy.go
- cluster/v1beta2/0000_00_clusters.open-cluster-management.io_managedclustersets.crd.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: verify
🔇 Additional comments (6)
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (6)
168-170: Doc casing normalization on allocatable — OKNo functional impact; reads cleaner and matches surrounding style.
179-181: Capacity description tweak — OKMinor wording change only; schema unchanged.
184-189: ClusterClaims description improvement — OKClearer guidance without altering validation.
208-210: Conditions description minor edit — OKNo behavioral change.
348-354: Version description casing — OKCosmetic doc change; schema intact.
265-346: Go markers and CRD managedNamespaces keys are in sync
- cluster/v1/types.go (lines ~171–174):
// +listType=map
// +listMapKey=clusterSet
// +listMapKey=name
ManagedNamespaces []ClusterSetManagedNamespaceConfigjson:"managedNamespaces,omitempty"- cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml (lines 265–346):
x-kubernetes-list-map-keys:
- clusterSet
- name
No discrepancies found—markers and CRD definitions already match.
|
/lgtm |
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Chores