Skip to content

feat: add SKE managed cluster and node pool APIs - #36

Draft
Rana-Ashar wants to merge 2 commits into
mainfrom
feature/stackit-managed-specs
Draft

Rana-Ashar wants to merge 2 commits into
mainfrom
feature/stackit-managed-specs

Conversation

@Rana-Ashar

Copy link
Copy Markdown

This PR adds the initial SKE managed cluster and node pool APIs to CAPSTK.

It introduces StackitManagedCluster and StackitManagedNodePool specifications, Cluster API v1beta2 contract fields, generated CRDs and RBAC, sample manifests, initial controllers, and tests.

// region is the STACKIT region in which infrastructure is created.
// +required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Pattern=`^[a-z]{2}[0-9]{2}$`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this an official pattern you got from our stackit api docs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This pattern is not coming from the SKE api docs. I reused the existing validation from StackitClusterSpec.Region.

Comment on lines +77 to +85
// providerIDList are the identification IDs of machine instances provided by
// the provider. This field must match the provider IDs as seen on the Node
// objects corresponding to a MachinePool's machine instances.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=10000
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=512
ProviderIDList []string `json:"providerIDList,omitempty"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this does not belong into the spec that is something that we retrieve from the stackit api and store inside the status

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

or where did you get the idea from putting this inside the spec?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This placement comes directly from the CAPI v1beta2 InfraMachinePool contract.

CAPI requires an InfraMachinePool to expose the instance provider IDs as spec.providerIDList. Core CAPI reads that field to match the MachinePool instances with Kubernetes Nodes and to track deletion.

So although this is controller-populated information rather than user desired state, it intentionally lives in spec for CAPI compatibility. I think we should keep it here for that reason.

https://cluster-api.sigs.k8s.io/developer/providers/contracts/infra-machinepool.html

Comment thread cloud/services/ske/ske.go
Comment on lines +26 to +54
// ManagedClusterDesiredState holds the cluster-level fields needed by the SKE
// service layer.
type ManagedClusterDesiredState struct {
ProjectID string
Region string
ClusterName string
KubernetesVersion string
NetworkID string
Labels map[string]string
}

// ManagedNodePoolDesiredState holds the node-pool-level fields needed by the
// SKE service layer.
type ManagedNodePoolDesiredState struct {
Name string
AvailabilityZones []string
MachineType string
ImageName string
ImageVersion string
Minimum int32
Maximum int32
VolumeSize int32
VolumeType string
AllowSystemComponents *bool
Labels map[string]string
Taints []ManagedNodePoolTaint
MaxSurge *int32
MaxUnavailable *int32
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the desired state should be reflected by the CRs themselves why do we need these "shadow" structs that are essentially just code duplication?

Or was this added as prematurely for the actually API abstraction/implementation?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants