hammer_and_pick: cleanup from review feedback

Co-authored-by: Vince Prignano <[email protected]>
Signed-off-by: Alexander Eldeib <[email protected]>

cloud/interfaces.go

@@ -36,7 +36,9 @@ type GetterService interface {
 	Delete(ctx context.Context, spec interface{}) error
 }
 
+// CredentialGetter is a GetterService which knows how to retrieve credentials for an Azure
+// resource in a resource group.
 type CredentialGetter interface {
 	GetterService
-	GetCredentials(ctx context.Context, spec interface{}) ([]byte, error)
+	GetCredentials(ctx context.Context, group string, cluster string) ([]byte, error)
 }

cloud/scope/machinepool.go

@@ -86,6 +86,10 @@ func NewMachinePoolScope(params MachinePoolScopeParams) (*MachinePoolScope, erro
 		params.Logger = klogr.New()
 	}
 
+	if err := params.AzureClients.setCredentials(params.AzureCluster.Spec.SubscriptionID); err != nil {
+		return nil, errors.Wrap(err, "failed to create Azure session")
+	}
+
 	helper, err := patch.NewHelper(params.AzureMachinePool, params.Client)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to init patch helper")

cloud/scope/managedcontrolplane.go

@@ -58,8 +58,7 @@ func NewManagedControlPlaneScope(params ManagedControlPlaneScopeParams) (*Manage
 		params.Logger = klogr.New()
 	}
 
-	err := params.AzureClients.setCredentials(params.ControlPlane.Spec.SubscriptionID)
-	if err != nil {
+	if err := params.AzureClients.setCredentials(params.ControlPlane.Spec.SubscriptionID); err != nil {
 		return nil, errors.Wrap(err, "failed to create Azure session")
 	}
 

cloud/services/agentpools/agentpools.go

@@ -65,12 +65,26 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 	}
 
 	existingSpec, err := s.Get(ctx, spec)
+	if err != nil && !azure.ResourceNotFound(err) {
+		return errors.Wrapf(err, "failed to get existing managed cluster")
+	}
 	existingPool, ok := existingSpec.(containerservice.AgentPool)
 	if !ok {
 		return errors.New("expected agent pool specification")
 	}
 
-	if err == nil {
+	// For updates, we want to pass whatever we find in the existing
+	// cluster, normalized to reflect the input we originally provided.
+	// AKS will populate defaults and read-only values, which we want
+	// to strip/clean to match what we expect.
+	isCreate := azure.ResourceNotFound(err)
+	if isCreate {
+		err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile)
+		if err != nil {
+			return fmt.Errorf("failed to create or update agent pool, %#+v", err)
+		}
+	} else {
+		// Normalize individual agent pools to diff in case we need to update
 		existingProfile := containerservice.AgentPool{
 			ManagedClusterAgentPoolProfileProperties: &containerservice.ManagedClusterAgentPoolProfileProperties{
 				VMSize:              existingPool.ManagedClusterAgentPoolProfileProperties.VMSize,
@@ -81,20 +95,16 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 			},
 		}
 
+		// Diff and check if we require an update
 		diff := cmp.Diff(profile, existingProfile)
 		if diff != "" {
-			klog.V(2).Infof("update required (+new -old):\n%s", diff)
+			klog.V(2).Infof("Update required (+new -old):\n%s", diff)
 			err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile)
 			if err != nil {
-				return fmt.Errorf("failed to create or update agent pool, %#+v", err)
+				return fmt.Errorf("failed to create or update agent pool, %#+v", err.Error())
 			}
 		} else {
-			klog.V(2).Infof("normalized and desired managed cluster matched, no update needed")
-		}
-	} else if azure.ResourceNotFound(err) {
-		err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile)
-		if err != nil {
-			return fmt.Errorf("failed to create or update agent pool, %#+v", err)
+			klog.V(2).Infof("Normalized and desired agent pool matched, no update needed")
 		}
 	}
 
@@ -110,14 +120,14 @@ func (s *Service) Delete(ctx context.Context, spec interface{}) error {
 
 	klog.V(2).Infof("deleting agent pool  %s ", agentPoolSpec.Name)
 	err := s.Client.Delete(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name)
-	if err != nil && azure.ResourceNotFound(err) {
-		// already deleted
-		return nil
-	}
 	if err != nil {
+		if azure.ResourceNotFound(err) {
+			// already deleted
+			return nil
+		}
 		return errors.Wrapf(err, "failed to delete agent pool %s in resource group %s", agentPoolSpec.Name, agentPoolSpec.ResourceGroup)
 	}
 
-	klog.V(2).Infof("successfully deleted agent pool %s ", agentPoolSpec.Name)
+	klog.V(2).Infof("Successfully deleted agent pool %s ", agentPoolSpec.Name)
 	return nil
 }

cloud/services/agentpools/client.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2020-02-01/containerservice"
 	"github.com/Azure/go-autorest/autorest"
+	"github.com/pkg/errors"
 	azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
 )
 
@@ -61,11 +62,10 @@ func (ac *AzureClient) Get(ctx context.Context, resourceGroupName, cluster, name
 func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, cluster, name string, properties containerservice.AgentPool) error {
 	future, err := ac.agentpools.CreateOrUpdate(ctx, resourceGroupName, cluster, name, properties)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed to begin operation")
 	}
-	err = future.WaitForCompletionRef(ctx, ac.agentpools.Client)
-	if err != nil {
-		return err
+	if err := future.WaitForCompletionRef(ctx, ac.agentpools.Client); err != nil {
+		return errors.Wrap(err, "failed to end operation")
 	}
 	_, err = future.Result(ac.agentpools)
 	return err
@@ -75,11 +75,10 @@ func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, cl
 func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, cluster, name string) error {
 	future, err := ac.agentpools.Delete(ctx, resourceGroupName, cluster, name)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed to begin operation")
 	}
-	err = future.WaitForCompletionRef(ctx, ac.agentpools.Client)
-	if err != nil {
-		return err
+	if err := future.WaitForCompletionRef(ctx, ac.agentpools.Client); err != nil {
+		return errors.Wrap(err, "failed to end operation")
 	}
 	_, err = future.Result(ac.agentpools)
 	return err

cloud/services/managedclusters/client.go

@@ -42,8 +42,9 @@ var _ Client = &AzureClient{}
 
 // NewClient creates a new VM client from subscription ID.
 func NewClient(subscriptionID string, authorizer autorest.Authorizer) *AzureClient {
-	c := newManagedClustersClient(subscriptionID, authorizer)
-	return &AzureClient{c}
+	return &AzureClient{
+		managedclusters: newManagedClustersClient(subscriptionID, authorizer),
+	}
 }
 
 // newManagedClustersClient creates a new managed clusters client from subscription ID.
@@ -77,11 +78,10 @@ func (ac *AzureClient) GetCredentials(ctx context.Context, resourceGroupName, na
 func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, name string, cluster containerservice.ManagedCluster) error {
 	future, err := ac.managedclusters.CreateOrUpdate(ctx, resourceGroupName, name, cluster)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to begin operation")
 	}
-	err = future.WaitForCompletionRef(ctx, ac.managedclusters.Client)
-	if err != nil {
-		return err
+	if err := future.WaitForCompletionRef(ctx, ac.managedclusters.Client); err != nil {
+		return errors.Wrapf(err, "failed to end operation")
 	}
 	_, err = future.Result(ac.managedclusters)
 	return err
@@ -91,11 +91,10 @@ func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, na
 func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, name string) error {
 	future, err := ac.managedclusters.Delete(ctx, resourceGroupName, name)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to begin operation")
 	}
-	err = future.WaitForCompletionRef(ctx, ac.managedclusters.Client)
-	if err != nil {
-		return err
+	if err := future.WaitForCompletionRef(ctx, ac.managedclusters.Client); err != nil {
+		return errors.Wrapf(err, "failed to end operation")
 	}
 	_, err = future.Result(ac.managedclusters)
 	return err

cloud/services/managedclusters/managedclusters.go

@@ -45,7 +45,6 @@ type Spec struct {
 	Location string
 
 	// Tags is a set of tags to add to this cluster.
-	// +optional
 	Tags map[string]string
 
 	// Version defines the desired Kubernetes version.
@@ -90,12 +89,8 @@ func (s *Service) Get(ctx context.Context, spec interface{}) (interface{}, error
 }
 
 // Get fetches a managed cluster kubeconfig from Azure.
-func (s *Service) GetCredentials(ctx context.Context, spec interface{}) ([]byte, error) {
-	managedClusterSpec, ok := spec.(*Spec)
-	if !ok {
-		return nil, errors.New("expected managed cluster specification")
-	}
-	return s.Client.GetCredentials(ctx, managedClusterSpec.ResourceGroup, managedClusterSpec.Name)
+func (s *Service) GetCredentials(ctx context.Context, group, name string) ([]byte, error) {
+	return s.Client.GetCredentials(ctx, group, name)
 }
 
 // Reconcile idempotently creates or updates a managed cluster, if possible.
@@ -185,7 +180,7 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 
 	err := s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroup, managedClusterSpec.Name, properties)
 	if err != nil {
-		return fmt.Errorf("failed to create or update managed cluster, %v", err)
+		return fmt.Errorf("failed to create or update managed cluster, %#+v", err)
 	}
 
 	return nil
@@ -198,13 +193,13 @@ func (s *Service) Delete(ctx context.Context, spec interface{}) error {
 		return errors.New("expected managed cluster specification")
 	}
 
-	klog.V(2).Infof("deleting managed cluster  %s ", managedClusterSpec.Name)
+	klog.V(2).Infof("Deleting managed cluster  %s ", managedClusterSpec.Name)
 	err := s.Client.Delete(ctx, managedClusterSpec.ResourceGroup, managedClusterSpec.Name)
-	if err != nil && azure.ResourceNotFound(err) {
-		// already deleted
-		return nil
-	}
 	if err != nil {
+		if azure.ResourceNotFound(err) {
+			// already deleted
+			return nil
+		}
 		return errors.Wrapf(err, "failed to delete managed cluster %s in resource group %s", managedClusterSpec.Name, managedClusterSpec.ResourceGroup)
 	}
 

cloud/services/publicips/publicips.go

@@ -29,7 +29,8 @@ import (
 
 // Spec specification for public ip
 type Spec struct {
-	Name string
+	Name    string
+	DNSName string
 }
 
 // Get provides information about a public ip.
@@ -70,7 +71,7 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
 				PublicIPAllocationMethod: network.Static,
 				DNSSettings: &network.PublicIPAddressDNSSettings{
 					DomainNameLabel: to.StringPtr(strings.ToLower(ipName)),
-					Fqdn:            to.StringPtr(s.Scope.Network().APIServerIP.DNSName),
+					Fqdn:            &publicIPSpec.DNSName,
 				},
 			},
 		},

cloud/services/scalesets/vmss.go

@@ -170,11 +170,11 @@ func (s *Service) Delete(ctx context.Context, spec interface{}) error {
 	}
 	klog.V(2).Infof("deleting VMSS %s ", vmssSpec.Name)
 	err := s.Client.Delete(ctx, vmssSpec.ResourceGroup, vmssSpec.Name)
-	if err != nil && azure.ResourceNotFound(err) {
-		// already deleted
-		return nil
-	}
 	if err != nil {
+		if azure.ResourceNotFound(err) {
+			// already deleted
+			return nil
+		}
 		return errors.Wrapf(err, "failed to delete VMSS %s in resource group %s", vmssSpec.Name, vmssSpec.ResourceGroup)
 	}
 

config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremachinepools.yaml

@@ -10,9 +10,13 @@ metadata:
 spec:
   group: exp.infrastructure.cluster.x-k8s.io
   names:
+    categories:
+    - cluster-api
     kind: AzureMachinePool
     listKind: AzureMachinePoolList
     plural: azuremachinepools
+    shortNames:
+    - amp
     singular: azuremachinepool
   scope: Namespaced
   versions:

config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremanagedcontrolplanes.yaml

@@ -127,6 +127,12 @@ spec:
             description: AzureManagedControlPlaneStatus defines the observed state
               of AzureManagedControlPlane
             properties:
+              initialized:
+                description: Initialized is true when the the control plane is available
+                  for initial contact. This may occur before the control plane is
+                  fully ready. In the AzureManagedControlPlane implementation, these
+                  are identical.
+                type: boolean
               ready:
                 description: Ready is true when the provider resource is ready.
                 type: boolean

controllers/azurecluster_reconciler.go

@@ -205,7 +205,8 @@ func (r *azureClusterReconciler) Reconcile() error {
 	}
 
 	publicIPSpec := &publicips.Spec{
-		Name: r.scope.Network().APIServerIP.Name,
+		Name:    r.scope.Network().APIServerIP.Name,
+		DNSName: r.scope.Network().APIServerIP.DNSName,
 	}
 	if err := r.publicIPSvc.Reconcile(r.scope.Context, publicIPSpec); err != nil {
 		return errors.Wrapf(err, "failed to reconcile control plane public ip for cluster %s", r.scope.Name())

exp/api/v1alpha3/azuremachinepool_types.go

@@ -121,6 +121,7 @@ type (
 
 	// +kubebuilder:object:root=true
 	// +kubebuilder:subresource:status
+	// +kubebuilder:resource:path=azuremachinepools,scope=Namespaced,categories=cluster-api,shortName=amp
 	// +kubebuilder:printcolumn:name="Replicas",type="string",JSONPath=".status.replicas",description="AzureMachinePool replicas count"
 	// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.ready",description="AzureMachinePool replicas count"
 	// +kubebuilder:printcolumn:name="State",type="string",JSONPath=".status.provisioningState",description="Azure VMSS provisioning state"

exp/api/v1alpha3/azuremanagedcontrolplane_types.go

@@ -72,6 +72,12 @@ type AzureManagedControlPlaneStatus struct {
 	// Ready is true when the provider resource is ready.
 	// +optional
 	Ready bool `json:"ready,omitempty"`
+
+	// Initialized is true when the the control plane is available for initial contact.
+	// This may occur before the control plane is fully ready.
+	// In the AzureManagedControlPlane implementation, these are identical.
+	// +optional
+	Initialized bool `json:"initialized,omitempty"`
 }
 
 // +kubebuilder:object:root=true

exp/controllers/azuremangedcontrolplane_controller.go

@@ -151,6 +151,7 @@ func (r *AzureManagedControlPlaneReconciler) reconcileNormal(ctx context.Context
 
 	// No errors, so mark us ready so the Cluster API Cluster Controller can pull it
 	scope.ControlPlane.Status.Ready = true
+	scope.ControlPlane.Status.Initialized = true
 
 	return reconcile.Result{}, nil
 }