add support for adopting external cluster

[pdettori]

Summary

Adds support for importing (adopting) external cluster as control plane

Related issue(s)

Fixes #163

[MikeSpreitzer]

FYI, all writes should supply a FieldManager in the XXXOptions.

[pdettori]

I prefer to leave this to a separate campaign/PR as there are many other places where there are writes so should probably be done consistently everywhere.

[MikeSpreitzer]

Yes, WIP. This still needs a doc update. I do not understand how the user passes the kubeconfig of the cluster to be adopted into the adopt subcommand. The adopt subcommand needs to get added into the command tree.

[pdettori]

Yes, WIP. This still needs a doc update. I do not understand how the user passes the kubeconfig of the cluster to be adopted into the adopt subcommand. The adopt subcommand needs to get added into the command tree.

@MikeSpreitzer - correct on all points - I started with the easiest updates, now working on the "adopt" subcommand and getting the kubeconfig. The current idea is that the user will supply the name of the context (and optionally the path of the Kubeconfig) in the Kubeconfig for the cluster to adopt as flags of the adopt subcommand, and the such subcommand will take care of creating ClusterRoles, ClusterRoleBindings, SA and Token in the adopted cluster so that the Token + cluster endpoint + CA cert can be used to build a new Kubeconfig for the adopted cluster to store in a secret in the namespace associated with the control plane in the hosting cluster.

[francostellari]

@pdettori I think you have some changes in chart/templates/operator.yaml that may be accidental

[francostellari]

Could a separated kubeconfig be passed with KUBECONFIG=... kflex... or `kflex --kubeconfig '?

[francostellari]

if no value is passed to --adopted-context could it default to the current one... especially if one passes a separate kubeconfig with only one context in it?

[francostellari]

As this command both create the secret and create the CP... it would be hard to use in combination with KS future Helm chart. The process would be

1. create/push the secret in the cluster manually or via kflex.
2. tell KS helm to create a CP using it (either upon installation or later on)

[pdettori]

Could a separated kubeconfig be passed with KUBECONFIG=... kflex... or `kflex --kubeconfig '?

Yes. These are all the options for the adopt subcommand. You'd be looking at the --adopted-kubeconfig flag.

$ kflex adopt --help
Adopt a control plane from an external cluster and switches the Kubeconfig context to
                the current instance

Usage:
  kflex adopt <name> [flags]

Flags:
  -c, --adopted-context string           path to adopted cluster context in adopted kubeconfig
  -a, --adopted-kubeconfig string        path to adopted cluster kubeconfig file. If unspecified, it uses the default Kubeconfig
  -s, --chatty-status                    chatty status indicator (default true)
  -x, --expiration-seconds int           adopted token expiration in seconds. Default is one year. (default 31536000)
  -h, --help                             help for adopt
  -k, --kubeconfig string                path to kubeconfig file
  -p, --postcreate-hook string           name of post create hook to run
  -e, --set stringArray                  set post create hook variables, in the form name=value 
  -d, --skip-url-override                skip URL override, used for local debugging only
  -u, --url-override https://127.0.0.1   URL overrride for adopted cluster. Required when cluster address uses local host address, e.g. https://127.0.0.1 
  -v, --verbosity int                    log level

[pdettori]

if no value is passed to --adopted-context could it default to the current one... especially if one passes a separate kubeconfig with only one context in it?

Currently you do need to pass the context. It might be confusing in the common developer scenario where there is one kubeconfig with a context for each cluster, since the "current" context is typically the kubeflex hosting cluster context. It might be possible when a different Kubeconfig is used (which needs to be specified with --adopted-kubeconfig) to check if there is only one context and in that case not require the use of --adopted-context. But I would leave this as a future feature/PR if we really think it is useful.

[MikeSpreitzer]

Since a kubeconfig has a "current context", the natural default for --adopted-context is the current context of the adopted kubeconfig.

[pdettori]

The example above is probably outdated... pls. look at the update user doc in the section "Creating a control plane of type external with the API"

[pdettori]

In any case it might be challenging to guess what is the default context if both the kubeflex hosting cluster and the external cluster are kind clusters created in the same host machine.

[MikeSpreitzer]

with the API ? This thread is a comment on the CLI.

Each kubeconfig has a well-defined "curent context", no guessing needed. That field can be unset, in which case i think it would be OK for the CLI to object (if it matters).

[pdettori]

Sorry for the confusion. It is true that the thread started as a comment for the kflex CLI adopt subcommand, but then @francostellari was asking clarifications about the bootstrap secret in order to use the helm chart to adopt a control plane, so the discussion shifted on using kubectl create secret generic and kubectl --kubeconfig=$SOURCE_KUBECONFIG config view --minify --flatten to create the bootstrap secret - and that part is documented now in Creating a control plane of type external with the API

[francostellari]

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.

[pdettori]

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"

[francostellari]

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?

[pdettori]

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.

[MikeSpreitzer]

(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?

[pdettori]

@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.

[MikeSpreitzer]

I suspect that the intended significance of "external" may not be clear to readers who do not already know what we are trying to say. I would have words here explaining that this type of ControlPlane is a representation of a preexisting cluster, one that was not created by KubeFlex. And not the hosting cluster.

[pdettori]

Updated in latest commit.

[MikeSpreitzer]

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.

[pdettori]

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"`
}

[MikeSpreitzer]

What you showed looks like a pure union.

[MikeSpreitzer]

What sets cp.Spec.AdoptedTokenExpirationSeconds?

[pdettori]

The default of 1 year is set as follows:

1. as a default for the expiration-seconds flag in the adopt subcommand:
   

   kubeflex/cmd/kflex/main.go

   Line 243 in 756f7b7

   adoptCmd.Flags().IntVarP(&adoptedTokenExpirationSeconds, "expiration-seconds", "x", 86400*365, "adopted token expiration in seconds. Default is one year.")

2. if not set in the CR (e.g. because using the API instead of the CLI), the same default is set by the controller logic in

   kubeflex/pkg/reconcilers/external/kubeconfig.go

   Lines 194 to 196 in 756f7b7

   	if expirationSeconds == nil {
   	expirationSeconds = &defaultAdoptedTokenExpirationSeconds
   	}

[MikeSpreitzer]

Nothing reads CPAdopt.AdoptedTokenExpirationSeconds.

[pdettori]

good catch. Fixed in latest commit.

[MikeSpreitzer]

Finished a review. Left individual comments.

[MikeSpreitzer]

Why?

[pdettori]

aren't normally TLS certs used to authenticate? but I am fine to remove this check if you think it's a valid scenario.

[MikeSpreitzer]

I was just thinking that this code should not reject things that are positively problematic.

Now it occurs to me to also consider what would be confusing to accept. This code is going to construct and use a ServiceAccount token regardless of whether a username and password are supplied here. Perhaps it would be clearest to reject a username and password, until the code is prepared to use them instead of a ServiceAccount token.

[MikeSpreitzer]

I have finished another round of review, this time the branch pointed at commit 28260e7

[MikeSpreitzer]

A comment on a public field of a public type should begin with the name of the field. This is a general rule of the Go language. FYI, in the Kubernetes world there is a preference --- in the case of part of an API object --- to cite the name as it appears on JSON (initial lowercase) rather than as it appears in the Go source (initial uppercase) and code-quoted with backticks. This is for the benefit of readers of the generated documentation, who are more likely to be writing JSON/YAML than Go. This preference is not strictly followed or enforced.

[pdettori]

updated in latest commit.

[MikeSpreitzer]

The "KubeFlex hosting cluster" is not "hosted by KubeFlex". If your intent was to exclude this then I would be clearer on that point. Perhaps something like the following?

external: this control plane type represents an existing cluster that was not created by KubeFlex and is not the KubeFlex hosting cluster

[pdettori]

Sounds good to me. Updated.

[MikeSpreitzer]

"Ensure that both your hosting and adopted clusters are on the same Docker network" redundant with the remark just above that "This setup requires that the external cluster ext1 and the KubeFlex hosting cluster are on the same docker network."

[pdettori]

removed the redundant sentence.

[MikeSpreitzer]

"This setup" is not entirely clear. Is it saying that adoption can only be done with both clusters are on the same Docker network (I hope this is not true), or is it saying that this example involves two clusters on the same Docker network? If the latter then I would reword to say something like "This example supposes that ...".

[pdettori]

Updated as suggested.

[MikeSpreitzer]

The wording here is not quite on point; the address is not going into environment variable, it is going into the file.

[pdettori]

Updated to clarify that.

[MikeSpreitzer]

I have finished another round of review.