Skip to content
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.

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

Bug: Use of log.Fatalf in resourcemanager packages #432

Closed
frodopwns opened this issue Oct 30, 2019 · 4 comments · Fixed by #824
Closed

Bug: Use of log.Fatalf in resourcemanager packages #432

frodopwns opened this issue Oct 30, 2019 · 4 comments · Fixed by #824
Assignees
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)

Comments

@frodopwns
Copy link
Contributor

Describe the bug
Some clients for azure resources are exiting due to log.Fatalf calls which runs os.Exit(1). This causes the service to exit and leads to unreachable code.

eg.
https://github.com/Azure/azure-service-operator/blob/master/pkg/resourcemanager/storages/storage.go#L73

To Reproduce
manually create a storage account, then create that same account using the operator
operator will exit with

2019/10/30 17:15:29 storage account not available: AlreadyExists
exit status 1
make: *** [run] Error 1

Expected behavior
Method returns error where it can be logged and the operator can decide to re-queue if necessary

Additional context
This happens in other files and all should probably be removed

10404 ± grep -r "log.Fatal" ./pkg/resourcemanager -l
./pkg/resourcemanager/storages/storage.go
./pkg/resourcemanager/resources/groups.go
./pkg/resourcemanager/resources/resources_test.go
./pkg/resourcemanager/resources/hybrid/groups.go
./pkg/resourcemanager/resourcegroups/resourcegroup.go
./pkg/resourcemanager/keyvaults/keyvault.go
./pkg/resourcemanager/rediscaches/rediscaches.go
./pkg/resourcemanager/cosmosdbs/cosmosdbs.go
@frodopwns frodopwns added bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug) labels Oct 30, 2019
@WilliamMortlMicrosoft WilliamMortlMicrosoft self-assigned this Dec 4, 2019
@WilliamMortlMicrosoft
Copy link
Contributor

leaving the log.fatal statements in test files... makes sense there

@WilliamMortlMicrosoft WilliamMortlMicrosoft changed the title Bug: Use of log.Fatalf in resourcemanager packages Bug: Use of logr and log.Fatalf in resourcemanager packages Mar 12, 2020
@frodopwns frodopwns changed the title Bug: Use of logr and log.Fatalf in resourcemanager packages Bug: Use of log.Fatalf in resourcemanager packages Mar 23, 2020
@frodopwns
Copy link
Contributor Author

replace fatalfs in packages with errors that get returned instead

@buhongw7583c
Copy link
Contributor

@frodopwns, cc @jananivMS , any thought to reproduce the issue on cosmosDB and appInsights?
I tried to create a cosmosDB manually then use the service operator to create another one with same name, no problem. Also tried to use wrong location name, modify the service principal to 'reader' permission, etc and couldn't run into the code 'log.Fatalf("failed to initialize authorizer: %v\n", err)'.

Any thought?

func getCosmosDBClient() documentdb.DatabaseAccountsClient {
cosmosDBClient := documentdb.NewDatabaseAccountsClientWithBaseURI(config.BaseURI(), config.SubscriptionID())
a, err := iam.GetResourceManagementAuthorizer()
if err != nil {
log.Fatalf("failed to initialize authorizer: %v\n", err)
}
cosmosDBClient.Authorizer = a
cosmosDBClient.AddToUserAgent(config.UserAgent())
return cosmosDBClient
}

@jananivMS
Copy link
Contributor

@buhongw7583c I dont think we will be able to repro some of these errors to happen like in this case, we only fail here if getting an authorizer fails. You could follow the same pattern as we use for other clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working high-priority Issues we intend to prioritize (security, outage, blocking bug)
Projects
None yet
4 participants