WIP: Adding initial proof of concept creating nested CRDs for each object #1

christopherhein · 2020-10-20T16:52:01Z

Adding an initial Proof of Concept for CAPN style, this is not full featured but currently implements part of the ControlPlane provider, the idea is that when a control plane is created it can downstream create the objects/components which are reconciled separately allowing us to swap out specific implementations, for example this currently will take an NestedControlPlane and will kick off creating a NestedPKI (Not necessarily the greatest name but solves to prove the model) the NestedPKI controller will create all the object necessary like the rootca, apiserver crt and key, etc crt and key. This pulls a lot from the original VC for implementation but could be easily replaced if you needed to implement a custom PKI generator.

This PR doesn't necessarily need to be merged but serves to help explain one path.

Signed-off-by: Chris Hein [email protected]

Signed-off-by: Chris Hein <[email protected]>

k8s-ci-robot · 2020-10-20T16:52:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein

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:

~~OWNERS~~ [christopherhein]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

christopherhein · 2020-10-20T16:52:12Z

/hold

christopherhein · 2020-10-20T17:16:16Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// annotations that are not visible to Nested Cluster but are kept in
+	// super master. For example, the annotations added by syncer controller.
+	// +optional
+	OpaqueMetaPrefixes []string `json:"opaqueMetaPrefixes,omitempty"`


this should be removed an implemented somewhere else since this specific to VC

christopherhein · 2020-10-20T17:26:02Z

controllers/controlplane/nestedpki_controller.go

+// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=nestedpkis,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=nestedpkis/status,verbs=get;update;patch
+
+func (r *NestedPKIReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {


Could we implement some of these functions with cert-manager

mtaufen · 2020-10-20T23:59:49Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// cluster PKI, if not set the PKI will expire after 1 year.
+	// +optional
+	// +kubebuilder:default=365
+	PKIExpireDays int64 `json:"pkiExpireDays,omitempty"`


I saw PKI and had some quick thoughts:

it feels rather coarse to set the expiry at the granularity of the whole PKI, rather than specific credentials

are you thinking about rotation yet?

Why put this in NestedControlPlaneSpec instead of NestedPKI?

I think we should err on the side of caution with adding fields. If we don't need to expose this as a user-configurable option yet, I think we should not do it. The above questions raise great points, and I am not sure we have enough of an understanding of the problem space to know where best to put it just yet.

Why put this in NestedControlPlaneSpec instead of NestedPKI?

👍 - I also think this is a question we should answer more generally. What is the purpose of the NestedControlPlane vs the NestedControlPlaneTemplate resource? What defines what should go where?

enxebre · 2020-10-21T11:07:13Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// TemplateRef allows you to specify the control plane template to create
+	// the control plane.
+	// +optional
+	TemplateRef *corev1.ObjectReference `json:"configRef,omitempty"`


just to clarify on what a template is here:
is the NestedControlPlane controller meant to create additional CR instances out of this templateRef as in https://github.com/kubernetes-sigs/cluster-api/blob/master/controllers/machineset_controller.go#L344?
Or is this referencing a CR which fields input will just be consumed by the controller? If this is the latter we should may reconsider having "template" in the naming.

This TemplateRef will refer to an NestedControlPlaneTemplate object. There may exist multiple NestedControlPlaneTemplate objects, and each NestedControlPlane object will refer to one of them. The NestedControlPlane controller will then create CR NestedKAS, NestedEtcd and NestedKCM, And the NestedKAS, NestedEtcd and the NestedKCM controllers will actually start each component.

enxebre · 2020-10-21T11:08:11Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// receive requests and that the VPC infra is ready.
+	// +optional
+	Ready bool `json:"ready"`
+


should probably also include a field to signal error here. E.g

// ErrorMessage indicates that there is a terminal problem reconciling the // state, and will be set to a descriptive error message. Probably to be done in hand with conditions. // +optional FailureMessage *string `json:"failureMessage,omitempty"`

Though this can probably be detailed later when introducing conditions

enxebre · 2020-10-21T11:09:52Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+// +kubebuilder:printcolumn:name="ClusterNamespace",type="string",JSONPath=".status.clusterNamespace",description="Namespace of the cluster",priority=1
+
+// NestedControlPlane is the Schema for the nestedcontrolplanes API
+type NestedControlPlane struct {


Just out of curiosity if there has been any discussion for naming nested vs hosted?

munnerz

I've not reviewed all changes in here as I think we need to take a step back and write up a document outlining what CRDs we are adding and their intended purpose/meaning.

As it stands it seems like we're adding in a lot of things that we know we want to implement, without first establishing the baseline definitions for what does what, and how it works.

There's a few points in here that intend to make it 'general purpose' so anyone can bring along their own implementation of how to provision a nested cluster. What's the model that an implementor should take to do this? Is it to create their own CRD types and reference them from templates? Is it to just build a controller that watches for NestedCluster resources and take appropriate action?

munnerz · 2020-10-21T11:04:56Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// prefixes used for all Namespaces in the cluster.
+	// e.g. {namespace}-{vcp.uid}-{vc-namespace}
+	// +optional
+	ClusterNamespace string `json:"clusterNamespace,omitempty"`


What's the vcp.uid? We should make these doc comments as clear as possible for the 'layman' user and avoid ambiguous terms if possible 😄

This also makes up the prefixes used for all Namespaces in the cluster. e.g. {namespace}-{vcp.uid}-{vc-namespace}

This seems okay for now. In future though it may be worth having some kind of namespacePrefix field to explicitly set this to some other value. It may be worth noting here that whilst it is true today, it should not be relied upon? If so, can we expose the chosen prefix through a status field to make it easier for other consumers of this API to make use of/build upon?

Also, I think there needs to be a length restriction on this field. We should also call out how the length of this field influences the restrictions on namespace names within nested clusters too.

+1 for the namespacePrefix

We need to call out this is typically defaulted and that it needs to have a max length validation and warning about it's usage for conflicting namespace prefixes, also rename to NamespacePrefix

munnerz · 2020-10-21T11:08:15Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// cluster. e.g. a pod dns will be
+	// {some-pod}.{some-namespace}.svc.{ClusterDomain}
+	// +kubebuilder:default=cluster.local
+	ClusterDomain string `json:"clusterDomain,omitempty"`


I think this should also have // +optional tags? Though perhaps kubebuilder automatically marks this as optional if it is omitempty? 👀

munnerz · 2020-10-21T11:09:33Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// TemplateRef allows you to specify the control plane template to create
+	// the control plane.
+	// +optional
+	TemplateRef *corev1.ObjectReference `json:"configRef,omitempty"`


Is this a reference to an object that itself describes how to provision the cluster? Does this refer to a NestedControlPlaneTemplate? I see this is marked optional - what happens if it is not specified?

Maybe we should create a default NestedControlPlaneTemplate, which will be used when the TemplateRef is not set.

munnerz · 2020-10-21T11:12:48Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// cluster PKI, if not set the PKI will expire after 1 year.
+	// +optional
+	// +kubebuilder:default=365
+	PKIExpireDays int64 `json:"pkiExpireDays,omitempty"`


I think we should err on the side of caution with adding fields. If we don't need to expose this as a user-configurable option yet, I think we should not do it. The above questions raise great points, and I am not sure we have enough of an understanding of the problem space to know where best to put it just yet.

Why put this in NestedControlPlaneSpec instead of NestedPKI?

👍 - I also think this is a question we should answer more generally. What is the purpose of the NestedControlPlane vs the NestedControlPlaneTemplate resource? What defines what should go where?

munnerz · 2020-10-21T11:42:07Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// meta data are generated by super master controllers, which are needed by
+	// nested cluster to interact with external systems.
+	// +optional
+	TransparentMetaPrefixes []string `json:"transparentMetaPrefixes,omitempty"`


I'm not too sure what this means - does "back populated" mean synced? It'd really help if we had a proposal document that explained some of this stuff and set out some definitions.

munnerz · 2020-10-21T11:43:51Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// Ready denotes that the NestedControlPlane API Server is ready to
+	// receive requests and that the VPC infra is ready.
+	// +optional
+	Ready bool `json:"ready"`


I don't think this field should exist as-is. If we are going to have an overarching concept of readiness, it should be denoted as a condition. That said, I think we should be careful to clearly define what we call ready or not. If an apiserver is failing 50% of requests, is it ready?

Can probably use ReadyReplicas since we'll likely have multiple replicas, this is pulled from the Control Plane types for CAPI -https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane.html#required-status-fields

munnerz · 2020-10-21T11:49:08Z

apis/controlplane/v1alpha3/nestedcontrolplanetemplate_types.go

+	// Components allows you to specify the multiple components necessary to
+	// provision a Virtual Control Plane
+	// +optional
+	Components []configv1alpha1.Addon `json:"components,omitempty"`


How does KubernetesVersion and Components relate? What if I define a component that uses an image on version 1.16 of k8s, and the KubernetesVersion is set to 1.18?

IMO, the Components should have higher privilege, sometimes users may wanna try out customized component.

In additon, Maybe we can use the KubernetesVersion as the default component version? say, the KubernetesVersion is set to 1.16 while only the KAS and etcd addon is provided, then the default 1.16 KCM template will be used.

munnerz · 2020-10-21T11:50:20Z

apis/controlplane/v1alpha3/nestedcontrolplanetemplate_types.go

+	// Default allows you to set this NestedTemplate as a a default template,
+	// by setting this to true any new NestedClusters will be created without
+	// a TemplateRef will be deployed with this template.
+	Default bool `json:"default,omitempty"`


I'd expect something like default templates to be a feature we add further down the line - initially, in this early development phase, requiring users to specify something explicitly works fine. If we introduce this without consideration, we end out not answering questions like "what if multiple templates exist that are all marked default?"

Use a validating webhook to prevent multiple templates from being marked as default?

charleszheng44 · 2020-10-23T18:09:16Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// prefixes used for all Namespaces in the cluster.
+	// e.g. {namespace}-{vcp.uid}-{vc-namespace}
+	// +optional
+	ClusterNamespace string `json:"clusterNamespace,omitempty"`


+1 for the namespacePrefix

charleszheng44 · 2020-10-23T18:19:30Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// TemplateRef allows you to specify the control plane template to create
+	// the control plane.
+	// +optional
+	TemplateRef *corev1.ObjectReference `json:"configRef,omitempty"`


This TemplateRef will refer to an NestedControlPlaneTemplate object. There may exist multiple NestedControlPlaneTemplate objects, and each NestedControlPlane object will refer to one of them. The NestedControlPlane controller will then create CR NestedKAS, NestedEtcd and NestedKCM, And the NestedKAS, NestedEtcd and the NestedKCM controllers will actually start each component.

charleszheng44 · 2020-10-23T18:21:28Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	// TemplateRef allows you to specify the control plane template to create
+	// the control plane.
+	// +optional
+	TemplateRef *corev1.ObjectReference `json:"configRef,omitempty"`


Maybe we should create a default NestedControlPlaneTemplate, which will be used when the TemplateRef is not set.

charleszheng44 · 2020-10-23T18:30:12Z

apis/controlplane/v1alpha3/nestedcontrolplane_types.go

+	Ready bool `json:"ready"`
+
+	// Conditions specifies the cpnditions for the managed control plane
+	// Conditions clusterv1.Conditions `json:"conditions,omitempty"`


Are we going to use the Conditions? I think it could be useful for debugging and monitoring.

Yea, we need to have conditions on these types. This should follow the base required fields from CAPI - https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane.html

charleszheng44 · 2020-10-23T18:40:44Z

apis/controlplane/v1alpha3/nestedcontrolplanetemplate_types.go

+	// Components allows you to specify the multiple components necessary to
+	// provision a Virtual Control Plane
+	// +optional
+	Components []configv1alpha1.Addon `json:"components,omitempty"`


IMO, the Components should have higher privilege, sometimes users may wanna try out customized component.

In additon, Maybe we can use the KubernetesVersion as the default component version? say, the KubernetesVersion is set to 1.16 while only the KAS and etcd addon is provided, then the default 1.16 KCM template will be used.

charleszheng44 · 2020-10-23T18:41:50Z

apis/controlplane/v1alpha3/nestedcontrolplanetemplate_types.go

+	// Default allows you to set this NestedTemplate as a a default template,
+	// by setting this to true any new NestedClusters will be created without
+	// a TemplateRef will be deployed with this template.
+	Default bool `json:"default,omitempty"`


Use a validating webhook to prevent multiple templates from being marked as default?

charleszheng44 · 2020-10-23T18:43:36Z

apis/controlplane/v1alpha3/nestedetcd_types.go

+// +kubebuilder:resource:scope=Namespaced,categories=cluster-api;capn;capi,shortName=ne
+
+// NestedEtcd is the Schema for the nestedetcds API
+type NestedEtcd struct {


There will also be a NestedKAS and a NestedKCM, right?

christopherhein · 2020-11-14T09:18:53Z

/close

k8s-ci-robot · 2020-11-14T09:19:01Z

@christopherhein: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

adding initial proof of concept creating nested CRDs for each object

a64adcb

Signed-off-by: Chris Hein <[email protected]>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 20, 2020

k8s-ci-robot requested review from justinsb and vincepri October 20, 2020 16:52

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2020

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 20, 2020

christopherhein marked this pull request as draft October 20, 2020 16:52

christopherhein commented Oct 20, 2020

View reviewed changes

mtaufen reviewed Oct 20, 2020

View reviewed changes

enxebre reviewed Oct 21, 2020

View reviewed changes

munnerz reviewed Oct 21, 2020

View reviewed changes

charleszheng44 reviewed Oct 23, 2020

View reviewed changes

k8s-ci-robot closed this Nov 14, 2020

christopherhein mentioned this pull request Nov 17, 2020

Proposal: define the CRD of nested components #11

Merged

christopherhein deleted the feature/capi-poc branch June 9, 2021 16:42

christopherhein mentioned this pull request Jul 5, 2021

The pod is always pending in the nested cluster #156

Open

WIP: Adding initial proof of concept creating nested CRDs for each object #1

WIP: Adding initial proof of concept creating nested CRDs for each object #1

Conversation

christopherhein commented Oct 20, 2020

k8s-ci-robot commented Oct 20, 2020

christopherhein commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleszheng44 Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleszheng44 Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherhein commented Nov 14, 2020

k8s-ci-robot commented Nov 14, 2020

charleszheng44 Oct 23, 2020 •

edited

Loading

charleszheng44 Oct 23, 2020 •

edited

Loading