pkg/destroy/azure: Delete app registrations during cluster destroy #2262

joelddiaz · 2019-08-23T14:30:47Z

With cloud-cred-operator tagging Service Principals, we can now located the Service Principals by tag, and find their parent Application Registration and delete them as part of cluster destroy.

This should keep the App Registrations created by cloud-cred-operator from leaking out after cluster create/destroy.

dep ensure -v (with some code importing github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac)

abhinavdahiya · 2019-08-23T17:43:52Z

dont' log errors with ERROR level, the destroy for azure returns them and prints them in DEBUG.

abhinavdahiya · 2019-08-23T18:23:32Z

pkg/destroy/azure/azure.go

 }

 func (o *ClusterUninstaller) configureClients() {
 	o.resourceGroupsClient = resources.NewGroupsGroupClient(o.SubscriptionID)
-	o.resourceGroupsClient.Authorizer = o.Authorizer
+	o.resourceGroupsClient.Authorizer = o.ResourceManagerAuthorizer


hmm.. this should be Authorizer

abhinavdahiya · 2019-08-23T18:27:30Z

pkg/destroy/azure/azure.go

+	for _, sp := range servicePrincipals {
+		logger = logger.WithField("appID", *sp.AppID)
+		appFilter := fmt.Sprintf("appId eq '%s'", *sp.AppID)
+		appResults, err := appClient.List(ctx, appFilter)


why not use https://godoc.org/github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac#ApplicationsClient.Get vs listing for appID can there be more than one with same ID???

the Get requires querying the objectID not the appID

[jdiaz@minigoomba ~]$ az ad app list --display-name jdiaz-app | jq -r ".[].appId , .[].objectId" 9ba357ad-0bf5-4a0c-8c3f-8e6eee8e6b36 10dbbb0c-29ae-4705-8bf9-95e99f0e8a3b

And all you get from the service principal is the appID of the app registration.

abhinavdahiya · 2019-08-23T18:29:08Z

pkg/destroy/azure/azure.go

+		return matchedSPs, err
+	}
+
+	for {


use somethings like

installer/pkg/asset/installconfig/azure/dns.go

Lines 121 to 132 in 0454021

for zonesPage, err := client.azureClient.List(ctx, to.Int32Ptr(100)); zonesPage.NotDone(); err = zonesPage.NextWithContext(ctx) {

if err != nil {

return nil, err

}

//TODO: filter out private zone and show only public zones.

//the property is present in the REST api response, but not mapped yet in the stable SDK (present in preview)

//https://github.com/Azure/azure-sdk-for-go/blob/07f918ba2d513bbc5b75bc4caac845e10f27449e/services/preview/dns/mgmt/2018-03-01-preview/dns/models.go#L857

for _, zone := range zonesPage.Values() {

allZones[to.String(zone.Name)] = to.String(zone.ID)

}

}

return allZones, nil

👍 i much prefer this. will change it

abhinavdahiya · 2019-08-23T22:46:55Z

/test e2e-azure

LGTM

con you squash the commits 80ca614 and 96d22b6

abhinavdahiya · 2019-08-23T22:47:27Z

/test e2e-azure

joelddiaz · 2019-08-23T23:08:01Z

squashed

joelddiaz · 2019-08-26T13:24:44Z

/retest

joelddiaz · 2019-08-26T13:56:48Z

/test e2e-azure

joelddiaz · 2019-08-26T15:44:11Z

/test e2e-azure

abhinavdahiya · 2019-08-26T15:55:29Z

https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/2262/pull-ci-openshift-installer-master-e2e-azure/146/artifacts/e2e-azure/pods/openshift-cloud-credential-operator_cloud-credential-operator-65d96ddfdf-96fdd_manager.log

The cred operator created service principals.

https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/2262/pull-ci-openshift-installer-master-e2e-azure/146/artifacts/e2e-azure/installer/.openshift_install.log
time="2019-08-26T15:03:06Z" level=debug msg="deleting public records"
time="2019-08-26T15:03:08Z" level=debug msg="removing matching private records from ci.azure.devcluster.openshift.com"
time="2019-08-26T15:03:10Z" level=info msg=deleted record=api.ci-op-1xw3yxn7-2dc90
time="2019-08-26T15:03:11Z" level=info msg=deleted record="*.apps.ci-op-1xw3yxn7-2dc90"
time="2019-08-26T15:03:11Z" level=debug msg="deleting resource group"
time="2019-08-26T15:17:01Z" level=info msg=deleted resource group=ci-op-1xw3yxn7-2dc90-7jkwd-rg
time="2019-08-26T15:17:01Z" level=debug msg="deleting application registrations
time="2019-08-26T15:17:02Z" level=debug msg="Purging asset "Terraform Variables" from disk"

Doesn't seem to have cleaned up any???

joelddiaz · 2019-08-26T17:55:43Z

time="2019-08-26T15:17:01Z" level=debug msg="deleting application registrations
time="2019-08-26T15:17:02Z" level=debug msg="Purging asset "Terraform Variables" from disk"

Doesn't seem to have cleaned up any???

I think I see what's happening. The cloud-cred-operator clips the infra prefix to 20 characters (https://github.com/openshift/cloud-credential-operator/blob/master/pkg/azure/actuator.go#L497-L500), so when searching for Service Principals, this code should do the same.

abhinavdahiya · 2019-08-26T18:20:28Z

time="2019-08-26T15:17:01Z" level=debug msg="deleting application registrations
time="2019-08-26T15:17:02Z" level=debug msg="Purging asset "Terraform Variables" from disk"

Doesn't seem to have cleaned up any???

I think I see what's happening. The cloud-cred-operator clips the infra prefix to 20 characters (https://github.com/openshift/cloud-credential-operator/blob/master/pkg/azure/actuator.go#L497-L500), so when searching for Service Principals, this code should do the same.

https://github.com/openshift/cloud-credential-operator/blob/14ea16f11aaf6b77d3bce0dcfb93546e12da6942/pkg/azure/actuator.go#L504-L506

make it unclip the credential name, why is it clipping the infra-id??

joelddiaz · 2019-08-26T19:28:27Z

I think I see what's happening. The cloud-cred-operator clips the infra prefix to 20 characters (https://github.com/openshift/cloud-credential-operator/blob/master/pkg/azure/actuator.go#L497-L500), so when searching for Service Principals, this code should do the same.

https://github.com/openshift/cloud-credential-operator/blob/14ea16f11aaf6b77d3bce0dcfb93546e12da6942/pkg/azure/actuator.go#L504-L506

make it unclip the credential name, why is it clipping the infra-id??

I think it was done out of concern for the 93 character limit. @ingvagabund maybe you can give more context?

ingvagabund · 2019-08-27T08:28:53Z

I think it was done out of concern for the 93 character limit. @ingvagabund maybe you can give more context?

Two things:

aws actuator clips the infra name when generating a user name: https://github.com/openshift/cloud-credential-operator/blob/master/pkg/aws/actuator/actuator.go#L1119-L1121. I followed the same pattern
Azure does not allow application (SP) names to be longer than 93 characters. It errors otherwise.

joelddiaz · 2019-08-27T16:06:53Z

/test e2e-azure

abhinavdahiya · 2019-08-27T16:14:43Z

I think it was done out of concern for the 93 character limit. @ingvagabund maybe you can give more context?

Two things:

1. aws actuator clips the infra name when generating a user name: https://github.com/openshift/cloud-credential-operator/blob/master/pkg/aws/actuator/actuator.go#L1119-L1121. I followed the same pattern

2. Azure does not allow application (SP) names to be longer than 93 characters. It errors otherwise.

But if depend on infra-id to filter the the service principals, I would rather truncate the credential name here.. the infra-id is max 32..

joelddiaz · 2019-08-27T19:17:01Z

PR to raise infra field length to 32 here openshift/cloud-credential-operator#108

With cloud-cred-operator tagging Service Principals, we can now located the Service Principals by tag, and find their parent Application Registration and delete them as part of cluster destroy.

joelddiaz · 2019-08-29T14:54:11Z

/test e2e-azure

joelddiaz · 2019-08-29T15:45:05Z

looks like it's deleting the app registrations now

time="2019-08-27T18:08:30Z" level=debug msg="deleting application registrations"
time="2019-08-27T18:08:31Z" level=info msg=deleted appID=f18a9b1f-72ff-43db-8a73-abf73bc37ad9
time="2019-08-27T18:08:37Z" level=info msg=deleted appID=47c260de-9275-4db2-b889-c34334c33ee9
time="2019-08-27T18:08:43Z" level=info msg=deleted appID=2e39df42-0018-472c-b1b0-20c524eb2d3c

just need to get through CI now...

abhinavdahiya · 2019-08-29T16:04:30Z

/approve

joelddiaz · 2019-08-29T17:00:13Z

/retest

joelddiaz · 2019-08-29T19:45:41Z

/retest

abhinavdahiya · 2019-08-29T20:58:48Z

/test e2e-azure

joelddiaz · 2019-08-30T11:22:16Z

/retest

joelddiaz · 2019-08-30T15:09:13Z

/test e2e-aws-scaleup-rhel7

abhinavdahiya · 2019-08-30T15:21:24Z

/lgtm

openshift-ci-robot · 2019-08-30T15:21:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, joelddiaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-08-30T17:44:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-08-30T19:03:32Z

@joelddiaz: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`742e463`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-libvirt	`742e463`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vendor azure sdk graphrbac

9447003

dep ensure -v (with some code importing github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac)

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 23, 2019

openshift-ci-robot requested review from jhixson74 and patrickdillon August 23, 2019 14:31

wking changed the title ~~delete app registrations during cluster destroy~~ pkg/destroy/azure: Delete app registrations during cluster destroy Aug 23, 2019

abhinavdahiya reviewed Aug 23, 2019

View reviewed changes

joelddiaz force-pushed the delete-appregistration branch from 792ff7c to 96d22b6 Compare August 23, 2019 19:39

joelddiaz force-pushed the delete-appregistration branch from 96d22b6 to 73a2b95 Compare August 23, 2019 23:07

joelddiaz force-pushed the delete-appregistration branch from 73a2b95 to d6f6c15 Compare August 26, 2019 18:40

locate and delete app registrations by tag

742e463

With cloud-cred-operator tagging Service Principals, we can now located the Service Principals by tag, and find their parent Application Registration and delete them as part of cluster destroy.

joelddiaz force-pushed the delete-appregistration branch from d6f6c15 to 742e463 Compare August 29, 2019 14:47

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019

openshift-ci-robot assigned abhinavdahiya Aug 30, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2019

openshift-merge-robot merged commit cfcfd81 into openshift:master Aug 30, 2019

joelddiaz deleted the delete-appregistration branch January 6, 2020 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/destroy/azure: Delete app registrations during cluster destroy #2262

pkg/destroy/azure: Delete app registrations during cluster destroy #2262

joelddiaz commented Aug 23, 2019

abhinavdahiya commented Aug 23, 2019

abhinavdahiya Aug 23, 2019

joelddiaz Aug 23, 2019

abhinavdahiya Aug 23, 2019

joelddiaz Aug 23, 2019

abhinavdahiya Aug 23, 2019

joelddiaz Aug 23, 2019

abhinavdahiya commented Aug 23, 2019

abhinavdahiya commented Aug 23, 2019

joelddiaz commented Aug 23, 2019

joelddiaz commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

abhinavdahiya commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

abhinavdahiya commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

ingvagabund commented Aug 27, 2019 •

edited

joelddiaz commented Aug 27, 2019

abhinavdahiya commented Aug 27, 2019

joelddiaz commented Aug 27, 2019

joelddiaz commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

abhinavdahiya commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

abhinavdahiya commented Aug 29, 2019

joelddiaz commented Aug 30, 2019

joelddiaz commented Aug 30, 2019

abhinavdahiya commented Aug 30, 2019

openshift-ci-robot commented Aug 30, 2019

openshift-bot commented Aug 30, 2019

openshift-ci-robot commented Aug 30, 2019 •

edited

	for zonesPage, err := client.azureClient.List(ctx, to.Int32Ptr(100)); zonesPage.NotDone(); err = zonesPage.NextWithContext(ctx) {
	if err != nil {
	return nil, err
	}
	//TODO: filter out private zone and show only public zones.
	//the property is present in the REST api response, but not mapped yet in the stable SDK (present in preview)
	//https://github.com/Azure/azure-sdk-for-go/blob/07f918ba2d513bbc5b75bc4caac845e10f27449e/services/preview/dns/mgmt/2018-03-01-preview/dns/models.go#L857
	for _, zone := range zonesPage.Values() {
	allZones[to.String(zone.Name)] = to.String(zone.ID)
	}
	}
	return allZones, nil

pkg/destroy/azure: Delete app registrations during cluster destroy #2262

pkg/destroy/azure: Delete app registrations during cluster destroy #2262

Conversation

joelddiaz commented Aug 23, 2019

abhinavdahiya commented Aug 23, 2019

abhinavdahiya Aug 23, 2019

Choose a reason for hiding this comment

joelddiaz Aug 23, 2019

Choose a reason for hiding this comment

abhinavdahiya Aug 23, 2019

Choose a reason for hiding this comment

joelddiaz Aug 23, 2019

Choose a reason for hiding this comment

abhinavdahiya Aug 23, 2019

Choose a reason for hiding this comment

joelddiaz Aug 23, 2019

Choose a reason for hiding this comment

abhinavdahiya commented Aug 23, 2019

abhinavdahiya commented Aug 23, 2019

joelddiaz commented Aug 23, 2019

joelddiaz commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

abhinavdahiya commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

abhinavdahiya commented Aug 26, 2019

joelddiaz commented Aug 26, 2019

ingvagabund commented Aug 27, 2019 • edited

joelddiaz commented Aug 27, 2019

abhinavdahiya commented Aug 27, 2019

joelddiaz commented Aug 27, 2019

joelddiaz commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

abhinavdahiya commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

joelddiaz commented Aug 29, 2019

abhinavdahiya commented Aug 29, 2019

joelddiaz commented Aug 30, 2019

joelddiaz commented Aug 30, 2019

abhinavdahiya commented Aug 30, 2019

openshift-ci-robot commented Aug 30, 2019

openshift-bot commented Aug 30, 2019

openshift-ci-robot commented Aug 30, 2019 • edited

ingvagabund commented Aug 27, 2019 •

edited

openshift-ci-robot commented Aug 30, 2019 •

edited