Skip to content

Commit

Permalink
address PR feedback
Browse files Browse the repository at this point in the history
reduce verbosity of logging (as errors get passed upwards)
don't rename default authorizer
  • Loading branch information
Joel Diaz committed Aug 23, 2019
1 parent 80ca614 commit 792ff7c
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 25 deletions.
2 changes: 1 addition & 1 deletion pkg/asset/installconfig/azure/azure.go
Expand Up @@ -80,7 +80,7 @@ func getRegions() (map[string]string, error) {
return nil, err
}
client := azsub.NewClient()
client.Authorizer = session.ResourceManagerAuthorizer
client.Authorizer = session.Authorizer
ctx, cancel := context.WithTimeout(context.TODO(), 30*time.Second)
defer cancel()
locations, err := client.ListLocations(ctx, session.Credentials.SubscriptionID)
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/installconfig/azure/dns.go
Expand Up @@ -109,7 +109,7 @@ func NewDNSConfig() (*DNSConfig, error) {

func newZonesClient(session *Session) ZonesGetter {
azureClient := azdns.NewZonesClient(session.Credentials.SubscriptionID)
azureClient.Authorizer = session.ResourceManagerAuthorizer
azureClient.Authorizer = session.Authorizer
return &ZonesClient{azureClient: azureClient}
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/asset/installconfig/azure/session.go
Expand Up @@ -20,9 +20,9 @@ var defaultAuthFilePath = filepath.Join(os.Getenv("HOME"), ".azure", "osServiceP

//Session is an object representing session for subscription
type Session struct {
GraphAuthorizer autorest.Authorizer
ResourceManagerAuthorizer autorest.Authorizer
Credentials Credentials
GraphAuthorizer autorest.Authorizer
Authorizer autorest.Authorizer
Credentials Credentials
}

//Credentials is the data type for credentials as understood by the azure sdk
Expand Down Expand Up @@ -71,7 +71,7 @@ func newSessionFromFile(authFilePath string) (*Session, error) {
return nil, errors.Wrap(err, "failed to map authsettings to credentials")
}

rmAuthorizer, err := authSettings.ClientCredentialsAuthorizerWithResource(azureenv.PublicCloud.ResourceManagerEndpoint)
authorizer, err := authSettings.ClientCredentialsAuthorizerWithResource(azureenv.PublicCloud.ResourceManagerEndpoint)
if err != nil {
return nil, errors.Wrap(err, "failed to get client credentials authorizer from saved azure auth settings")
}
Expand All @@ -82,9 +82,9 @@ func newSessionFromFile(authFilePath string) (*Session, error) {
}

return &Session{
GraphAuthorizer: graphAuthorizer,
ResourceManagerAuthorizer: rmAuthorizer,
Credentials: *credentials,
GraphAuthorizer: graphAuthorizer,
Authorizer: authorizer,
Credentials: *credentials,
}, nil
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/machines/azure/zones.go
Expand Up @@ -32,7 +32,7 @@ func skusClient() (client *compute.ResourceSkusClient, err error) {
}

skusClient := compute.NewResourceSkusClient(ssn.Credentials.SubscriptionID)
skusClient.Authorizer = ssn.ResourceManagerAuthorizer
skusClient.Authorizer = ssn.Authorizer
return &skusClient, nil
}

Expand Down
27 changes: 12 additions & 15 deletions pkg/destroy/azure/azure.go
Expand Up @@ -25,10 +25,10 @@ import (

// ClusterUninstaller holds the various options for the cluster we want to delete.
type ClusterUninstaller struct {
SubscriptionID string
TenantID string
GraphAuthorizer autorest.Authorizer
ResourceManagerAuthorizer autorest.Authorizer
SubscriptionID string
TenantID string
GraphAuthorizer autorest.Authorizer
Authorizer autorest.Authorizer

InfraID string

Expand All @@ -46,10 +46,10 @@ func (o *ClusterUninstaller) configureClients() {
o.resourceGroupsClient.Authorizer = o.ResourceManagerAuthorizer

o.zonesClient = dns.NewZonesClient(o.SubscriptionID)
o.zonesClient.Authorizer = o.ResourceManagerAuthorizer
o.zonesClient.Authorizer = o.Authorizer

o.recordsClient = dns.NewRecordSetsClient(o.SubscriptionID)
o.recordsClient.Authorizer = o.ResourceManagerAuthorizer
o.recordsClient.Authorizer = o.Authorizer

o.serviceprincipalsClient = graphrbac.NewServicePrincipalsClient(o.TenantID)
o.serviceprincipalsClient.Authorizer = o.GraphAuthorizer
Expand All @@ -66,12 +66,12 @@ func New(logger logrus.FieldLogger, metadata *types.ClusterMetadata) (providers.
}

return &ClusterUninstaller{
SubscriptionID: session.Credentials.SubscriptionID,
TenantID: session.Credentials.TenantID,
GraphAuthorizer: session.GraphAuthorizer,
ResourceManagerAuthorizer: session.ResourceManagerAuthorizer,
InfraID: metadata.InfraID,
Logger: logger,
SubscriptionID: session.Credentials.SubscriptionID,
TenantID: session.Credentials.TenantID,
GraphAuthorizer: session.GraphAuthorizer,
Authorizer: session.Authorizer,
InfraID: metadata.InfraID,
Logger: logger,
}, nil
}

Expand Down Expand Up @@ -250,22 +250,19 @@ func deleteApplicationRegistrations(ctx context.Context, appClient graphrbac.App
appFilter := fmt.Sprintf("appId eq '%s'", *sp.AppID)
appResults, err := appClient.List(ctx, appFilter)
if err != nil {
logger.WithError(err).Errorf("unable to get App")
errorList = append(errorList, err)
continue
}

apps := appResults.Values()
if len(apps) != 1 {
msg := fmt.Sprintf("should have recieved only a single result matching AppID, received %d instead", len(apps))
logger.Error(msg)
errorList = append(errorList, errors.New(msg))
continue
}

_, err = appClient.Delete(ctx, *apps[0].ObjectID)
if err != nil {
logger.WithError(err).Error("error deleting Application Registration")
errorList = append(errorList, err)
continue
}
Expand Down

0 comments on commit 792ff7c

Please sign in to comment.