Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Adding DeleteApp func to AzureClient and returning appObjectID in CreateApp #3869

Merged
merged 7 commits into from
Sep 19, 2018
Merged

Adding DeleteApp func to AzureClient and returning appObjectID in CreateApp #3869

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
931da57
adding delete app fn and adding appObjectID to createApp
Sep 19, 2018
a244076
changed appName to applicationName
Sep 19, 2018
8dd4afb
returning autorest.Response from CreateApp
Sep 19, 2018
a68ac31
returning graphrbac.Application type for CreateApp
Sep 19, 2018
9df2d47
correcting param name
Sep 19, 2018
33f10f8
updating test files
Sep 19, 2018
f1a4484
updating test files
Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func autofillApimodel(dc *deployCmd) error {
},
}
}
applicationID, servicePrincipalObjectID, secret, err := dc.client.CreateApp(ctx, appName, appURL, replyURLs, requiredResourceAccess)
_, applicationID, servicePrincipalObjectID, secret, err := dc.client.CreateApp(ctx, appName, appURL, replyURLs, requiredResourceAccess)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shrutir25 I think it would be good to avoid underscore placeholders since it goes against the tenets of idiomatic Go. We could change the return type of CreateApp to autorest.Response instead.

I see two benefits

  1. We address the concern above.

  2. It also becomes consistent with DeleteApplication.

if err != nil {
return errors.Wrap(err, "apimodel invalid: ServicePrincipalProfile was empty, and we failed to create valid credentials")
}
Expand Down
25 changes: 21 additions & 4 deletions pkg/armhelpers/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/Azure/azure-sdk-for-go/services/authorization/mgmt/2015-07-01/authorization"
"github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac"
"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/date"
"github.com/Azure/go-autorest/autorest/to"
"github.com/satori/go.uuid"
Expand All @@ -28,6 +29,11 @@ func (az *AzureClient) CreateGraphApplication(ctx context.Context, applicationCr
return az.applicationsClient.Create(ctx, applicationCreateParameters)
}

// DeleteGraphApplication creates an application via the graphrbac client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should say "deletes an application".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup ! I ll change it now

func (az *AzureClient) DeleteGraphApplication(ctx context.Context, applicationObjectID string) (result autorest.Response, err error) {
return az.applicationsClient.Delete(ctx, applicationObjectID)
}

// CreateGraphPrincipal creates a service principal via the graphrbac client
func (az *AzureClient) CreateGraphPrincipal(ctx context.Context, servicePrincipalCreateParameters graphrbac.ServicePrincipalCreateParameters) (graphrbac.ServicePrincipal, error) {
return az.servicePrincipalsClient.Create(ctx, servicePrincipalCreateParameters)
Expand All @@ -50,7 +56,7 @@ func (az *AzureClient) ListRoleAssignmentsForPrincipal(ctx context.Context, scop
}

// CreateApp is a simpler method for creating an application
func (az *AzureClient) CreateApp(ctx context.Context, appName, appURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationID, servicePrincipalObjectID, servicePrincipalClientSecret string, err error) {
func (az *AzureClient) CreateApp(ctx context.Context, appName, appURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationObjectID, applicationID, servicePrincipalObjectID, servicePrincipalClientSecret string, err error) {
notBefore := time.Now()
notAfter := time.Now().Add(10000 * 24 * time.Hour)

Expand Down Expand Up @@ -78,9 +84,10 @@ func (az *AzureClient) CreateApp(ctx context.Context, appName, appURL string, re
}
applicationResp, err := az.CreateGraphApplication(ctx, applicationReq)
if err != nil {
return "", "", "", err
return "", "", "", "", err
}
applicationID = to.String(applicationResp.AppID)
applicationObjectID = to.String(applicationResp.ObjectID)

log.Debugf("ad: creating servicePrincipal for applicationID: %q", applicationID)

Expand All @@ -90,12 +97,22 @@ func (az *AzureClient) CreateApp(ctx context.Context, appName, appURL string, re
}
servicePrincipalResp, err := az.servicePrincipalsClient.Create(ctx, servicePrincipalReq)
if err != nil {
return "", "", "", err
return "", "", "", "", err
}

servicePrincipalObjectID = to.String(servicePrincipalResp.ObjectID)

return applicationID, servicePrincipalObjectID, servicePrincipalClientSecret, nil
return applicationObjectID, applicationID, servicePrincipalObjectID, servicePrincipalClientSecret, nil
}

// DeleteApp is a simpler method for deleting an application and the associated spn
func (az *AzureClient) DeleteApp(ctx context.Context, appName, applicationObjectID string) (autorest.Response, error) {
shrutir25 marked this conversation as resolved.
Show resolved Hide resolved
log.Debugf("ad: deleting application with name=%q", appName)
applicationResp, err := az.DeleteGraphApplication(ctx, applicationObjectID)
if err != nil {
return applicationResp, err
}
return applicationResp, nil
}

// CreateRoleAssignmentSimple is a wrapper around RoleAssignmentsClient.Create
Expand Down
4 changes: 3 additions & 1 deletion pkg/armhelpers/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/Azure/azure-sdk-for-go/services/preview/msi/mgmt/2015-08-31-preview/msi"
"github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources"
azStorage "github.com/Azure/azure-sdk-for-go/storage"
"github.com/Azure/go-autorest/autorest"
log "github.com/sirupsen/logrus"
"k8s.io/api/core/v1"
)
Expand Down Expand Up @@ -102,7 +103,8 @@ type ACSEngineClient interface {

// CreateGraphPrincipal creates a service principal via the graphrbac client
CreateGraphPrincipal(ctx context.Context, servicePrincipalCreateParameters graphrbac.ServicePrincipalCreateParameters) (graphrbac.ServicePrincipal, error)
CreateApp(ctx context.Context, applicationName, applicationURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationID, servicePrincipalObjectID, secret string, err error)
CreateApp(ctx context.Context, applicationName, applicationURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationObjectID, applicationID, servicePrincipalObjectID, secret string, err error)
DeleteApp(ctx context.Context, appName, applicationObjectID string) (autorest.Response, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this parameter applicationName for consistency with CreateApp and with other parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure makes sense !


// User Assigned MSI
//CreateUserAssignedID - Creates a user assigned msi.
Expand Down
9 changes: 7 additions & 2 deletions pkg/armhelpers/mockclients.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,13 @@ func (mc *MockACSEngineClient) CreateGraphPrincipal(ctx context.Context, service
}

// CreateApp is a simpler method for creating an application
func (mc *MockACSEngineClient) CreateApp(ctx context.Context, applicationName, applicationURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationID, servicePrincipalObjectID, secret string, err error) {
return "app-id", "client-id", "client-secret", nil
func (mc *MockACSEngineClient) CreateApp(ctx context.Context, applicationName, applicationURL string, replyURLs *[]string, requiredResourceAccess *[]graphrbac.RequiredResourceAccess) (applicationObjectID, applicationID, servicePrincipalObjectID, secret string, err error) {
return "app-object-id", "app-id", "client-id", "client-secret", nil
}

// DeleteApp is a simpler method for deleting an application
func (mc *MockACSEngineClient) DeleteApp(ctx context.Context, appName, applicationObjectID string) (response autorest.Response, err error) {
return response, nil
}

// User Assigned MSI
Expand Down