Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Jan 27, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions pkg/destroy/azure/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,14 +578,18 @@ func extractODataError(err error) error {
}

func deleteApplicationRegistrations(ctx context.Context, graphClient *msgraphsdk.GraphServiceClient, logger logrus.FieldLogger, infraID string) error {
var errorList []error

tag := fmt.Sprintf("kubernetes.io_cluster.%s=owned", infraID)
servicePrincipals, err := getServicePrincipalsByTag(ctx, graphClient, tag, infraID)
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
Copy link
Contributor

@sadasu sadasu Jan 4, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@r4f4 r4f4 Jan 4, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@r4f4 r4f4 Jan 4, 2023

Choose a reason for hiding this comment

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

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.

if servicePrincipals == nil {
logger.Debug("Empty response from API when listing Service Principals by tag")
return nil
}

var errorList []error
for _, sp := range servicePrincipals {
appID := *sp.GetAppId()
logger := logger.WithField("appID", appID)
Expand All @@ -602,6 +606,11 @@ func deleteApplicationRegistrations(ctx context.Context, graphClient *msgraphsdk
errorList = append(errorList, err)
continue
}
// msgraphsdk can return a `nil` response even if no errors occurred
if resp == nil {
logger.Debugf("Empty response getting Application from Service Principal %s", *sp.GetDisplayName())
continue
}
apps := resp.GetValue()
Copy link
Contributor

@sadasu sadasu Jan 4, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@r4f4 r4f4 Jan 4, 2023

Choose a reason for hiding this comment

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

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.

if len(apps) != 1 {
err = fmt.Errorf("should have received only a single matching AppID, received %d instead", len(apps))
Expand All @@ -626,7 +635,7 @@ func getServicePrincipalsByTag(ctx context.Context, graphClient *msgraphsdk.Grap
},
}
resp, err := graphClient.ServicePrincipals().Get(ctx, &listQuery)
if err != nil {
if err != nil || resp == nil {
return nil, err
}
return resp.GetValue(), nil
Expand Down