Support Auto-Approval when adding Synapse Managed Private Endpoints

[owenfarrell]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Summary

This pull request adds support for automating the approval of private endpoints that that are created in a Synapse managed virtual network.

In order to support auto-approval, this pull request includes a number of enhancements for resources in other services, namely:

• Waiting for the provisioning state when creating/updating Synapse firewall rules
• Waiting for the provisioning state when creating/updating Synapse managed private endpoints

Fixes: #11975
Fixes: #13107
Fixes: #13510
Supersedes: #13511

Output from acceptance testing:

$ make acctests SERVICE='synapse' TESTARGS='-run="^TestAccSynapse(FirewallRule|ManagedPrivateEndpoint)_"'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/synapse -run="^TestAccSynapse(FirewallRule|ManagedPrivateEndpoint)_" -timeout 180m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccSynapseFirewallRule_basic
=== PAUSE TestAccSynapseFirewallRule_basic
=== RUN   TestAccSynapseFirewallRule_requiresImport
=== PAUSE TestAccSynapseFirewallRule_requiresImport
=== RUN   TestAccSynapseFirewallRule_update
=== PAUSE TestAccSynapseFirewallRule_update
=== RUN   TestAccSynapseManagedPrivateEndpoint_basic
=== PAUSE TestAccSynapseManagedPrivateEndpoint_basic
=== RUN   TestAccSynapseManagedPrivateEndpoint_requiresImport
=== PAUSE TestAccSynapseManagedPrivateEndpoint_requiresImport
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveCognitive
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveCognitive
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveCosmos
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveCosmos
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveMariaDB
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveMariaDB
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveMySQL
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveMySQL
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApprovePostgreSQL
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApprovePostgreSQL
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApprovePurview
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApprovePurview
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveSQL
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveSQL
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveSearch
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveSearch
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveStorage
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveStorage
=== RUN   TestAccSynapseManagedPrivateEndpoint_autoApproveSynapse
=== PAUSE TestAccSynapseManagedPrivateEndpoint_autoApproveSynapse
=== CONT  TestAccSynapseFirewallRule_basic
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveMySQL
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveMariaDB
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveCosmos
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveCognitive
=== CONT  TestAccSynapseManagedPrivateEndpoint_requiresImport
=== CONT  TestAccSynapseManagedPrivateEndpoint_basic
=== CONT  TestAccSynapseFirewallRule_update
--- PASS: TestAccSynapseFirewallRule_basic (992.66s)
=== CONT  TestAccSynapseFirewallRule_requiresImport
--- PASS: TestAccSynapseManagedPrivateEndpoint_requiresImport (1139.11s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveSearch
--- PASS: TestAccSynapseFirewallRule_update (1148.04s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveSynapse
--- PASS: TestAccSynapseManagedPrivateEndpoint_basic (1529.44s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveStorage
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveCognitive (1584.47s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApprovePurview
--- PASS: TestAccSynapseFirewallRule_requiresImport (671.35s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApproveSQL
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveMariaDB (1860.28s)
=== CONT  TestAccSynapseManagedPrivateEndpoint_autoApprovePostgreSQL
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveMySQL (1983.12s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveCosmos (2297.65s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApprovePurview (1290.12s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveSQL (1321.02s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveSynapse (1912.23s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveSearch (2036.53s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApproveStorage (1905.40s)
--- PASS: TestAccSynapseManagedPrivateEndpoint_autoApprovePostgreSQL (1917.87s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/synapse       3782.831s

[tombuildsstuff]

hey @owenfarrell

Thanks for this PR.

Taking a look though here whilst this looks good there's a couple of errors with the logging which'll mean these are output as the pointer rather than the string value - but if we can fix those up (and add an explicit interval) then this should otherwise be good to go 👍

Thanks!

[owenfarrell]

Thanks @tombuildsstuff! I always get the parsed/new IDs crossed up in logging statements.

I've folded in all of your requested changes and rerun acceptance tests - everything still comes out clean on my end. Let me know if CI tells a different story.

[ms-henglu]

Hi @owenfarrell , thanks for this PR!

But actually when the long running operation finishes, the provision state will become Succeeded at the same time, but the firewall changes may not be propagated to workspace, so error ClientIpAddressNotAuthorized still happens.

Here'are the request traces I captured before.
https://gist.github.com/ms-henglu/5b29285326c4ff6c9bc1c08911ce63c8

[owenfarrell]

@ms-henglu - Thanks for the trace. And big thanks for following up with the service team to confirm what's going on behind the scenes. It's a bummer that ProvisioningState doesn't reflect the state of the Microsoft-managed side of the workspace. That would make this implementation ideal.

@tombuildsstuff - If I'm reading this right, PollInterval overrides MinTimeout. So if we remove PollInterval from the state change configuration and set MinTimeout to our desired "sleep" (1 minute seems sufficient based on @ms-henglu's comment), then this implementation would:

• Wait 1 minute
• Validate that the requested firewall rule actually exists
• Validate that the provisioning state of the firewall rule is Succeeded

And maybe that's a good as we can get for now.

[katbyte]

also @ms-henglu - could we get the service team to fix the provisioning status so it doesn't change to Succeeded until its actually finished?

[ms-henglu]

Hi @katbyte , Yes, I'm working on that. However, there're some technique issues to fix it and it might not be done in near future. 😢

[owenfarrell]

@katbyte - per my comment above, I recognize that this isn't a perfect solution... but I think this kind of implementation would still be in the right direction.

if we remove PollInterval from the state change configuration and set MinTimeout to our desired "sleep" (1 minute seems sufficient based on @ms-henglu's comment), then this implementation would:

• Wait 1 minute
• Validate that the requested firewall rule actually exists
• Validate that the provisioning state of the firewall rule is Succeeded

The net result - compared to the latest release - would still be an improvement. If/when the service team makes changes so that ProvisioningState better suits our needs, that's great. But reintroducing PollInterval so that the refresh function is more aggressive, that's an optimization effort for later.

Thoughts?

[owenfarrell]

@katbyte - I've just pushed up a handful of commits. And I realize I've introduced a ton of scope creep as this now cuts across multiple services. But I at least wanted to illustrate the longer-term goal of resolving #13107.

In the process of introducing auto-approval for Synapse Managed Private Endpoints, I hit all the following bumps in the road (summarized here to avoid the need for anyone to reread previous posts):

• Creating a Synapse Firewall Rule does not wait for the provisioning state to reach Succeeded before completing the create/update process. This causes subsequent operations (like creating an MPE) to fail.

  My proposed solution here, given the limitations in the API that @ms-henglu referenced, is to set the Delay of the state change configuration to 1 minute. If/when the service is updated so that the provisioning state is a more accurate reflection of network access, removing that initial Delay should be easy enough.

• Creating a Synapse Managed Private Endpoint does not wait for the provisioning state to reach Succeeded before completing the create/update process. This forces downstream resources, or a downstream operation in this case, to inject a manual wait step (e.g., time_sleep) as they cannot rely on the resource's create/update process.

• Creating a Search Service does not wait for the provisioning state to reach Succeeded. When creating a Syanpse Managed Private Endpoint that connects to a Search Service, the MPE creation suffers from a false start (i.e., the service can't support a private endpoint connection yet) and times out as a result.

• The sku for Purview accounts is now a lagging indicator of utilization instead of a leading configuration attribute. As a result, this creates constant drift with the state file

I'm happy to split this out for reviewing purposes if that helps. But for the time being I've updated the original post to reflect all of the related issues this PR addresses and a complete list of related acceptance tests.

[alxgda]

Hi! Could we have the same for Azure Data Factory? azurerm_data_factory_managed_private_endpoint has no option to automatically approve the creation of a private endpoint.

[owenfarrell]

@alxgda - I think that's a reasonable ask. The key question I have in my mind is "is this the right approach?"

I proposed a couple of implementation options to #13107. But I haven't seen any feedback from the maintainers (here or there) on a preference. And that's the critical next step.

[owenfarrell]

@katbyte - related to the previous comment, there are two pieces that I really need some guidance on.

1. What's the preferred approach to approving private endpoints - embedding that within the services that create endpoints or independently?
2. Given the API limitations around Synapse firewall provisioning state, can we roll with the initial Delay approach I have here as a workaround in the interim?

[owenfarrell]

Thanks for the ping @tombuildsstuff.

Can you help me set expectations on if/when this PR will get some focus? I know it's assigned to you, but not sure if that's still the plan.

My struggle here is that:

1. I haven't seen any feedback on the approach - if this is totally misguided and never going to see the light of day, I'd like to stop wasting everyone's time. Given the related issues, I think these changes are desirable. I know there is one outstanding item w/r/t feasibility, but that's an Azure service limitation and this PR makes its best effort at working around that.
2. the go-azure-sdk refactor effort is going to cause the size of this PR to explode. I'm of two minds as to whether or not it would make sense to carve this up in to multiple PRs.... splitting out all of the dependency related work doesn't add functionality (and, in fact, adds a bunch of unreachable code to the codebase)... but provides an opportunity for a more targeted review.

Thoughts?

[owenfarrell]

One byproduct of #17625 is that this PR is now blocked by hashicorp/go-azure-sdk#79

[owenfarrell]

Waiting on #17753

[onenessboy]

@owenfarrell is this fix completed ? I tried your suggestion is_manual_connection = false its not working..