-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_network_manager
- support management group register provider
#19853
azurerm_network_manager
- support management group register provider
#19853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @teowa
Thanks for this PR.
Unfortunately this PR contains a number of design issues which are going to need to be addressed before we can proceed here - namely registering Resource Providers at the Management Group level generally requires elevated permissions which may not be available, in addition the approach used here is problematic (for example, what happens if the RP isn't registered in 10s [since this can take up to 15 minutes] - what happens if we attempt to register multiple in parallel etc).
Once those issues are addressed we can take another look here, but I think it'd be helpful to have some additional context on what the problem is that we're looking to solve here.
Thanks!
if len(state.Scope[0].ManagementGroups) > 0 { | ||
metadata.Logger.Info("try to register 'Microsoft.Network' provider to the Management Group scope") | ||
resourceProvidersClient := metadata.Client.Resource.ResourceProvidersClient | ||
for _, group := range state.Scope[0].ManagementGroups { | ||
groupId, err := managementGroupParse.ManagementGroupID(group) | ||
if err != nil { | ||
return err | ||
} | ||
_, err = resourceProvidersClient.RegisterAtManagementGroupScope(ctx, "Microsoft.Network", groupId.Name) | ||
if err != nil { | ||
return fmt.Errorf("registering the 'Microsoft.Network' provider with Management Group %s: %+v", groupId, err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give some more context on the issue here? Terraform shouldn't be silently registering resource providers at management group scopes which generally require extra-permissions
} | ||
} | ||
// https://github.com/Azure/azure-rest-api-specs/issues/22041 | ||
time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resource Provider registration takes up to 15 minutes, and unfortunately time.Sleep
's aren't something we can ship/rely on - as such I think we can remove all of this RP auto-registration logic here?
Provisioning network manager using MG as scope need the 'Microsoft.Network' provider be registered at MG level, and using MG to provision network manager can parallelize the acc tests, see #19593 (comment). But seems we have to close this PR due to lack of related method, Azure/azure-rest-api-specs#22041. A good implementation should be a independent resource like |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Microsoft.Network
provider at Management Group scope if the network manager use MG as scope.