Add capi to supported providers #26

ranatrk · 2023-12-13T16:37:03Z

Issue weaveworks/weave-gitops-enterprise#3528

Add capi to providers
Add capi support to controller reconcile loop

Add capi crd regenerate cluster discovery crds to include capi

… test

Add condition to check for cluster kubeconfig before creating the secret

… existing clusters in reconcile loop

bigkevmcd · 2023-12-18T07:32:48Z

api/v1alpha1/automatedclusterdiscovery_types.go

@@ -28,13 +28,17 @@ type AKS struct {
 	SubscriptionID string `json:"subscriptionID"`
 }

+// CAPI defines the desired state of CAPI, TBD
+type CAPI struct {
+}


Should we let you filter by labels / namespace?

bigkevmcd · 2023-12-18T07:33:08Z

api/v1alpha1/automatedclusterdiscovery_types.go

@@ -44,6 +48,8 @@ type AutomatedClusterDiscoverySpec struct {

 	AKS *AKS `json:"aks,omitempty"`

+	CAPI *CAPI `json:"capi,omitempty"`
+


We could document these?

What does this actually mean?

Also, maybe given that we don't use it, we don't actually need it?

bigkevmcd · 2023-12-18T07:33:29Z

config/crd/bases/clusters.weave.works_automatedclusterdiscoveries.yaml

@@ -52,6 +52,9 @@ spec:
                required:
                - subscriptionID
                type: object
+              capi:
+                description: CAPI defines the desired state of CAPI, TBD


This doesn't look right, or up-to-date?

@bigkevmcd I removed the TBD, but for now CAPI struct is empty, I added it to be consistent and if we need to add something in the future to it.

bigkevmcd · 2023-12-18T07:36:08Z

internal/controller/automatedclusterdiscovery_controller.go

+
+	logger.Info("creating secret", "name", secret.GetName())
+	if err := controllerutil.SetOwnerReference(gitopsCluster, secret, r.Scheme); err != nil {
+		return nil, fmt.Errorf("failed to set ownership on created Secret: %w", err)


Let's logger.Error this?

bigkevmcd · 2023-12-18T07:36:56Z

internal/controller/automatedclusterdiscovery_controller.go

+		secret.Data["value"] = value
+
+		return nil
+	})


You could simplify this to if _, err := ... ; err != nil { ...

bigkevmcd · 2023-12-18T07:37:31Z

pkg/providers/capi/capi.go

+
+var _ providers.Provider = (*CAPIProvider)(nil)
+
+func NewCAPIProvider(client client.Client) *CAPIProvider {


Exported...so, we should have a Doc comment.

bigkevmcd · 2023-12-18T07:38:02Z

pkg/providers/capi/capi.go

+			ID:         string(capiCluster.GetObjectMeta().GetUID()),
+			KubeConfig: nil,
+		})
+		// TODO: kubeconfig is in a secret in the namespace of the cluster called clustername-kubeconfig


I think we can remove this comment now?

bigkevmcd · 2023-12-18T07:39:03Z

pkg/providers/capi/capi.go

+	return clusters, nil
+}
+
+func (p *CAPIProvider) ClusterID(ctx context.Context, kubeClient client.Reader) (string, error) {


How about a comment in this to explain that the ProviderCluster has an ID and we can't find a Cluster ID in the cluster so these won't match so that later folks can understand this 😄

bigkevmcd · 2023-12-18T07:40:36Z

pkg/providers/capi/capi.go

+
+	for _, capiCluster := range capiClusters.Items {
+		clusters = append(clusters, &providers.ProviderCluster{
+			Name:       capiCluster.Name,


Should we return the labels from the CAPI Cluster for https://github.com/weaveworks/cluster-reflector-controller/blob/main/pkg/providers/provider.go#L39?

bigkevmcd · 2023-12-18T10:25:24Z

pkg/providers/capi/capi.go

+func (p *CAPIProvider) ListClusters(ctx context.Context) ([]*providers.ProviderCluster, error) {
+	kubeClient := p.Kubeclient
+	capiClusters := &capiclusterv1.ClusterList{}
+	err := kubeClient.List(ctx, capiClusters, &client.ListOptions{})


This should be restricted to the NS for the AutomatedClusterDiscovery.

ACD uses Ownership to handle removal and we can't have cross-NS ownership.

Add doc comments

Add docstring comments

Update capi provider to take namespace from acd and list clusters in that namespace

bigkevmcd · 2023-12-19T09:38:38Z

config/crd/bases/clusters.weave.works_automatedclusterdiscoveries.yaml

@@ -72,6 +75,9 @@ spec:
              name:
                description: Name is the name of the cluster
                type: string
+              namespace:


This can come out.

foot · 2023-12-19T10:22:49Z

api/v1alpha1/automatedclusterdiscovery_types.go

 // AutomatedClusterDiscoverySpec defines the desired state of AutomatedClusterDiscovery
 type AutomatedClusterDiscoverySpec struct {
 	// Name is the name of the cluster
 	Name string `json:"name,omitempty"`

+	// Namespace of the cluster
+	Namespace string `json:"namespace,omitempty"`


can rm this

and use acd.metadata.namespace instead

foot · 2023-12-19T10:36:02Z

api/v1alpha1/automatedclusterdiscovery_types.go

 	AKS *AKS `json:"aks,omitempty"`

+	// CAPI defines the desired state of CAPI
+	CAPI *CAPI `json:"capi,omitempty"`


Lets rm this too -- @bigkevmcd

bigkevmcd

I think there are couple of small things that can be tidied up if you want before merging, but otherwise 👍🏻

We will need to provide a way to exclude clusters in a separate branch.

api/v1alpha1/automatedclusterdiscovery_types.go

bigkevmcd · 2023-12-19T14:50:05Z

internal/controller/automatedclusterdiscovery_controller.go

+	_, err = controllerutil.CreateOrPatch(ctx, r.Client, secret, func() error {
+		if value, err := clientcmd.Write(*cluster.KubeConfig); err != nil {
+			return err
+		} else {


Don't need the else here, as you will have returned in the other branch.

https://go.dev/wiki/CodeReviewComments#indent-error-flow

Co-authored-by: Kevin McDermott <[email protected]>

…ion when creating the secret

…ller into capi

ranatrk added 3 commits December 13, 2023 18:27

Add capi provider to providers

fd935ab

Add capi support in reconcile loop in controller

0884a3f

Add capi crd regenerate cluster discovery crds to include capi

WIP add basic test for capi in automated cluster discpvery controller…

8259ada

… test

ranatrk added the enhancement New feature or request label Dec 13, 2023

ranatrk added 3 commits December 13, 2023 18:38

remove comment

ca4a1ac

Cleanup undeleted secret/gitopscluster from older test in controller

393a1de

Add capi automatedclusterdiscovery to samples

bc53816

Add condition to check for cluster kubeconfig before creating the secret

ranatrk marked this pull request as ready for review December 14, 2023 16:36

ranatrk requested a review from a team December 14, 2023 16:37

Add condition to check for secretref existance when updating secretof…

405d62e

… existing clusters in reconcile loop

bigkevmcd requested changes Dec 18, 2023

View reviewed changes

bigkevmcd reviewed Dec 18, 2023

View reviewed changes

ranatrk added 3 commits December 18, 2023 16:48

Return capi labels in providercluster labels

f479a7e

Add doc comments

Simplify condition checking when publishing event for cluster created

e91d34c

Add docstring comments

Add namespace to automatedclusterdiscoveryspec

eb2bc4c

Update capi provider to take namespace from acd and list clusters in that namespace

ranatrk requested a review from bigkevmcd December 18, 2023 20:06

bigkevmcd requested changes Dec 19, 2023

View reviewed changes

foot reviewed Dec 19, 2023

View reviewed changes

Remove namespace and CAPI spec from automatedclusterdiscoveryspec

73aa132

ranatrk requested review from foot and bigkevmcd December 19, 2023 11:50

bigkevmcd approved these changes Dec 19, 2023

View reviewed changes

ranatrk and others added 3 commits December 19, 2023 16:59

update aks desc comment

5aae441

Co-authored-by: Kevin McDermott <[email protected]>

Update condition statement when publishing event during cluster creat…

48f46c3

…ion when creating the secret

Merge branch 'capi' of github.com:weaveworks/cluster-reflector-contro…

bf04068

…ller into capi

ranatrk merged commit dfb060a into main Dec 19, 2023
3 checks passed

ranatrk deleted the capi branch December 19, 2023 15:40

ranatrk restored the capi branch December 19, 2023 15:45

ranatrk mentioned this pull request Dec 19, 2023

Add capi to supported providers #27

Merged

ranatrk deleted the capi branch December 19, 2023 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capi to supported providers #26

Add capi to supported providers #26

ranatrk commented Dec 13, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 19, 2023

bigkevmcd Dec 18, 2023

ranatrk Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023

bigkevmcd Dec 18, 2023 •

edited

Loading

bigkevmcd Dec 19, 2023

foot Dec 19, 2023

foot Dec 19, 2023

foot Dec 19, 2023

bigkevmcd left a comment

bigkevmcd Dec 19, 2023

		@@ -44,6 +48,8 @@ type AutomatedClusterDiscoverySpec struct {

		AKS *AKS `json:"aks,omitempty"`

		CAPI *CAPI `json:"capi,omitempty"`


		var _ providers.Provider = (*CAPIProvider)(nil)

		func NewCAPIProvider(client client.Client) *CAPIProvider {

Add capi to supported providers #26

Add capi to supported providers #26

Conversation

ranatrk commented Dec 13, 2023

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

bigkevmcd Dec 18, 2023 • 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

bigkevmcd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigkevmcd Dec 18, 2023 •

edited

Loading