Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 43 additions & 0 deletions api/hypershift/v1beta1/hostedcluster_helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package v1beta1

// EffectiveShards returns the effective shard configuration for managed etcd.
// If shards are not explicitly configured, returns a default single shard.
func (m *ManagedEtcdSpec) EffectiveShards(hcp *HostedControlPlane) []ManagedEtcdShardSpec {
if len(m.Shards) > 0 {
return m.Shards
}

replicas := int32(1)
if hcp.Spec.ControllerAvailabilityPolicy == HighlyAvailable {
replicas = 3
}

return []ManagedEtcdShardSpec{
{
Name: "default",
ResourcePrefixes: []string{"/"},
Priority: EtcdShardPriorityCritical,
Replicas: &replicas,
BackupSchedule: "*/30 * * * *",
},
}
}

// EffectiveShards returns the effective shard configuration for unmanaged etcd.
// If shards are not explicitly configured, returns a default single shard using
// the legacy endpoint and tls fields.
func (u *UnmanagedEtcdSpec) EffectiveShards() []UnmanagedEtcdShardSpec {
if len(u.Shards) > 0 {
return u.Shards
}

return []UnmanagedEtcdShardSpec{
{
Name: "default",
ResourcePrefixes: []string{"/"},
Priority: EtcdShardPriorityCritical,
Endpoint: u.Endpoint,
TLS: u.TLS,
},
}
}
146 changes: 137 additions & 9 deletions api/hypershift/v1beta1/hostedcluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,8 @@ type EtcdSpec struct {
// HyperShift.
type ManagedEtcdSpec struct {
// storage specifies how etcd data is persisted.
// When shards are specified, this serves as the default for all shards
// unless overridden per-shard.
Comment on lines +1903 to +1904

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could also make it so that this field cannot be set, forcing a choice in each shard to be explicit

// +required
// +kubebuilder:validation:XValidation:rule="has(self.restoreSnapshotURL) == has(oldSelf.restoreSnapshotURL)",message="restoreSnapshotURL cannot be added or removed after creation"
Storage ManagedEtcdStorageSpec `json:"storage"`
Expand All @@ -1910,6 +1912,81 @@ type ManagedEtcdSpec struct {
// +optional
// +openshift:enable:FeatureGate=HCPEtcdBackup
Backup HCPEtcdBackupConfig `json:"backup,omitzero"`

// shards configures etcd sharding by Kubernetes resource kind.
// When not specified, a default single shard accepting all prefixes is used.
// When specified, exactly one shard must have "/" in its resourcePrefixes.
// +optional
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any particular decision process to get to 10 as a maximum?

Should document in the godoc that this list accepts at most 10 items

// +listType=map
// +listMapKey=name
// +kubebuilder:validation:XValidation:rule="self.exists(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The rule itself checks that at least 1 item has / in the resourcePrefixes, but the message says exactly one, perhaps you want

Suggested change
// +kubebuilder:validation:XValidation:rule="self.exists(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix"
// +kubebuilder:validation:XValidation:rule="self.exists_one(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix"

// +kubebuilder:validation:XValidation:rule="self.all(s, s.resourcePrefixes.all(p, p == '/' || p.endsWith('#')))",message="non-default prefixes must end with '#'"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this rule here vs on the resourcePrefixes field itself?

Shards []ManagedEtcdShardSpec `json:"shards,omitempty"`
}

// EtcdShardPriority defines the operational priority of an etcd shard
// +kubebuilder:validation:Enum=Critical;High;Medium;Low
type EtcdShardPriority string

const (
EtcdShardPriorityCritical EtcdShardPriority = "Critical"
EtcdShardPriorityHigh EtcdShardPriority = "High"
EtcdShardPriorityMedium EtcdShardPriority = "Medium"
EtcdShardPriorityLow EtcdShardPriority = "Low"
)

// ManagedEtcdShardSpec defines configuration for a single managed etcd shard
type ManagedEtcdShardSpec struct {
// name is the unique identifier for this shard
// Must be DNS-1035 compliant (lowercase alphanumeric + hyphens)
// Used for resource naming: etcd-{name}, etcd-{name}-client, etc.
Comment on lines +1942 to +1944

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For consistency with how we describe this validation on other APIs

Suggested change
// name is the unique identifier for this shard
// Must be DNS-1035 compliant (lowercase alphanumeric + hyphens)
// Used for resource naming: etcd-{name}, etcd-{name}-client, etc.
// name is the unique identifier for this shard.
// Must be at most 15 characters in length and consist only of lowercase
// alphanumeric characters and hyphens, and must start with an alphabetic
// character and end with an alphanumeric character.
// Used for resource naming: etcd-{name}, etcd-{name}-client, etc.

// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:XValidation:rule="self.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')",message="name must be DNS-1035 compliant"
// +kubebuilder:validation:MaxLength=15

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why 15? DNS1035 is typically 63 capped. Looks like we are prefixing this, is it always etcd- or are we using other prefixes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should add MinLength=1 too

Name string `json:"name,omitempty"`

// resourcePrefixes specifies which Kubernetes resources are stored in this shard
// Format: "group/resource#" or "/" for default (catch-all)
// Examples: "/events#", "/coordination.k8s.io/leases#", "/"
// Exactly one shard must have "/" as a prefix
Comment on lines +1951 to +1954

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Formatting here won't be preserved when this is generated into documentation. Prefer to use complete sentences.

// +required
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=255
// +listType=set
ResourcePrefixes []string `json:"resourcePrefixes,omitempty"`

// priority determines operational importance and default backup frequency
// Critical: Default backup every 30 minutes
// High: Default backup hourly
// Medium/Low: Default backup disabled
// +optional
// +default="Medium"
Priority EtcdShardPriority `json:"priority,omitempty"`
Comment on lines +1963 to +1969

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this implemented? Do we actually care about this as a feature?


// storage specifies storage configuration for this shard
// If not specified, inherits from ManagedEtcdSpec.Storage
// +optional
Storage ManagedEtcdStorageSpec `json:"storage,omitzero"`

// replicas is the number of etcd replicas for this shard
// Must be 1 or 3. If not specified, defaults based on cluster's
// ControllerAvailabilityPolicy (1 for SingleReplica, 3 for HighlyAvailable)
// +optional
// +kubebuilder:validation:Enum=1;3
Replicas *int32 `json:"replicas,omitempty"`
Comment on lines +1976 to +1981

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Defaulting based on another resource is typically considered bad practice. I think this is what those utils were for, where/when is the defaulting called?


// backupSchedule is the cron schedule for backups (standard cron format)
// If empty, uses priority-based default or disables backups
Comment on lines +1983 to +1984

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAICT there is no way to have the priority field empty currently (there's a default) so I don't know how I would actually represent "please disable backups" in this API 🤔

// Examples: "*/30 * * * *" (every 30 min), "0 * * * *" (hourly)
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=100
BackupSchedule string `json:"backupSchedule,omitempty"`
}

// ManagedEtcdStorageType is a storage type for an etcd cluster.
Expand Down Expand Up @@ -1981,20 +2058,71 @@ type PersistentVolumeEtcdStorageSpec struct {
}

// UnmanagedEtcdSpec specifies configuration which enables the control plane to
// integrate with an eternally managed etcd cluster.
// integrate with an externally managed etcd cluster.
type UnmanagedEtcdSpec struct {
// endpoint is the full etcd cluster client endpoint URL. For example:
//
// https://etcd-client:2379
//
// If the URL uses an HTTPS scheme, the TLS field is required.
//
// +kubebuilder:validation:Pattern=`^https://`
// endpoint is the full etcd cluster client endpoint URL.
// Used only when shards is not specified (legacy single-etcd mode).
// When shards are specified, this field is ignored.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be better to add an XValidation to prevent them from both being set at the same time?

// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:XValidation:rule="self.startsWith('https://')",message="endpoint must start with https://"
// +kubebuilder:validation:MaxLength=255
Endpoint string `json:"endpoint,omitempty"`

// tls specifies TLS configuration for HTTPS etcd client endpoints.
// Used only when shards is not specified (legacy single-etcd mode).
// When shards are specified, this field is ignored.
// +optional
TLS EtcdTLSConfig `json:"tls,omitzero"`

// shards configures etcd sharding by Kubernetes resource kind.
// When not specified, uses endpoint and tls fields (legacy single-etcd mode).
// When specified, exactly one shard must have "/" in its resourcePrefixes.
// +optional
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=10
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:XValidation:rule="self.exists(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix"
// +kubebuilder:validation:XValidation:rule="self.all(s, s.resourcePrefixes.all(p, p == '/' || p.endsWith('#')))",message="non-default prefixes must end with '#'"
Shards []UnmanagedEtcdShardSpec `json:"shards,omitempty"`
}
Comment on lines +2061 to +2089

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Validation gap allows invalid UnmanagedEtcdSpec state.

The endpoint and tls fields are marked optional, and documentation states they're "ignored when shards is specified." However, there's no validation ensuring that when shards is NOT specified, endpoint and tls ARE provided. This allows an invalid state where neither legacy fields nor shards are configured.

Consider adding an XValidation rule:

// +kubebuilder:validation:XValidation:rule="has(self.shards) || (self.endpoint != '' && has(self.tls))",message="either shards must be specified, or both endpoint and tls are required"

The mutual exclusivity validation (preventing both shards and endpoint/tls from being set simultaneously) was also flagged in a previous review.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 2058 - 2085, Add
XValidation rules on the UnmanagedEtcdSpec type to enforce that either Shards is
provided or both Endpoint and TLS are provided, and to prevent supplying
Endpoint/TLS when Shards is set; specifically add a rule like "has(self.shards)
|| (self.endpoint != '' && has(self.tls))" with an appropriate message, and a
second rule that forbids endpoint/tls when shards exists (e.g.
"not(has(self.shards)) || (self.endpoint == '' && not(has(self.tls)))") so
UnmanagedEtcdSpec (type name) cannot be left without configuration and cannot
mix legacy fields (Endpoint, TLS) with Shards.


// UnmanagedEtcdShardSpec defines configuration for a single unmanaged etcd shard
type UnmanagedEtcdShardSpec struct {
// name is the unique identifier for this shard
// Must be DNS-1035 compliant (lowercase alphanumeric + hyphens)
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:XValidation:rule="self.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')",message="name must be DNS-1035 compliant"
// +kubebuilder:validation:MaxLength=15
Name string `json:"name,omitempty"`

// resourcePrefixes specifies which Kubernetes resources are stored in this shard
// Format: "group/resource#" or "/" for default (catch-all)
// Examples: "/events#", "/coordination.k8s.io/leases#", "/"
// Exactly one shard must have "/" as a prefix
// +required
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=255
// +listType=set
ResourcePrefixes []string `json:"resourcePrefixes,omitempty"`

// priority determines operational importance
// +optional
// +default="Medium"
Priority EtcdShardPriority `json:"priority,omitempty"`

// endpoint is the full etcd shard client endpoint URL
// Example: https://etcd-events-client:2379
// +required
// +kubebuilder:validation:Pattern=`^https://`
// +kubebuilder:validation:MaxLength=255
Endpoint string `json:"endpoint"`

// tls specifies TLS configuration for HTTPS etcd client endpoints.
// tls specifies TLS configuration for this shard's HTTPS endpoint
// +required
TLS EtcdTLSConfig `json:"tls"`
}
Expand Down
63 changes: 62 additions & 1 deletion api/hypershift/v1beta1/zz_generated.deepcopy.go

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

Loading
Loading