Update Kubernetes Cluster controller and add support for meta.externalName #43
There are no files selected for viewing
| +12 −5 | makelib/common.mk | |
| +35 −0 | makelib/controlplane.mk | |
| +17 −2 | makelib/docs.mk | |
| +1 −7 | makelib/golang.mk | |
| +14 −1 | makelib/k8s_tools.mk | |
| +45 −0 | makelib/local.xpkg.mk |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,16 @@ package civokubernetes | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sort" | ||
| "strings" | ||
|
|
||
| "github.com/civo/civogo" | ||
| "github.com/crossplane-contrib/provider-civo/apis/civo/cluster/v1alpha1" | ||
| v1alpha1provider "github.com/crossplane-contrib/provider-civo/apis/civo/provider/v1alpha1" | ||
| xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1" | ||
| "github.com/crossplane/crossplane-runtime/pkg/event" | ||
| "github.com/crossplane/crossplane-runtime/pkg/logging" | ||
| "github.com/crossplane/crossplane-runtime/pkg/meta" | ||
| "github.com/crossplane/crossplane-runtime/pkg/ratelimiter" | ||
| "github.com/crossplane/crossplane-runtime/pkg/reconciler/managed" | ||
| "github.com/crossplane/crossplane-runtime/pkg/resource" | ||
|
|
@@ -38,7 +39,7 @@ type connecter struct { | |
|
|
||
| type external struct { | ||
| kube client.Client | ||
| civoClient *civogo.Client | ||
| } | ||
|
|
||
| // Setup sets up a Civo Kubernetes controller. | ||
|
|
@@ -84,7 +85,7 @@ func (c *connecter) Connect(ctx context.Context, mg resource.Managed) (managed.E | |
| return nil, errors.New("could not find secret") | ||
| } | ||
|
|
||
| civoClient, err := civogo.NewClient(string(s.Data["credentials"]), providerConfig.Spec.Region) | ||
|
|
||
|
Comment on lines
-87
to
89
There was a problem hiding this comment. why removing the There was a problem hiding this comment.
|
||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -95,15 +96,15 @@ func (c *connecter) Connect(ctx context.Context, mg resource.Managed) (managed.E | |
| }, nil | ||
| } | ||
|
|
||
| // nolint | ||
| func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.ExternalObservation, error) { | ||
| cr, ok := mg.(*v1alpha1.CivoKubernetes) | ||
| if !ok { | ||
| return managed.ExternalObservation{}, errors.New("invalid object") | ||
| } | ||
| civoCluster, err := e.civoClient.GetKubernetesCluster(meta.GetExternalName(cr.GetObjectMeta())) | ||
|
Comment on lines
-104
to
+105
There was a problem hiding this comment. Not sure what |
||
| if err != nil { | ||
| return managed.ExternalObservation{}, nil | ||
|
Comment on lines
-106
to
+107
There was a problem hiding this comment. if |
||
| } | ||
| if civoCluster == nil { | ||
| return managed.ExternalObservation{ResourceExists: false}, nil | ||
|
|
@@ -142,18 +143,23 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex | |
| return managed.ExternalObservation{ResourceExists: true}, err | ||
| } | ||
| } | ||
| // UPDATE CHECK -------------------------------------------- | ||
| cr.SetConditions(xpv1.Available()) | ||
| upToDate, _ := e.ResourceIsUpToDate(ctx, cr, civoCluster) | ||
|
|
||
| if upToDate { | ||
| cr.Status.Message = "Cluster is up to date" | ||
| } else { | ||
| cr.Status.Message = "Cluster is being updated" | ||
| } | ||
|
|
||
|
Comment on lines
-145
to
+155
There was a problem hiding this comment. not sure about the current logic and why we try to update the cluster in the Observe @RealHarshThakur any thoughts about it? At the meantime, you @vladfr removed this logic completely. Not sure what is a more correct approach here. |
||
| return managed.ExternalObservation{ | ||
| ResourceExists: true, | ||
| ResourceUpToDate: upToDate, | ||
| ConnectionDetails: cd, | ||
| }, nil | ||
| // -------------------------------------------- | ||
|
|
||
| case "BUILDING": | ||
| cr.Status.Message = "Cluster is being created" | ||
| cr.SetConditions(xpv1.Creating()) | ||
|
|
@@ -170,33 +176,102 @@ func (e *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext | |
| if !ok { | ||
| return managed.ExternalCreation{}, errors.New("invalid object") | ||
| } | ||
|
|
||
| // at the first call, this id will be the cluster name; civo should return 404 | ||
| existingClusterID := meta.GetExternalName(cr.GetObjectMeta()) | ||
| civoCluster, err := e.civoClient.GetKubernetesCluster(existingClusterID) | ||
|
Comment on lines
-173
to
+182
There was a problem hiding this comment. Same as above: not sure what meta.GetExternalName would return for a crossplane resource: it should fetch the annotation crossplane.io/external-name but this doesn't correspond to the clusterID as it's requested by the GetKubernetesCluster method (that now, as I see, it's never used). |
||
| if err != nil { | ||
| if civogo.DatabaseKubernetesClusterNotFoundError.Is(err) { | ||
| // 404 cluster not found, we continue with the create | ||
| } else { | ||
| // cluster lookup error, return | ||
| return managed.ExternalCreation{}, err | ||
| } | ||
| } | ||
| if civoCluster != nil { | ||
| return managed.ExternalCreation{}, nil | ||
| } | ||
|
|
||
| clusterRegion := cr.Spec.Region | ||
| if clusterRegion == "" { | ||
| clusterRegion = e.civoClient.Region | ||
| } | ||
|
|
||
|
Comment on lines
+194
to
+199
There was a problem hiding this comment. we don't use region in the spec, it's implicit in the provider configs |
||
| // Create or Update | ||
| kc := &civogo.KubernetesClusterConfig{ | ||
| Name: cr.Spec.Name, | ||
| Region: clusterRegion, | ||
| NetworkID: *cr.Spec.NetworkID, | ||
| Pools: cr.Spec.Pools, | ||
| Applications: strings.Join(cr.Spec.Applications, ","), | ||
| Tags: strings.Join(cr.Spec.Tags, " "), | ||
| } | ||
|
|
||
| if cr.Spec.CNIPlugin != nil { | ||
| kc.CNIPlugin = *cr.Spec.CNIPlugin | ||
| } | ||
| if cr.Spec.Version != nil { | ||
| kc.KubernetesVersion = *cr.Spec.Version | ||
| } | ||
|
|
||
| if cr.Spec.FirewallID != nil { | ||
| kc.InstanceFirewall = *cr.Spec.FirewallID | ||
| } | ||
|
|
||
| newCluster, err := e.civoClient.NewKubernetesClusters(kc) | ||
|
Comment on lines
-181
to
+221
There was a problem hiding this comment. I would readd the wrapper function and move the creation of the createCluster request in the originary class. Besides that, if you added optional |
||
| if err != nil { | ||
| log.Warn("Cluster creation failed", err) | ||
| return managed.ExternalCreation{}, err | ||
| } | ||
|
|
||
| meta.SetExternalName(cr, newCluster.ID) | ||
|
|
||
|
Comment on lines
+227
to
+228
There was a problem hiding this comment. this is interesting, effectively then you can use the function you proposed above to directly fetch the cluster by ID :claps There was a problem hiding this comment. Yes, Civo API uses cloud-generated IDs for resource identification, so this maps to using Your comment above about |
||
| cr.SetConditions(xpv1.Creating()) | ||
|
|
||
| return managed.ExternalCreation{ | ||
| ExternalNameAssigned: true, | ||
| }, nil | ||
| } | ||
|
|
||
| func (e *external) ResourceIsUpToDate(ctx context.Context, mg resource.Managed, remote *civogo.KubernetesCluster) (bool, error) { | ||
| desired, ok := mg.(*v1alpha1.CivoKubernetes) | ||
| if !ok { | ||
| return false, errors.New("invalid object") | ||
| } | ||
|
|
||
| if len(desired.Spec.Pools) != len(remote.Pools) || !arePoolsEqual(desired, remote) { | ||
| return false, nil | ||
| } | ||
|
|
||
| if desired.Spec.Version != nil && *desired.Spec.Version > remote.Version { | ||
| return false, nil | ||
| } | ||
|
|
||
| if stringSlicesNeedUpdate(desired.Spec.Tags, remote.Tags) { | ||
| return false, nil | ||
| } | ||
|
|
||
| // nolint | ||
| var remoteAppNames []string | ||
| for _, app := range remote.InstalledApplications { | ||
| remoteAppNames = append(remoteAppNames, app.Name) | ||
| } | ||
|
|
||
| if stringSlicesNeedUpdate(desired.Spec.Applications, remoteAppNames) { | ||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| // nolint | ||
| func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.ExternalUpdate, error) { | ||
| desiredCivoCluster, ok := mg.(*v1alpha1.CivoKubernetes) | ||
| if !ok { | ||
| return managed.ExternalUpdate{}, errors.New("invalid object") | ||
| } | ||
| desiredClusterID := meta.GetExternalName(desiredCivoCluster.GetObjectMeta()) | ||
| remoteCivoCluster, err := e.civoClient.GetKubernetesCluster(desiredClusterID) | ||
| if err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
|
|
@@ -215,30 +290,95 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext | |
| if len(desiredCivoCluster.Spec.Pools) != len(remoteCivoCluster.Pools) || !arePoolsEqual(desiredCivoCluster, remoteCivoCluster) { | ||
|
|
||
| log.Debug("Pools are not equal") | ||
| desiredClusterConfig := &civogo.KubernetesClusterConfig{ | ||
| Pools: desiredCivoCluster.Spec.Pools, | ||
| Region: desiredCivoCluster.Spec.Region, | ||
|
There was a problem hiding this comment. @RealHarshThakur thoughts if we should introduce There was a problem hiding this comment. Region is required by the API, so this is why it needs to be passed here. I think the CRD should have it too, it would be good to follow the API as close as possible. And right now, it still picks up the default Region from the ProviderConfig. Another argument in favour of the region is that if you want to deploy in multiple regions, you can use the same ProviderConfig. |
||
| } | ||
| if _, err := e.civoClient.UpdateKubernetesCluster(desiredClusterID, desiredClusterConfig); err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
| } | ||
|
|
||
| if desiredCivoCluster.Spec.Version != nil { | ||
| if *desiredCivoCluster.Spec.Version > remoteCivoCluster.Version { | ||
| log.Info("Updating cluster version") | ||
| desiredClusterConfig := &civogo.KubernetesClusterConfig{ | ||
| Name: desiredCivoCluster.Name, | ||
| KubernetesVersion: *desiredCivoCluster.Spec.Version, | ||
| Region: desiredCivoCluster.Spec.Region, | ||
| } | ||
| if _, err := e.civoClient.UpdateKubernetesCluster(desiredClusterID, desiredClusterConfig); err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if desiredCivoCluster.Spec.FirewallID != nil && desiredCivoCluster.Spec.FirewallID != &remoteCivoCluster.FirewallID { | ||
| desiredClusterConfig := &civogo.KubernetesClusterConfig{ | ||
| Name: desiredCivoCluster.Name, | ||
| InstanceFirewall: *desiredCivoCluster.Spec.FirewallID, | ||
| Region: desiredCivoCluster.Spec.Region, | ||
| } | ||
| if _, err := e.civoClient.UpdateKubernetesCluster(desiredClusterID, desiredClusterConfig); err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
| } | ||
|
|
||
| if stringSlicesNeedUpdate(desiredCivoCluster.Spec.Tags, remoteCivoCluster.Tags) { | ||
| desiredClusterConfig := &civogo.KubernetesClusterConfig{ | ||
| Name: desiredCivoCluster.Name, | ||
| Tags: strings.Join(desiredCivoCluster.Spec.Tags, " "), | ||
| Region: desiredCivoCluster.Spec.Region, | ||
| } | ||
| if _, err := e.civoClient.UpdateKubernetesCluster(desiredClusterID, desiredClusterConfig); err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
| } | ||
|
|
||
| // nolint | ||
| var remoteAppNames []string | ||
| for _, app := range remoteCivoCluster.InstalledApplications { | ||
| remoteAppNames = append(remoteAppNames, app.Name) | ||
| } | ||
|
|
||
| if stringSlicesNeedUpdate(desiredCivoCluster.Spec.Applications, remoteAppNames) { | ||
| desiredClusterConfig := &civogo.KubernetesClusterConfig{ | ||
| Name: desiredCivoCluster.Name, | ||
| Applications: strings.Join(desiredCivoCluster.Spec.Applications, " "), | ||
| Region: desiredCivoCluster.Spec.Region, | ||
| } | ||
| if _, err := e.civoClient.UpdateKubernetesCluster(desiredClusterID, desiredClusterConfig); err != nil { | ||
| return managed.ExternalUpdate{}, err | ||
| } | ||
| } | ||
|
|
||
| return managed.ExternalUpdate{}, nil | ||
| } | ||
|
|
||
| func stringSlicesNeedUpdate(desired, remote []string) bool { | ||
| if len(desired) != len(remote) { | ||
| return true | ||
| } else if len(desired) == 0 { | ||
| return false | ||
| } | ||
|
|
||
| sort.Strings(desired) | ||
| sort.Strings(remote) | ||
|
|
||
| for i := range desired { | ||
| if desired[i] != remote[i] { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func (e *external) Delete(ctx context.Context, mg resource.Managed) error { | ||
| cr, ok := mg.(*v1alpha1.CivoKubernetes) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| civoCluster, err := e.civoClient.GetKubernetesCluster(meta.GetExternalName(cr.GetObjectMeta())) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -262,7 +402,8 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) error { | |
| // ------------------------------------------------ | ||
| cr.Status.Message = deletionMessage | ||
| cr.SetConditions(xpv1.Deleting()) | ||
| _, err = e.civoClient.DeleteKubernetesCluster(civoCluster.ID) | ||
| return err | ||
| } | ||
|
|
||
| func arePoolsEqual(desiredCivoCluster *v1alpha1.CivoKubernetes, remoteCivoCluster *civogo.KubernetesCluster) bool { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The destination region is set in the ProviderConfig, so we're not setting in the spec of the resource, so I would keep as it is