Iterate on changes

Implement some of Anubhuti's feedback.
  - remove unused clients
  - renamed UberClient interface -> ACSEngineClient
  - removed the unused AzureTemplateDeployer as part of the interfaces
  cleanup

Cleanup the usage of interfaces. More closely follows the pattern I'd
been trying to follow. The AzureClient winds up implementing the
ACSEngine interface, as well as the interfaces inside ACSEngine.

Also fixed some warnings here and there in the new code I've been adding

cmd/generate.go

@@ -31,7 +31,7 @@ type generateCmd struct {
 	apiVersion       string
 }
 
-func NewGenerateCmd() *cobra.Command {
+func newGenerateCmd() *cobra.Command {
 	gc := generateCmd{}
 
 	generateCmd := &cobra.Command{

cmd/root.go

@@ -17,6 +17,7 @@ var (
 	debug bool
 )
 
+// NewRootCmd returns the root command for ACS-Engine.
 func NewRootCmd() *cobra.Command {
 	rootCmd := &cobra.Command{
 		Use:   rootName,
@@ -32,11 +33,11 @@ func NewRootCmd() *cobra.Command {
 	p := rootCmd.PersistentFlags()
 	p.BoolVar(&debug, "debug", false, "enable verbose debug logs")
 
-	rootCmd.AddCommand(NewVersionCmd())
-	rootCmd.AddCommand(NewGenerateCmd())
+	rootCmd.AddCommand(newVersionCmd())
+	rootCmd.AddCommand(newGenerateCmd())
 
 	if val := os.Getenv("ACSENGINE_EXPERIMENTAL_FEATURES"); val == "1" {
-		rootCmd.AddCommand(NewUpgradeCmd())
+		rootCmd.AddCommand(newUpgradeCmd())
 	}
 
 	return rootCmd

cmd/upgrade.go

@@ -45,11 +45,11 @@ type upgradeCmd struct {
 
 	upgradeContainerService *api.UpgradeContainerService
 	upgradeAPIVersion       string
-	client                  armhelpers.UberClient
+	client                  armhelpers.ACSEngineClient
 }
 
 // NewUpgradeCmd run a command to upgrade a Kubernetes cluster
-func NewUpgradeCmd() *cobra.Command {
+func newUpgradeCmd() *cobra.Command {
 	uc := upgradeCmd{}
 
 	upgradeCmd := &cobra.Command{

cmd/version.go

@@ -6,11 +6,14 @@ import (
 )
 
 var (
-	BuildSHA  = "unset"
+	// BuildSHA holds the git commit SHA at `make build` time.
+	BuildSHA = "unset"
+
+	// BuildTime holds the `date` at `make build` time.
 	BuildTime = "unset"
 )
 
-func NewVersionCmd() *cobra.Command {
+func newVersionCmd() *cobra.Command {
 	versionCmd := &cobra.Command{
 		Use:   "version",
 		Short: "Print the version of ACS-Engine",

main.go

@@ -5,10 +5,6 @@ import (
 	log "github.com/Sirupsen/logrus"
 )
 
-const (
-	ClientID = "76e0feec-6b7f-41f0-81a7-b1b944520261"
-)
-
 func main() {
 	if err := cmd.NewRootCmd().Execute(); err != nil {
 		log.Fatalln(err)

pkg/armhelpers/azureclient.go

@@ -10,9 +10,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/Azure/acs-engine/pkg/acsengine"
-
-	"github.com/Azure/azure-sdk-for-go/arm/authorization"
 	"github.com/Azure/azure-sdk-for-go/arm/compute"
 	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
 	"github.com/Azure/azure-sdk-for-go/arm/resources/subscriptions"
@@ -22,6 +19,8 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	log "github.com/Sirupsen/logrus"
 	"github.com/mitchellh/go-homedir"
+
+	"github.com/Azure/acs-engine/pkg/acsengine"
 )
 
 const (
@@ -37,27 +36,24 @@ var (
 	RequiredResourceProviders = []string{"Microsoft.Compute", "Microsoft.Storage", "Microsoft.Network"}
 )
 
-// AzureClient is the uber client
-// If done right, we really shouldn't need SubscriptionID or TenantID in the client anywhere else
-// they're only used for setting up the clients, we can add them back later if needed
+// AzureClient implements the `ACSEngineClient` interface.
+// This client is backed by real Azure clients talking to an ARM endpoint.
 type AzureClient struct {
-	DeploymentsClient     resources.DeploymentsClient
-	GroupsClient          resources.GroupsClient
-	RoleAssignmentsClient authorization.RoleAssignmentsClient
-	ResourcesClient       resources.GroupClient
-	ProvidersClient       resources.ProvidersClient
-	SubscriptionsClient   subscriptions.GroupClient
-	VirtualMachinesClient compute.VirtualMachinesClient
+	deploymentsClient     resources.DeploymentsClient
+	resourcesClient       resources.GroupClient
+	providersClient       resources.ProvidersClient
+	subscriptionsClient   subscriptions.GroupClient
+	virtualMachinesClient compute.VirtualMachinesClient
 }
 
-// now we can just return the full azure sdk client as the tempalte deployer, so TD can be mocked
-func (ac *AzureClient) TemplateDeployer() TemplateDeployer {
-	return ac.DeploymentsClient
+// DeploymentsClient returns an implementation of the `DeploymentsClient` interface
+func (az *AzureClient) DeploymentsClient() DeploymentsClient {
+	return az
 }
 
-// now we can just return the full azure sdk client as the tempalte deployer, so TD can be mocked
-func (ac *AzureClient) VMClient() VMClient {
-	return ac.VirtualMachinesClient
+// VirtualMachinesClient returns an implementation of the `VirtualMachinesClient` interface
+func (az *AzureClient) VirtualMachinesClient() VirtualMachinesClient {
+	return az
 }
 
 // NewAzureClientWithDeviceAuth returns an AzureClient by having a user complete a device authentication flow
@@ -229,21 +225,17 @@ func getOAuthConfig(env azure.Environment, subscriptionID string) (*adal.OAuthCo
 
 func getClient(env azure.Environment, subscriptionID string, armSpt *adal.ServicePrincipalToken, adSpt *adal.ServicePrincipalToken) (*AzureClient, error) {
 	c := &AzureClient{
-		DeploymentsClient:     resources.NewDeploymentsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		GroupsClient:          resources.NewGroupsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		RoleAssignmentsClient: authorization.NewRoleAssignmentsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		ResourcesClient:       resources.NewGroupClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		ProvidersClient:       resources.NewProvidersClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		VirtualMachinesClient: compute.NewVirtualMachinesClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		deploymentsClient:     resources.NewDeploymentsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		resourcesClient:       resources.NewGroupClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		providersClient:       resources.NewProvidersClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		virtualMachinesClient: compute.NewVirtualMachinesClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
 	}
 
 	authorizer := autorest.NewBearerAuthorizer(armSpt)
-	c.DeploymentsClient.Authorizer = authorizer
-	c.GroupsClient.Authorizer = authorizer
-	c.RoleAssignmentsClient.Authorizer = authorizer
-	c.ResourcesClient.Authorizer = authorizer
-	c.ProvidersClient.Authorizer = authorizer
-	c.VirtualMachinesClient.Authorizer = authorizer
+	c.deploymentsClient.Authorizer = authorizer
+	c.resourcesClient.Authorizer = authorizer
+	c.providersClient.Authorizer = authorizer
+	c.virtualMachinesClient.Authorizer = authorizer
 
 	err := c.ensureProvidersRegistered(subscriptionID)
 	if err != nil {
@@ -253,8 +245,8 @@ func getClient(env azure.Environment, subscriptionID string, armSpt *adal.Servic
 	return c, nil
 }
 
-func (azureClient *AzureClient) ensureProvidersRegistered(subscriptionID string) error {
-	registeredProviders, err := azureClient.ProvidersClient.List(to.Int32Ptr(100), "")
+func (az *AzureClient) ensureProvidersRegistered(subscriptionID string) error {
+	registeredProviders, err := az.providersClient.List(to.Int32Ptr(100), "")
 	if err != nil {
 		return err
 	}
@@ -276,7 +268,7 @@ func (azureClient *AzureClient) ensureProvidersRegistered(subscriptionID string)
 			log.Debugf("Already registered for %q", provider)
 		} else {
 			log.Info("Registering subscription to resource provider. provider=%q subscription=%q", provider, subscriptionID)
-			if _, err := azureClient.ProvidersClient.Register(provider); err != nil {
+			if _, err := az.providersClient.Register(provider); err != nil {
 				return err
 			}
 		}

pkg/armhelpers/deployer.go

@@ -1,59 +0,0 @@
-package armhelpers
-
-import (
-	"fmt"
-	"time"
-
-	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
-	"github.com/Azure/go-autorest/autorest"
-	"github.com/prometheus/common/log"
-)
-
-type AzureTemplateDeployer struct{}
-
-func (az *AzureClient) Validate(resourceGroup, name string, d resources.Deployment) (resources.DeploymentValidateResult, error) {
-	return az.DeploymentsClient.Validate(resourceGroup, name, d)
-}
-
-func (az *AzureClient) CreateOrUpdate(resourceGroup, name string, d resources.Deployment, cancel chan struct{}) (<-chan resources.DeploymentExtended, <-chan error) {
-	return az.DeploymentsClient.CreateOrUpdate(resourceGroup, name, d, cancel)
-}
-
-func (az *AzureClient) Delete(resourceGroup, name string, cancel chan struct{}) (<-chan autorest.Response, <-chan error) {
-	return az.DeploymentsClient.Delete(resourceGroup, name, cancel)
-}
-
-func (az *AzureClient) Get(resourceGroup, name string) (resources.DeploymentExtended, error) {
-	return az.DeploymentsClient.Get(resourceGroup, name)
-}
-
-func DeployTemplate(deployer TemplateDeployer, resourceGroupName, deploymentName string, template map[string]interface{}, parameters map[string]interface{}) (response *resources.DeploymentExtended, err error) {
-	// this is needed because either ARM or the SDK can't distinguish between past
-	// deployments and current deployments with the same deploymentName.
-	uniqueSuffix := fmt.Sprintf("-%d", time.Now().Unix())
-	deploymentName = deploymentName + uniqueSuffix
-
-	deployment := resources.Deployment{
-		Properties: &resources.DeploymentProperties{
-			Template:   &template,
-			Parameters: &parameters,
-			Mode:       resources.Incremental,
-		},
-	}
-
-	log.Infof("Starting ARM Deployment. This will take some time. deployment=%q", deploymentName)
-
-	resChan, errChan := deployer.CreateOrUpdate(
-		resourceGroupName,
-		deploymentName,
-		deployment,
-		nil)
-	if err := <-errChan; err != nil {
-		return nil, err
-	}
-	res := <-resChan
-
-	log.Infof("Finished ARM Deployment. deployment=%q. res=%q", deploymentName, res)
-
-	return nil, nil
-}

pkg/armhelpers/interfaces.go

@@ -3,33 +3,23 @@ package armhelpers
 import (
 	"github.com/Azure/azure-sdk-for-go/arm/compute"
 	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
-	"github.com/Azure/go-autorest/autorest"
 )
 
-// the master client interface
-// if this is mocked out, then the entire context of the target environment is mocked:
-// once one of the NewAzureClient... returns, it has returned a fully initialized UberClient
-// all other code that needs to do work in subscription/AAD tenant will use this
-// currently only one impl: AzureClient
-type UberClient interface {
-	TemplateDeployer() TemplateDeployer // wraps the deployment client
-	VMClient() VMClient
+// ACSEngineClient is the interface used to talk to an Azure environment.
+// This interface exposes just the subset of Azure APIs and clients needed for
+// ACS-Engine.
+type ACSEngineClient interface {
+	DeploymentsClient() DeploymentsClient // wraps the deployment client
+	VirtualMachinesClient() VirtualMachinesClient
 }
 
-// TemplateDeployer is an interface that knows how to deploy templates
-type TemplateDeployer interface {
-	// Validate validates if a deployment is valid (e.g. it can be run successfully)
-	Validate(resourceGroup, name string, d resources.Deployment) (resources.DeploymentValidateResult, error)
-	// CreateOrUpdate a deployment of a template, returns an error if one occurs
-	CreateOrUpdate(resourceGroup, name string, d resources.Deployment, cancel <-chan struct{}) (<-chan resources.DeploymentExtended, <-chan error)
-	// Delete a deployment of a template, returns an error if one occurs
-	Delete(resourceGroup, name string, cancel <-chan struct{}) (<-chan autorest.Response, <-chan error)
-	// Get a deployment, returns nil if it doesn't exist, returns an error if one occurs
-	Get(resourceGroup, name string) (resources.DeploymentExtended, error)
+// DeploymentsClient exposes methods needed for handling Deployments
+type DeploymentsClient interface {
+	DeployTemplate(resourceGroup, name string, template, parameters map[string]interface{}, cancel <-chan struct{}) (*resources.DeploymentExtended, error)
 }
 
-// VMClient is an interface that knows how to do VM operations
-type VMClient interface {
+// VirtualMachinesClient exposes methods needed for handling VirtualMachines
+type VirtualMachinesClient interface {
 	// List lists VM resources
-	List(resourceGroup string) (compute.VirtualMachineListResult, error)
+	ListVirtualMachines(resourceGroup string) (compute.VirtualMachineListResult, error)
 }

pkg/armhelpers/templatedeployer.go

@@ -0,0 +1,41 @@
+package armhelpers
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/Azure/azure-sdk-for-go/arm/resources/resources"
+	"github.com/prometheus/common/log"
+)
+
+// DeployTemplate implements the TemplateDeployer interface for the AzureClient client
+func (az *AzureClient) DeployTemplate(resourceGroupName, deploymentName string, template map[string]interface{}, parameters map[string]interface{}, cancel <-chan struct{}) (*resources.DeploymentExtended, error) {
+	// this is needed because either ARM or the SDK can't distinguish between past
+	// deployments and current deployments with the same deploymentName.
+	uniqueSuffix := fmt.Sprintf("-%d", time.Now().Unix())
+	deploymentName = deploymentName + uniqueSuffix
+
+	deployment := resources.Deployment{
+		Properties: &resources.DeploymentProperties{
+			Template:   &template,
+			Parameters: &parameters,
+			Mode:       resources.Incremental,
+		},
+	}
+
+	log.Infof("Starting ARM Deployment. This will take some time. deployment=%q", deploymentName)
+
+	resChan, errChan := az.deploymentsClient.CreateOrUpdate(
+		resourceGroupName,
+		deploymentName,
+		deployment,
+		cancel)
+	if err := <-errChan; err != nil {
+		return nil, err
+	}
+	res := <-resChan
+
+	log.Infof("Finished ARM Deployment. deployment=%q. res=%q", deploymentName, res)
+
+	return &res, nil
+}

pkg/armhelpers/vmclient.go

@@ -0,0 +1,10 @@
+package armhelpers
+
+import (
+	"github.com/Azure/azure-sdk-for-go/arm/compute"
+)
+
+// ListVirtualMachines returns (the first page of) the machines in the specified resource group.
+func (az *AzureClient) ListVirtualMachines(resourceGroup string) (compute.VirtualMachineListResult, error) {
+	return az.virtualMachinesClient.List(resourceGroup)
+}

pkg/operations/upgradecluster.go

@@ -25,7 +25,7 @@ type ClusterTopology struct {
 // upgrades are supported for Kubernetes cluster only.
 type UpgradeCluster struct {
 	ClusterTopology
-	Client armhelpers.UberClient
+	Client armhelpers.ACSEngineClient
 }
 
 // UpgradeCluster runs the workflow to upgrade a Kubernetes cluster.
@@ -45,7 +45,7 @@ func (uc *UpgradeCluster) UpgradeCluster(subscriptionID uuid.UUID, resourceGroup
 }
 
 func (uc *UpgradeCluster) getUpgradableResources(subscriptionID uuid.UUID, resourceGroup string) error {
-	vmListResult, err := uc.Client.VMClient().List(resourceGroup)
+	vmListResult, err := uc.Client.VirtualMachinesClient().ListVirtualMachines(resourceGroup)
 	if err != nil {
 		return err
 	}