Skip to content

✨add ClusterClaimConfiguration to API #363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+73 −0
Open

✨add ClusterClaimConfiguration to API #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54d9a77
add clusterclaimconfig type
o-farag Mar 26, 2025
0ef1a0d
resolve merge conflict
o-farag Apr 1, 2025
34775f3
move ClusterClaimConfiguration to be under RegistrationConfiguration
o-farag Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions operator/v1/0000_00_operator.open-cluster-management.io_klusterlets.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,24 @@ spec:
ClusterAnnotations is annotations with the reserve prefix "agent.open-cluster-management.io" set on
ManagedCluster when creating only, other actors can update it afterwards.
type: object
clusterClaimConfiguration:
description: |-
ClusterClaimConfiguration represents the configuration of ClusterClaim
Effective only when the `ClusterClaim` feature gate is enabled.
properties:
maxCustomClusterClaims:
default: 20
description: Maximum number of custom ClusterClaims allowed.
format: int32
type: integer
reservedClusterClaimSuffixes:
description: Custom suffixes for reserved ClusterClaims.
items:
type: string
type: array
required:
- maxCustomClusterClaims
type: object
featureGates:
description: "FeatureGates represents the list of feature gates
for registration\nIf it is set empty, default feature gates
Expand Down
18 changes: 18 additions & 0 deletions operator/v1/types_klusterlet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,24 @@ type RegistrationConfiguration struct {
// This provides driver details required to register with hub
// +optional
RegistrationDriver RegistrationDriver `json:"registrationDriver,omitempty"`

// ClusterClaimConfiguration represents the configuration of ClusterClaim
// Effective only when the `ClusterClaim` feature gate is enabled.
// +optional
ClusterClaimConfiguration *ClusterClaimConfiguration `json:"clusterClaimConfiguration,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ClusterClaimConfiguration *ClusterClaimConfiguration `json:"clusterClaimConfiguration,omitempty"`
ClusterClaims *ClusterClaimConfiguration `json:"clusterClaims,omitempty"`

nit, not sure if it is better/simpler to remove the suffix configuration and make it as clusterClaim or clusterClaims

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elgnay what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the name ClusterClaims. @qiujian16 Is there any naming convention or best practice we should follow when defining the API?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda think it is a configuration, clusterClaims might be confusing on allowing user to specify claims they would like to create.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with quijian16, since this configures cluster claims and aren't actual clusterclaims

}

// ClusterClaimConfiguration represents the configuration of ClusterClaim
type ClusterClaimConfiguration struct {
// Maximum number of custom ClusterClaims allowed.
// +kubebuilder:validation:Required
// +kubebuilder:default:=20
// +required
MaxCustomClusterClaims int32 `json:"maxCustomClusterClaims"`

// Custom suffixes for reserved ClusterClaims.
// +optional
ReservedClusterClaimSuffixes []string `json:"reservedClusterClaimSuffixes,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not quite understand why a customer needs to configure the reserved cluster claims?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not quite understand why a customer needs to configure the reserved cluster claims?

@zhujian7 the proposed enhancement outlines this addition, I'm not sure if a specific customer requested it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is guaranteed that reserved ClusterClaims will be reported in the status of the ManagedCluster. However, custom ClusterClaims are not guaranteed to appear due to the limitation on the number of custom ClusterClaims, even if the user sets a relatively high limit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, ClusterClaims matching the ReservedClusterClaimSuffixes should be treated as additional reserved claims. They must be merged with, not override, the built-in (hardcoded) reserved claims when reported to the hub. Please either include this information in the field comment or consider renaming the field to AdditionalReservedClusterClaimSuffixes for clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could start with a predefined suffix. I do not know if there are use cases that need is to be configurable, and I am not sure what it should be when user change the suffix from one to another.

}

type RegistrationDriver struct {
Expand Down
26 changes: 26 additions & 0 deletions operator/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions operator/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.