diff --git a/api/v1beta3/cloudstackcluster_webhook.go b/api/v1beta3/cloudstackcluster_webhook.go index 1e7a5feb..5c38be19 100644 --- a/api/v1beta3/cloudstackcluster_webhook.go +++ b/api/v1beta3/cloudstackcluster_webhook.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/cluster-api-provider-cloudstack/pkg/webhookutil" + "sigs.k8s.io/cluster-api/util/annotations" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -109,6 +110,13 @@ func (r *CloudStackCluster) ValidateUpdate(old runtime.Object) error { "controlplaneendpoint.port", errorList) } + if annotations.IsExternallyManaged(oldCluster) && !annotations.IsExternallyManaged(r) { + errorList = append(errorList, + field.Forbidden(field.NewPath("metadata", "annotations"), + "removal of externally managed (managed-by) annotation is not allowed"), + ) + } + return webhookutil.AggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, errorList) } diff --git a/api/v1beta3/cloudstackcluster_webhook_test.go b/api/v1beta3/cloudstackcluster_webhook_test.go index 2770edf7..0ec212b5 100644 --- a/api/v1beta3/cloudstackcluster_webhook_test.go +++ b/api/v1beta3/cloudstackcluster_webhook_test.go @@ -23,6 +23,8 @@ import ( . "github.com/onsi/gomega" infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta3" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/util/annotations" ) var _ = Describe("CloudStackCluster webhooks", func() { @@ -81,4 +83,24 @@ var _ = Describe("CloudStackCluster webhooks", func() { Should(MatchError(MatchRegexp(forbiddenRegex, "controlplaneendpoint\\.port"))) }) }) + + Context("When updating a CloudStackCluster's annotations", func() { + It("Should reject removal of externally managed ('managed-by') annotation from CloudStackCluster", func() { + // Create a CloudStackCluster with managed-by annotation + annotations.AddAnnotations(dummies.CSCluster, map[string]string{clusterv1.ManagedByAnnotation: ""}) + Ω(k8sClient.Create(ctx, dummies.CSCluster)).Should(Succeed()) + + // Remove the annotation and update CloudStackCluster + dummies.CSCluster.Annotations = make(map[string]string) + Ω(k8sClient.Update(ctx, dummies.CSCluster)). + Should(MatchError(MatchRegexp(forbiddenRegex, "removal of externally managed"))) + }) + + It("Should allow adding of externally managed ('managed-by') annotation to CloudStackCluster", func() { + Ω(k8sClient.Create(ctx, dummies.CSCluster)).Should(Succeed()) + + annotations.AddAnnotations(dummies.CSCluster, map[string]string{clusterv1.ManagedByAnnotation: ""}) + Ω(k8sClient.Update(ctx, dummies.CSCluster)).Should(Succeed()) + }) + }) }) diff --git a/controllers/cloudstackcluster_controller.go b/controllers/cloudstackcluster_controller.go index a42acfca..315987bd 100644 --- a/controllers/cloudstackcluster_controller.go +++ b/controllers/cloudstackcluster_controller.go @@ -22,11 +22,13 @@ import ( "reflect" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" "github.com/pkg/errors" @@ -34,6 +36,7 @@ import ( csCtrlrUtils "sigs.k8s.io/cluster-api-provider-cloudstack/controllers/utils" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -183,16 +186,35 @@ func (reconciler *CloudStackClusterReconciler) SetupWithManager(ctx context.Cont return !reflect.DeepEqual(oldCluster, newCluster) }, }, - ).Build(reconciler) + ). + WithEventFilter(predicates.ResourceIsNotExternallyManaged(log)). + Build(reconciler) if err != nil { return errors.Wrap(err, "building CloudStackCluster controller") } // Add a watch on CAPI Cluster objects for unpause and ready events. + clusterToInfraFn := util.ClusterToInfrastructureMapFunc(ctx, infrav1.GroupVersion.WithKind("CloudStackCluster"), mgr.GetClient(), &infrav1.CloudStackCluster{}) if err = controller.Watch( &source.Kind{Type: &clusterv1.Cluster{}}, - handler.EnqueueRequestsFromMapFunc( - util.ClusterToInfrastructureMapFunc(ctx, infrav1.GroupVersion.WithKind("CloudStackCluster"), mgr.GetClient(), &infrav1.CloudStackCluster{})), + handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request { + requests := clusterToInfraFn(o) + if requests == nil { + return nil + } + + c := &infrav1.CloudStackCluster{} + if err := reconciler.K8sClient.Get(ctx, requests[0].NamespacedName, c); err != nil { + log.V(4).Error(err, "Failed to get CloudStack cluster") + return nil + } + + if annotations.IsExternallyManaged(c) { + log.V(4).Info("CloudStackCluster is externally managed, skipping mapping") + return nil + } + return requests + }), predicates.ClusterUnpaused(log), ); err != nil { return errors.Wrap(err, "building CloudStackCluster controller") diff --git a/controllers/cloudstackmachine_controller.go b/controllers/cloudstackmachine_controller.go index 0a3483af..0f7846dc 100644 --- a/controllers/cloudstackmachine_controller.go +++ b/controllers/cloudstackmachine_controller.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/predicates" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -126,7 +127,7 @@ func (r *CloudStackMachineReconciliationRunner) Reconcile() (retRes ctrl.Result, r.ConsiderAffinity, r.GetOrCreateVMInstance, r.RequeueIfInstanceNotRunning, - r.AddToLBIfNeeded, + r.RunIf(func() bool { return !annotations.IsExternallyManaged(r.CSCluster) }, r.AddToLBIfNeeded), r.GetOrCreateMachineStateChecker, ) } diff --git a/pkg/cloud/isolated_network.go b/pkg/cloud/isolated_network.go index d1458637..e8386c9d 100644 --- a/pkg/cloud/isolated_network.go +++ b/pkg/cloud/isolated_network.go @@ -24,6 +24,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" + "sigs.k8s.io/cluster-api/util/annotations" ) type IsoNetworkIface interface { @@ -64,7 +65,12 @@ func (c *client) AssociatePublicIPAddress( return errors.Wrapf(err, "fetching a public IP address") } isoNet.Spec.ControlPlaneEndpoint.Host = publicAddress.Ipaddress - csCluster.Spec.ControlPlaneEndpoint.Host = publicAddress.Ipaddress + if !annotations.IsExternallyManaged(csCluster) { + // Do not update the infracluster's controlPlaneEndpoint when the controlplane + // is externally managed, it is the responsibility of the externally managed + // control plane to update this. + csCluster.Spec.ControlPlaneEndpoint.Host = publicAddress.Ipaddress + } isoNet.Status.PublicIPID = publicAddress.Id // Check if the address is already associated with the network. @@ -152,11 +158,13 @@ func (c *client) GetPublicIP( if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) return nil, err - } else if ip != "" && publicAddresses.Count == 1 { // Endpoint specified and IP found. + } else if ip != "" && publicAddresses.Count == 1 { + // Endpoint specified and IP found. // Ignore already allocated here since the IP was specified. return publicAddresses.PublicIpAddresses[0], nil - } else if publicAddresses.Count > 0 { // Endpoint not specified. - for _, v := range publicAddresses.PublicIpAddresses { // Pick first available address. + } else if publicAddresses.Count > 0 { + // Endpoint not specified. Pick first available address. + for _, v := range publicAddresses.PublicIpAddresses { if v.Allocated == "" { // Found un-allocated Public IP. return v, nil } @@ -220,17 +228,6 @@ func (c *client) GetOrCreateLoadBalancerRule( isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster, ) (retErr error) { - // Check/set ports. - // Prefer control plane endpoint. Take iso net port if CP missing. Set to default if both missing. - if csCluster.Spec.ControlPlaneEndpoint.Port != 0 { - isoNet.Spec.ControlPlaneEndpoint.Port = csCluster.Spec.ControlPlaneEndpoint.Port - } else if isoNet.Spec.ControlPlaneEndpoint.Port != 0 { // Override default public port if endpoint port specified. - csCluster.Spec.ControlPlaneEndpoint.Port = isoNet.Spec.ControlPlaneEndpoint.Port - } else { - csCluster.Spec.ControlPlaneEndpoint.Port = 6443 - isoNet.Spec.ControlPlaneEndpoint.Port = 6443 - } - // Check if rule exists. if err := c.ResolveLoadBalancerRuleDetails(fd, isoNet, csCluster); err == nil || !strings.Contains(strings.ToLower(err.Error()), "no load balancer rule found") { @@ -275,14 +272,27 @@ func (c *client) GetOrCreateIsolatedNetwork( return errors.Wrapf(err, "tagging network with id %s", networkID) } - // Associate Public IP with CloudStackIsolatedNetwork - if err := c.AssociatePublicIPAddress(fd, isoNet, csCluster); err != nil { - return errors.Wrapf(err, "associating public IP address to csCluster") - } + if !annotations.IsExternallyManaged(csCluster) { + // Associate Public IP with CloudStackIsolatedNetwork + if err := c.AssociatePublicIPAddress(fd, isoNet, csCluster); err != nil { + return errors.Wrapf(err, "associating public IP address to csCluster") + } + + // Check/set ControlPlaneEndpoint port. + // Prefer csCluster ControlPlaneEndpoint port. Use isonet port if CP missing. Set to default if both missing. + if csCluster.Spec.ControlPlaneEndpoint.Port != 0 { + isoNet.Spec.ControlPlaneEndpoint.Port = csCluster.Spec.ControlPlaneEndpoint.Port + } else if isoNet.Spec.ControlPlaneEndpoint.Port != 0 { // Override default public port if endpoint port specified. + csCluster.Spec.ControlPlaneEndpoint.Port = isoNet.Spec.ControlPlaneEndpoint.Port + } else { + csCluster.Spec.ControlPlaneEndpoint.Port = 6443 + isoNet.Spec.ControlPlaneEndpoint.Port = 6443 + } - // Setup a load balancing rule to map VMs to Public IP. - if err := c.GetOrCreateLoadBalancerRule(fd, isoNet, csCluster); err != nil { - return errors.Wrap(err, "getting or creating load balancing rule") + // Setup a load balancing rule to map VMs to Public IP. + if err := c.GetOrCreateLoadBalancerRule(fd, isoNet, csCluster); err != nil { + return errors.Wrap(err, "getting or creating load balancing rule") + } } // Open the Isolated Network on endopint port.