OCPBUGS-4549: destroy: azure: handle `nil` responses from msgraph sdk #6717

r4f4 · 2022-12-19T09:36:47Z

It seems the SDK can return nil as a response even if no errors occurred. If we don't check for that, we get the following error on Gov cloud:

12-19 13:21:52.860  level=debug msg=deleting application registrations
12-19 13:21:52.860  E1219 05:21:48.969224     743 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
12-19 13:21:52.860  goroutine 1 [running]:
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.logPanic({0x6132160?, 0x22371b20})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x0?})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
12-19 13:21:52.861  panic({0x6132160, 0x22371b20})
12-19 13:21:52.861  	/usr/lib/golang/src/runtime/panic.go:884 +0x212
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.getServicePrincipalsByTag({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0xc000792570, 0x29}, {0xc000dcc650, 0xd})
12-19 13:21:52.861  	/go/src/github.com/openshift/installer/pkg/destroy/azure/azure.go:632 +0x322
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.deleteApplicationRegistrations({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0x1d98a0d0, 0xc0001da000}, {0xc000dcc650, 0xd})

This is a fallout of #6614

openshift-ci-robot · 2022-12-19T09:39:12Z

@r4f4: This pull request references Jira Issue OCPBUGS-4549, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jinyunma

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

It seems the SDK can return nil as a response even if no errors occurred. If we don't check for that, we get the following error on Gov cloud:

12-19 13:21:52.860  level=debug msg=deleting application registrations
12-19 13:21:52.860  E1219 05:21:48.969224     743 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
12-19 13:21:52.860  goroutine 1 [running]:
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.logPanic({0x6132160?, 0x22371b20})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x0?})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
12-19 13:21:52.861  panic({0x6132160, 0x22371b20})
12-19 13:21:52.861  	/usr/lib/golang/src/runtime/panic.go:884 +0x212
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.getServicePrincipalsByTag({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0xc000792570, 0x29}, {0xc000dcc650, 0xd})
12-19 13:21:52.861  	/go/src/github.com/openshift/installer/pkg/destroy/azure/azure.go:632 +0x322
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.deleteApplicationRegistrations({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0x1d98a0d0, 0xc0001da000}, {0xc000dcc650, 0xd})

This is a fallout of #6614

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.

pkg/destroy/azure/azure.go

patrickdillon · 2022-12-19T19:18:36Z

pkg/destroy/azure/azure.go

Do we know what a nil response means? Was the deletion successful? API not supported?

If we don't know, or aren't certain, I would suggest do a little more here in terms of logging, perhaps a warning or or adding an error to the error list.

In this case it's just a GET (all the apps with ID == appID), the delete happens a few lines later. I cannot easily tell why, would have to dig dipper into the msgraph code: https://github.com/microsoftgraph/msgraph-sdk-go/blob/main/serviceprincipals/service_principal_item_request_builder.go#L267-L273

I think this is the SendAsync function: https://github.com/microsoft/kiota-http-go/blob/main/nethttp_request_adapter.go#L286-L287

Anyway, it's probably a good idea to at least add some logging in those cases. Thanks.

pkg/destroy/azure/azure.go

r4f4 · 2022-12-19T21:47:57Z

/hold
while we check what happens if we return error instead of ignoring a nil response

It seems the SDK can return `nil` as a response even if no errors occurred. If we don't check for that, we get the following error on Gov cloud: ``` 12-19 13:21:52.860 level=debug msg=deleting application registrations 12-19 13:21:52.860 E1219 05:21:48.969224 743 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference) 12-19 13:21:52.860 goroutine 1 [running]: 12-19 13:21:52.860 k8s.io/apimachinery/pkg/util/runtime.logPanic({0x6132160?, 0x22371b20}) 12-19 13:21:52.860 /go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99 12-19 13:21:52.860 k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x0?}) 12-19 13:21:52.860 /go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75 12-19 13:21:52.861 panic({0x6132160, 0x22371b20}) 12-19 13:21:52.861 /usr/lib/golang/src/runtime/panic.go:884 +0x212 12-19 13:21:52.861 github.com/openshift/installer/pkg/destroy/azure.getServicePrincipalsByTag({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0xc000792570, 0x29}, {0xc000dcc650, 0xd}) 12-19 13:21:52.861 /go/src/github.com/openshift/installer/pkg/destroy/azure/azure.go:632 +0x322 12-19 13:21:52.861 github.com/openshift/installer/pkg/destroy/azure.deleteApplicationRegistrations({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0x1d98a0d0, 0xc0001da000}, {0xc000dcc650, 0xd}) ```

r4f4 · 2022-12-20T19:36:54Z

/hold cancel

barbacbd

/lgtm

sadasu · 2023-01-04T21:20:58Z

pkg/destroy/azure/azure.go

 	if err != nil {
 		return errors.Wrap(err, "failed to gather list of Service Principals by tag")
 	}
+	// msgraphsdk can return a `nil` response even if no errors occurred


This logic can be moved within https://github.com/openshift/installer/blob/master/pkg/destroy/azure/azure.go#L621. Also, looking at getServicePrincipalsByTag(), it appears that this method can set resp to nil even if the original API did not return a nil.

I preferred to do it here so I don't have to change the getServicePrincipalsByTag to receive a logger just so I can log a message in case that happens. I can change it if you think there is an advantage in doing that.

Also, looking at getServicePrincipalsByTag(), it appears that this method can set resp to nil even if the original API did not return a nil.

That's OK because in that case err != nil and the error condition will (and should) be evaluated first.

sadasu · 2023-01-04T21:21:31Z

pkg/destroy/azure/azure.go

+			logger.Debugf("Empty response getting Application from Service Principal %s", *sp.GetDisplayName())
+			continue
+		}
 		apps := resp.GetValue()


Do we have to chck for resp == nil for https://github.com/openshift/installer/blob/master/pkg/destroy/azure/azure.go#L611?

Hard to say since the API is not very clear about it. I don't think so since in the docs they don't mention any error handling https://learn.microsoft.com/en-us/graph/sdks/create-requests?tabs=Go#delete-an-entity. Notice that ApplicationsById does not return an Application object, but an ApplicationItemRequestBuilder instead. I'd be very surprised if a nil response was returned for that.

patrickdillon · 2023-01-20T19:30:50Z

/approve

openshift-ci · 2023-01-20T19:31:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/destroy/azure/OWNERS~~ [patrickdillon]

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

openshift-ci-robot · 2023-01-20T20:16:51Z

/retest-required

Remaining retests: 0 against base HEAD 3d92c1e and 2 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-21T00:28:34Z

/retest-required

Remaining retests: 0 against base HEAD 7c387e7 and 1 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-21T04:28:30Z

/retest-required

Remaining retests: 0 against base HEAD 26ce7ce and 0 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-21T08:28:26Z

/hold

Revision 420b556 was retested 3 times: holding

r4f4 · 2023-01-24T11:03:52Z

/hold cancel

r4f4 · 2023-01-24T11:04:05Z

/skip

openshift-ci-robot · 2023-01-24T11:35:31Z

/retest-required

Remaining retests: 0 against base HEAD 950ed9f and 2 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-24T20:22:02Z

/retest-required

Remaining retests: 0 against base HEAD 8c032f1 and 1 for PR HEAD 420b556 in total

r4f4 · 2023-01-25T18:58:47Z

/retest-required

openshift-ci-robot · 2023-01-25T23:36:17Z

/retest-required

Remaining retests: 0 against base HEAD a611870 and 0 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-26T02:36:19Z

/hold

Revision 420b556 was retested 3 times: holding

r4f4 · 2023-01-26T13:35:27Z

/hold cancel

openshift-ci-robot · 2023-01-26T14:35:47Z

/retest-required

Remaining retests: 0 against base HEAD 29cafe7 and 2 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-27T00:07:41Z

/retest-required

Remaining retests: 0 against base HEAD 148d2fb and 1 for PR HEAD 420b556 in total

openshift-ci · 2023-01-27T07:46:36Z

@r4f4: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agent-compact-ipv4	746cc30f245fdfdec3da185c5607b7e04445706f	link	true	`/test e2e-agent-compact-ipv4`
ci/prow/e2e-azure-ovn-shared-vpc	`420b556`	link	false	`/test e2e-azure-ovn-shared-vpc`
ci/prow/e2e-azure-ovn-resourcegroup	`420b556`	link	false	`/test e2e-azure-ovn-resourcegroup`
ci/prow/e2e-aws-ovn-workers-rhel8	`420b556`	link	false	`/test e2e-aws-ovn-workers-rhel8`
ci/prow/e2e-aws-ovn-disruptive	`420b556`	link	false	`/test e2e-aws-ovn-disruptive`
ci/prow/e2e-azurestack	`420b556`	link	false	`/test e2e-azurestack`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci-robot · 2023-01-27T08:07:55Z

/retest-required

Remaining retests: 0 against base HEAD fca4137 and 0 for PR HEAD 420b556 in total

openshift-ci-robot · 2023-01-27T12:38:44Z

@r4f4: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-4549 has been moved to the MODIFIED state.

Details

In response to this:

It seems the SDK can return nil as a response even if no errors occurred. If we don't check for that, we get the following error on Gov cloud:

12-19 13:21:52.860  level=debug msg=deleting application registrations
12-19 13:21:52.860  E1219 05:21:48.969224     743 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
12-19 13:21:52.860  goroutine 1 [running]:
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.logPanic({0x6132160?, 0x22371b20})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:75 +0x99
12-19 13:21:52.860  k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x0?})
12-19 13:21:52.860  	/go/src/github.com/openshift/installer/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:49 +0x75
12-19 13:21:52.861  panic({0x6132160, 0x22371b20})
12-19 13:21:52.861  	/usr/lib/golang/src/runtime/panic.go:884 +0x212
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.getServicePrincipalsByTag({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0xc000792570, 0x29}, {0xc000dcc650, 0xd})
12-19 13:21:52.861  	/go/src/github.com/openshift/installer/pkg/destroy/azure/azure.go:632 +0x322
12-19 13:21:52.861  github.com/openshift/installer/pkg/destroy/azure.deleteApplicationRegistrations({0x1d9580b0, 0xc0008443c0}, 0xc000cec420, {0x1d98a0d0, 0xc0001da000}, {0xc000dcc650, 0xd})

This is a fallout of #6614

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.

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 19, 2022

barbacbd reviewed Dec 19, 2022

View reviewed changes

pkg/destroy/azure/azure.go Outdated Show resolved Hide resolved

r4f4 force-pushed the azure-servp-fix branch from 0e49c94 to 746cc30 Compare December 19, 2022 16:21

patrickdillon reviewed Dec 19, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2022

r4f4 force-pushed the azure-servp-fix branch from 8609a62 to 420b556 Compare December 20, 2022 19:34

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2022

barbacbd reviewed Dec 20, 2022

View reviewed changes

openshift-ci bot assigned barbacbd Dec 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2022

sadasu reviewed Jan 4, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023

openshift-merge-robot merged commit 5ad7ab9 into openshift:master Jan 27, 2023

openshift-ci-robot mentioned this pull request Feb 11, 2023

OCPBUGS-4549: azure: fix MS Graph calls on Gov cloud #6844

Merged

OCPBUGS-4549: destroy: azure: handle nil responses from msgraph sdk #6717

OCPBUGS-4549: destroy: azure: handle nil responses from msgraph sdk #6717

Uh oh!

Conversation

r4f4 commented Dec 19, 2022

Uh oh!

openshift-ci-robot commented Dec 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r4f4 commented Dec 19, 2022

Uh oh!

r4f4 commented Dec 20, 2022

Uh oh!

barbacbd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Jan 20, 2023

Uh oh!

openshift-ci bot commented Jan 20, 2023

Uh oh!

openshift-ci-robot commented Jan 20, 2023

Uh oh!

openshift-ci-robot commented Jan 21, 2023

Uh oh!

openshift-ci-robot commented Jan 21, 2023

Uh oh!

openshift-ci-robot commented Jan 21, 2023

Uh oh!

r4f4 commented Jan 24, 2023

Uh oh!

r4f4 commented Jan 24, 2023

Uh oh!

openshift-ci-robot commented Jan 24, 2023

Uh oh!

openshift-ci-robot commented Jan 24, 2023

Uh oh!

r4f4 commented Jan 25, 2023

Uh oh!

openshift-ci-robot commented Jan 25, 2023

Uh oh!

openshift-ci-robot commented Jan 26, 2023

Uh oh!

r4f4 commented Jan 26, 2023

Uh oh!

openshift-ci-robot commented Jan 26, 2023

Uh oh!

openshift-ci-robot commented Jan 27, 2023

Uh oh!

openshift-ci bot commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jan 27, 2023

Uh oh!

openshift-ci-robot commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

OCPBUGS-4549: destroy: azure: handle `nil` responses from msgraph sdk #6717

OCPBUGS-4549: destroy: azure: handle `nil` responses from msgraph sdk #6717

openshift-ci bot commented Jan 27, 2023 •

edited

Loading