Skip to content
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

add support for adopting external cluster #306

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

add support for adopting external cluster #306

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
43ae169
initial PR draft
pdettori Dec 9, 2024
3c7e658
add CLI subcommand and more reconciliation logic
pdettori Dec 13, 2024
8eabbb7
finalize implementation and add user docs
pdettori Dec 16, 2024
756f7b7
use metav1 instead of v1 in import
pdettori Dec 16, 2024
28260e7
address some comments for PR #306
pdettori Jan 9, 2025
ecbb432
address more comments for PR 306
pdettori Jan 10, 2025
d80da22
address more comments on PR 306
pdettori Jan 10, 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
20 changes: 18 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ CONTAINER_REGISTRY ?= ghcr.io/kubestellar/kubeflex
# latest tag
LATEST_TAG ?= $(shell git describe --tags $(git rev-list --tags --max-count=1))

# Image URL to use all building/pushing image targets
IMG ?= ghcr.io/kubestellar/kubeflex/manager:latest
KO_DOCKER_REPO ?= ko.local
IMAGE_TAG ?= $(shell git rev-parse --short HEAD)
CMD_NAME ?= manager
IMG ?= ${KO_DOCKER_REPO}/${CMD_NAME}:${IMAGE_TAG}

# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
ENVTEST_K8S_VERSION = 1.26.1

Expand Down Expand Up @@ -195,6 +198,19 @@ chart: manifests kustomize
@mkdir -p chart/crds
$(KUSTOMIZE) build config/crd > chart/crds/crds.yaml

.PHONY: ko-local-build
ko-local-build:
KO_DOCKER_REPO=${KO_DOCKER_REPO} ko build -B ./cmd/${CMD_NAME} -t ${IMAGE_TAG} --platform linux/${ARCH}

# this is used for local testing
.PHONY: kind-load-image
kind-load-image:
kind load docker-image ${IMG} --name kubeflex

.PHONY: install-local-chart
install-local-chart: chart kind-load-image
helm upgrade --install --create-namespace -n kubeflex-system kubeflex-operator ./chart

##@ Build Dependencies

## Location to install dependencies to
Expand Down
2 changes: 1 addition & 1 deletion api/v1alpha1/conditions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,5 @@ func generateCondition(ctype ConditionType, reason ConditionReason, message stri
}

func addTime(t time.Duration) metav1.Time {
return metav1.NewTime(time.Now().Add(2 * time.Hour))
return metav1.NewTime(time.Now().Add(t))
}
20 changes: 14 additions & 6 deletions api/v1alpha1/controlplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,17 @@ import (

// ControlPlaneSpec defines the desired state of ControlPlane
type ControlPlaneSpec struct {
Type ControlPlaneType `json:"type,omitempty"`
Backend BackendDBType `json:"backend,omitempty"`
PostCreateHook *string `json:"postCreateHook,omitempty"`
PostCreateHookVars map[string]string `json:"postCreateHookVars,omitempty"`
Type ControlPlaneType `json:"type,omitempty"`
Backend BackendDBType `json:"backend,omitempty"`
// BootstrapSecretRef contains a reference to the kubeconfig used to bootstrap adoption of
// an external cluster
// +optional
Copy link
Contributor

@MikeSpreitzer MikeSpreitzer Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is an "inline union": some unconditional fields plus a discriminator (Type) that determines whether some of the other fields should be present or absent. FYI, these are frowned upon in the Kubernetes API design community. They prefer pure unions: a pure union struct has only a discriminator plus fields whose proper presence is determined by the discriminator.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct, but I am concerned about introducing a non-backward compatible change if we go with the pure union. Just to be clear, is something like the following what you have in mind for the "pure union"?

type OffClusterSpec struct {
    BootstrapSecretRef           *SecretReference
    AdoptedTokenExpirationSeconds *int64
}

type InClusterSpec struct {
    Backend                      BackendDBType
    PostCreateHook     *string
    PostCreateHookVars map[string]string
}

type ControlPlaneSpec struct {
    Type          ControlPlaneType      `json:"type,omitempty"`
    inCluster         * InClusterSpec    `json:"inCluster,omitempty"`
    offCluster         * OffClusterSpec    `json:"offCluster,omitempty"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What you showed looks like a pure union.

BootstrapSecretRef *SecretReference `json:"bootstrapSecretRef,omitempty"`
// expiration time for token of adopted cluster
// +optional
MikeSpreitzer marked this conversation as resolved.
Show resolved Hide resolved
AdoptedTokenExpirationSeconds *int64 `json:"adoptedTokenExpirationSeconds,omitempty"`
MikeSpreitzer marked this conversation as resolved.
Show resolved Hide resolved
PostCreateHook *string `json:"postCreateHook,omitempty"`
PostCreateHookVars map[string]string `json:"postCreateHookVars,omitempty"`
}

// ControlPlaneStatus defines the observed state of ControlPlane
Expand Down Expand Up @@ -71,14 +78,15 @@ const (
BackendDBTypeDedicated BackendDBType = "dedicated"
)

// +kubebuilder:validation:Enum=k8s;ocm;vcluster;host
// +kubebuilder:validation:Enum=k8s;ocm;vcluster;host;external
type ControlPlaneType string

const (
ControlPlaneTypeK8S ControlPlaneType = "k8s"
ControlPlaneTypeOCM ControlPlaneType = "ocm"
ControlPlaneTypeVCluster ControlPlaneType = "vcluster"
ControlPlaneTypeHost ControlPlaneType = "host"
ControlPlaneTypeExternal ControlPlaneType = "external"
)

// We do not use ObjectReference as its use is discouraged in favor of a locally defined type.
Expand All @@ -90,7 +98,7 @@ type SecretReference struct {
// `name` is the name of the secret.
// Required
Name string `json:"name"`
// Required
// +optional
Key string `json:"key"`
// Required
InClusterKey string `json:"inClusterKey"`
Expand Down
10 changes: 10 additions & 0 deletions api/v1alpha1/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.

34 changes: 30 additions & 4 deletions chart/crds/crds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,40 @@ spec:
spec:
description: ControlPlaneSpec defines the desired state of ControlPlane
properties:
adoptedTokenExpirationSeconds:
description: expiration time for token of adopted cluster
format: int64
type: integer
backend:
enum:
- shared
- dedicated
type: string
bootstrapSecretRef:
description: |-
BootstrapSecretRef contains a reference to the kubeconfig used to bootstrap adoption of
an external cluster
properties:
inClusterKey:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please clarify what is inClusterKey?

Perhaps I missed, but I did not see a documentation of what the bootstrap secret should look like. This is useful if somebody wants/needs to create it without using kflex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You may see the defaults being used for the bootstrap secret in cmd/kflex/adopt/adopt.go. The name is defined in:

func GenerateBoostrapSecretName(cpName string) string {
	return fmt.Sprintf("%s-bootstrap", cpName)
}

namespace is SystemNamespace = "kubeflex-system" and key is KubeconfigSecretKeyInCluster = "kubeconfig-incluster"

Copy link
Contributor

Choose a reason for hiding this comment

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

To further clarify, inClusterKey is the name of the secret generated by kflex from the bootstrap secret... right?

Since you have a default, can you make it not required?

Copy link
Collaborator Author

@pdettori pdettori Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To further clarify, inClusterKey is the name of the secret generated by kflex from the bootstrap secret

No, inClusterKey is the name of the key used for the kubeconfig, which is kubeconfig-incluster. The name of the secret would be <cp-name>-bootstrap. You do not have to provide these values in the ControlPlane CR, they are optional.

Copy link
Contributor

@MikeSpreitzer MikeSpreitzer Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(A) Since the golang source re-uses the existing type SecretReference, regularity is implied here. It is a reference to the same sort of thing. Same schema for contents. (And, BTW, the expectation(s) on Secret contents should be documented in the comment on the SecretReference type.) I am not sure yet whether this usage really is regular.

(B) @pdettori, what do you mean by "You do not have to provide these values in the ControlPlane CR, they are optional"? Which values are optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MikeSpreitzer - re. (B) - what I stated previously was not correct. The bootstrap secret must be provided in the secret reference. Please check "Creating a control plane of type external with the API" for an example ControlPlane CR with the bootstrapSecretRef.

description: Required
type: string
key:
type: string
name:
description: |-
`name` is the name of the secret.
Required
type: string
namespace:
description: |-
`namespace` is the namespace of the secret.
Required
type: string
required:
- inClusterKey
- name
- namespace
type: object
postCreateHook:
type: string
postCreateHookVars:
Expand All @@ -71,6 +100,7 @@ spec:
- ocm
- vcluster
- host
- external
type: string
type: object
status:
Expand Down Expand Up @@ -119,7 +149,6 @@ spec:
description: Required
type: string
key:
description: Required
type: string
name:
description: |-
Expand All @@ -133,7 +162,6 @@ spec:
type: string
required:
- inClusterKey
- key
- name
- namespace
type: object
Expand Down Expand Up @@ -253,7 +281,6 @@ spec:
description: Required
type: string
key:
description: Required
type: string
name:
description: |-
Expand All @@ -267,7 +294,6 @@ spec:
type: string
required:
- inClusterKey
- key
- name
- namespace
type: object
Expand Down
5 changes: 3 additions & 2 deletions chart/templates/operator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ spec:
- --secure-listen-address=0.0.0.0:8443
- --upstream=http://127.0.0.1:8080/
- --logtostderr=true
- --v={{.Values.verbosity}}
- --v=0
MikeSpreitzer marked this conversation as resolved.
Show resolved Hide resolved
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.13.1
name: kube-rbac-proxy
ports:
Expand All @@ -614,12 +614,13 @@ spec:
- --health-probe-bind-address=:8081
- --metrics-bind-address=127.0.0.1:8080
- --leader-elect
- --zap-log-level={{max (.Values.verbosity | default 2 | int) 1}}
env:
- name: HELM_CONFIG_HOME
value: /tmp
- name: HELM_CACHE_HOME
value: /tmp
image: ghcr.io/kubestellar/kubeflex/manager:latest
image: ko.local/manager:756f7b7
imagePullPolicy: IfNotPresent
livenessProbe:
httpGet:
Expand Down
Loading
Loading