clean up implementation from review

api/v1alpha3/azurecluster_validation.go

@@ -159,5 +159,8 @@ func validateControlPlaneIP(old, new *PublicIPSpec, fldPath *field.Path) *field.
 	if old != nil && new != nil && old.Name != new.Name {
 		return field.Invalid(fldPath, new, fmt.Sprintf("changing control plane endpoint after cluster creation is not allowed"))
 	}
+	if new != nil && new.Name == "" {
+		return field.Invalid(fldPath, new, fmt.Sprintf("control plane endpoint IP name must be non-empty"))
+	}
 	return nil
 }

api/v1alpha3/azurecluster_webhook.go

@@ -52,6 +52,16 @@ func (c *AzureCluster) ValidateCreate() error {
 	clusterlog.Info("validate create", "name", c.Name)
 	allErrs := c.validateCluster()
 
+	// validation is smart/lazy enough to work with the same object compared to itself
+	// it works well for update to take both objects.
+	if err := validateControlPlaneIP(
+		c.Spec.NetworkSpec.PublicIP,
+		c.Spec.NetworkSpec.PublicIP,
+		field.NewPath("spec").Child("networkSpec").Child("publicIp"),
+	); err != nil {
+		allErrs = append(allErrs, err)
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}

api/v1alpha3/types.go

@@ -153,6 +153,7 @@ type PublicIP struct {
 
 // PublicIPSpec defines the inputs to create an Azure public IP address.
 type PublicIPSpec struct {
+	// +kubebuilder:validation:MinLength=1
 	Name string `json:"name,omitempty"`
 }
 

cloud/scope/cluster.go

@@ -106,33 +106,27 @@ func (s *ClusterScope) Network() *infrav1.Network {
 	return &s.AzureCluster.Status.Network
 }
 
-// PublicIPSpec returns the public IP specs.
-func (s *ClusterScope) PublicIPSpecs() ([]azure.PublicIPSpec, error) {
-	spec := []azure.PublicIPSpec{
-		{
-			Name: azure.GenerateNodeOutboundIPName(s.ClusterName()),
-		},
-	}
-
-	// If IP provided by name, expect to exist and don't precreate.
+// PublicIPSpecs returns the public IP specs.
+func (s *ClusterScope) PublicIPSpecs() []azure.PublicIPSpec {
+	// If user provides an API server IP, do not return it return it here.
+	// We expect it to be created by them and will not reconcile it.
 	if s.AzureCluster.Spec.NetworkSpec.PublicIP != nil && s.AzureCluster.Spec.NetworkSpec.PublicIP.Name != "" {
-		return spec, nil
-	}
-
-	if s.Network().APIServerIP.Name == "" {
-		h := fnv.New32a()
-		if _, err := h.Write([]byte(fmt.Sprintf("%s/%s/%s", s.SubscriptionID(), s.ResourceGroup(), s.ClusterName()))); err != nil {
-			return nil, errors.Wrapf(err, "failed to write hash sum for api server ip")
+		return []azure.PublicIPSpec{
+			{
+				Name: azure.GenerateNodeOutboundIPName(s.ClusterName()),
+			},
 		}
-		s.Network().APIServerIP.Name = azure.GeneratePublicIPName(s.ClusterName(), fmt.Sprintf("%x", h.Sum32()))
 	}
 
-	spec = append(spec, azure.PublicIPSpec{
-		Name:    s.Network().APIServerIP.Name,
-		DNSName: s.Network().APIServerIP.DNSName,
-	})
-
-	return spec, nil
+	return []azure.PublicIPSpec{
+		{
+			Name: azure.GenerateNodeOutboundIPName(s.ClusterName()),
+		},
+		{
+			Name:    s.Network().APIServerIP.Name,
+			DNSName: s.Network().APIServerIP.DNSName,
+		},
+	}
 }
 
 // Vnet returns the cluster Vnet.
@@ -221,3 +215,22 @@ func (s *ClusterScope) SetFailureDomain(id string, spec clusterv1.FailureDomainS
 	}
 	s.AzureCluster.Status.FailureDomains[id] = spec
 }
+
+// SetAPIServerIP will set the spec for a for a given key
+func (s *ClusterScope) SetAPIServerIP() error {
+	// If IP provided by name, expect to exist and don't precreate.
+	if s.AzureCluster.Spec.NetworkSpec.PublicIP != nil {
+		s.AzureCluster.Status.Network.APIServerIP.Name = s.AzureCluster.Spec.NetworkSpec.PublicIP.Name
+	} else if s.Network().APIServerIP.Name == "" {
+		// otherwise, generate
+		h := fnv.New32a()
+		if _, err := h.Write([]byte(fmt.Sprintf("%s/%s/%s", s.SubscriptionID(), s.ResourceGroup(), s.ClusterName()))); err != nil {
+			return errors.Wrapf(err, "failed to write hash sum for api server ip")
+		}
+		s.AzureCluster.Status.Network.APIServerIP.Name = azure.GeneratePublicIPName(s.ClusterName(), fmt.Sprintf("%x", h.Sum32()))
+	}
+
+	s.AzureCluster.Status.Network.APIServerIP.DNSName = s.GenerateFQDN()
+
+	return nil
+}

cloud/scope/machine.go

@@ -87,15 +87,15 @@ type MachineScope struct {
 }
 
 // PublicIPSpec returns the public IP specs.
-func (m *MachineScope) PublicIPSpecs() ([]azure.PublicIPSpec, error) {
-	var spec []azure.PublicIPSpec
-	if m.AzureMachine.Spec.AllocatePublicIP == true {
-		nicName := azure.GenerateNICName(m.Name())
-		spec = append(spec, azure.PublicIPSpec{
-			Name: azure.GenerateNodePublicIPName(nicName),
-		})
+func (m *MachineScope) PublicIPSpecs() []azure.PublicIPSpec {
+	if m.AzureMachine.Spec.AllocatePublicIP {
+		return []azure.PublicIPSpec{
+			{
+				Name: azure.GenerateNodePublicIPName(azure.GenerateNICName(m.Name())),
+			},
+		}
 	}
-	return spec, nil
+	return nil
 }
 
 // NICSpecs returns the network interface specs.

cloud/services/publicips/publicips.go

@@ -30,12 +30,7 @@ import (
 
 // Reconcile gets/creates/updates a public ip.
 func (s *Service) Reconcile(ctx context.Context) error {
-	publicIPSpecs, err := s.Scope.PublicIPSpecs()
-	if err != nil {
-		return errors.Wrap(err, "failed to extract public IP specs")
-	}
-
-	for _, publicIPSpec := range publicIPSpecs {
+	for _, publicIPSpec := range s.Scope.PublicIPSpecs() {
 		s.Scope.V(2).Info("creating public IP", "public-ip", publicIPSpec.Name)
 
 		ipSpec := network.PublicIPAddress{
@@ -76,12 +71,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 
 // Delete deletes the public IP with the provided scope.
 func (s *Service) Delete(ctx context.Context) error {
-	publicIPSpecs, err := s.Scope.PublicIPSpecs()
-	if err != nil {
-		return errors.Wrap(err, "failed to extract public IP specs")
-	}
-
-	for _, publicIPSpec := range publicIPSpecs {
+	for _, publicIPSpec := range s.Scope.PublicIPSpecs() {
 		s.Scope.V(2).Info("deleting public IP", "public ip", publicIPSpec.Name)
 
 		err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), publicIPSpec.Name)

cloud/services/publicips/publicips_test.go

@@ -63,7 +63,7 @@ func TestReconcilePublicIP(t *testing.T) {
 					{
 						Name: "my-publicip-3",
 					},
-				}, nil)
+				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("testlocation")
 				s.ClusterName().AnyTimes().Return("foo")
@@ -82,7 +82,7 @@ func TestReconcilePublicIP(t *testing.T) {
 						Name:    "my-publicip",
 						DNSName: "fakedns",
 					},
-				}, nil)
+				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("testlocation")
 				s.ClusterName().Return("foo")
@@ -138,7 +138,7 @@ func TestDeletePublicIP(t *testing.T) {
 					{
 						Name: "my-publicip-2",
 					},
-				}, nil)
+				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 
 				m.Delete(context.TODO(), "my-rg", "my-publicip").Return(nil)
@@ -155,7 +155,7 @@ func TestDeletePublicIP(t *testing.T) {
 					{
 						Name: "my-publicip",
 					},
-				}, nil)
+				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				m.Delete(context.TODO(), "my-rg", "my-publicip").
 					Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
@@ -170,7 +170,7 @@ func TestDeletePublicIP(t *testing.T) {
 					{
 						Name: "my-publicip",
 					},
-				}, nil)
+				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				m.Delete(context.TODO(), "my-rg", "my-publicip").
 					Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))

cloud/services/publicips/service.go

@@ -26,7 +26,7 @@ import (
 type PublicIPScope interface {
 	logr.Logger
 	azure.ClusterDescriber
-	PublicIPSpecs() ([]azure.PublicIPSpec, error)
+	PublicIPSpecs() []azure.PublicIPSpec
 }
 
 // Service provides operations on Azure resources.

config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml

@@ -471,6 +471,7 @@ spec:
                       generated.
                     properties:
                       name:
+                        minLength: 1
                         type: string
                     type: object
                   subnets:

controllers/azurecluster_reconciler.go

@@ -18,8 +18,6 @@ package controllers
 
 import (
 	"context"
-	"fmt"
-	"hash/fnv"
 	"strconv"
 
 	"github.com/Azure/go-autorest/autorest/to"
@@ -76,6 +74,10 @@ func newAzureClusterReconciler(scope *scope.ClusterScope) *azureClusterReconcile
 func (r *azureClusterReconciler) Reconcile(ctx context.Context) error {
 	klog.V(2).Infof("reconciling cluster %s", r.scope.ClusterName())
 
+	if err := r.scope.SetAPIServerIP(); err != nil {
+		return errors.Wrap(err, "failed to set api server IP")
+	}
+
 	if err := r.setFailureDomainsForLocation(ctx); err != nil {
 		return errors.Wrapf(err, "failed to get availability zones for cluster %s in location %s", r.scope.ClusterName(), r.scope.Location())
 	}
@@ -295,23 +297,6 @@ func (r *azureClusterReconciler) deleteNSG(ctx context.Context) error {
 	return nil
 }
 
-// CreateOrUpdateNetworkAPIServerIP creates or updates public ip name and dns name
-func (r *azureClusterReconciler) createOrUpdateNetworkAPIServerIP() error {
-	// Take user provided IP address, or generate if no name exists.
-	if r.scope.AzureCluster.Spec.NetworkSpec.PublicIP != nil && r.scope.AzureCluster.Spec.NetworkSpec.PublicIP.Name != "" {
-		r.scope.Network().APIServerIP.Name = r.scope.AzureCluster.Spec.NetworkSpec.PublicIP.Name
-	} else if r.scope.Network().APIServerIP.Name == "" {
-		h := fnv.New32a()
-		if _, err := h.Write([]byte(fmt.Sprintf("%s/%s/%s", r.scope.SubscriptionID(), r.scope.ResourceGroup(), r.scope.ClusterName()))); err != nil {
-			return errors.Wrapf(err, "failed to write hash sum for api server ip")
-		}
-		r.scope.Network().APIServerIP.Name = azure.GeneratePublicIPName(r.scope.ClusterName(), fmt.Sprintf("%x", h.Sum32()))
-	}
-
-	r.scope.Network().APIServerIP.DNSName = r.scope.GenerateFQDN()
-	return nil
-}
-
 func (r *azureClusterReconciler) setFailureDomainsForLocation(ctx context.Context) error {
 	spec := &availabilityzones.Spec{}
 	zonesInterface, err := r.availabilityZonesSvc.Get(ctx, spec)