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

[New Resource:] azurerm_databricks_virtual_network_peering #20728

Merged
merged 25 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b8e3272
Update go-azure-sdk v0.20230228.1160358 => v0.20230301.1141943
WodansSon Mar 2, 2023
54c4add
Check-in progress...
WodansSon Mar 2, 2023
9bfdd46
Check-in progress...
WodansSon Mar 2, 2023
37dd29f
pull changes from main
WodansSon Mar 2, 2023
7dd41e2
Check-in progress...
WodansSon Mar 2, 2023
d25ed01
Checking-in progress...
WodansSon Mar 4, 2023
a7edac1
terrafmt docs...
WodansSon Mar 4, 2023
cffd531
update test case to use sku parameter...
WodansSon Mar 4, 2023
7ddaec2
Check-in progress...
WodansSon Mar 4, 2023
99fff59
Remove test case...
WodansSon Mar 4, 2023
05838f0
Check-in progress...
WodansSon Mar 4, 2023
c2cef32
match field names with network resource
WodansSon Mar 6, 2023
4d9c7e4
add comment about delete SDK issue...
WodansSon Mar 7, 2023
93d5d18
Per PR review moved resource mutex from sync to the terraform interna…
WodansSon Mar 15, 2023
361abde
Checking-in PR changes progress...
WodansSon Mar 18, 2023
51dd98a
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
WodansSon Mar 18, 2023
b833626
go mod vendor
WodansSon Mar 18, 2023
42dab33
Update client name to match SDK...
WodansSon Mar 18, 2023
ad7868d
Check-in progress...
WodansSon Mar 20, 2023
4073635
Remove workaround for SDK issue...
WodansSon Mar 20, 2023
390cbf1
Update delete func to be just DeleteThenPoll...
WodansSon Mar 22, 2023
ff03612
Address additional PR comments...
WodansSon Mar 22, 2023
d5a2ee6
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
WodansSon Mar 22, 2023
31efc4d
Add test workaround for AzSecPack...
WodansSon Mar 22, 2023
c42e391
Remove parse directory...
WodansSon Mar 22, 2023
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
5 changes: 5 additions & 0 deletions internal/services/databricks/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,28 @@ package client

import (
"github.com/hashicorp/go-azure-sdk/resource-manager/databricks/2022-04-01-preview/accessconnector"
"github.com/hashicorp/go-azure-sdk/resource-manager/databricks/2023-02-01/vnetpeering"
"github.com/hashicorp/go-azure-sdk/resource-manager/databricks/2023-02-01/workspaces"
"github.com/hashicorp/terraform-provider-azurerm/internal/common"
)

type Client struct {
AccessConnectorClient *accessconnector.AccessConnectorClient
WorkspacesClient *workspaces.WorkspacesClient
VnetPeeringsClient *vnetpeering.VNetPeeringClient
}

func NewClient(o *common.ClientOptions) *Client {
AccessConnectorClient := accessconnector.NewAccessConnectorClientWithBaseURI(o.ResourceManagerEndpoint)
o.ConfigureClient(&AccessConnectorClient.Client, o.ResourceManagerAuthorizer)
WorkspacesClient := workspaces.NewWorkspacesClientWithBaseURI(o.ResourceManagerEndpoint)
o.ConfigureClient(&WorkspacesClient.Client, o.ResourceManagerAuthorizer)
VnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.ResourceManagerEndpoint)
o.ConfigureClient(&VnetPeeringsClient.Client, o.ResourceManagerAuthorizer)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI #21004 introduces the new base layer, so this'll want to become:

Suggested change
VnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.ResourceManagerEndpoint)
o.ConfigureClient(&VnetPeeringsClient.Client, o.ResourceManagerAuthorizer)
vnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.Environment.ResourceManager)
if err != nil {
return nil, fmt.Errorf("building VNetPeering client: %+v", err)
}
o.Configure(vnetPeeringsClient.Client, o.Authorizers.ResourceManager)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


return &Client{
AccessConnectorClient: &AccessConnectorClient,
WorkspacesClient: &WorkspacesClient,
VnetPeeringsClient: &VnetPeeringsClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI #21004 introduces the new base layer, so this'll want to become:

Suggested change
VnetPeeringsClient: &VnetPeeringsClient,
VnetPeeringsClient: vnetPeeringsClient,

(I'm intentionally not touching AccessConnectorClient and WorkspacesClient above to avoid a merge conflict, fwiw - those are changed as a part of #21004)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,326 @@
package databricks

import (
"fmt"
"log"
"strings"
"time"

"github.com/hashicorp/go-azure-helpers/lang/response"
"github.com/hashicorp/go-azure-helpers/resourcemanager/commonschema"
"github.com/hashicorp/go-azure-sdk/resource-manager/databricks/2023-02-01/vnetpeering"
"github.com/hashicorp/terraform-provider-azurerm/helpers/tf"
"github.com/hashicorp/terraform-provider-azurerm/internal/clients"
"github.com/hashicorp/terraform-provider-azurerm/internal/locks"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/databricks/validate"
networkValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/network/validate"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/timeouts"
"github.com/hashicorp/terraform-provider-azurerm/utils"
)

// peerMutex is used to prevent multiple Peering resources being created, updated
// or deleted at the same time
const resourceLock string = "databricks.VnetPeerings"
Copy link
Contributor

Choose a reason for hiding this comment

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

every other lock in the provider uses the resource type here, can we update this to match:

Suggested change
const resourceLock string = "databricks.VnetPeerings"
const databricksVnetPeeringsResourceType string = "azurerm_databricks_virtual_network_peering"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


var peerMutex = locks.NewMutexKV()
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use locks.ByName(id.Name, databricksVnetPeeringsResourceType) here instead, we shouldn't be newing up a new lock here:

Suggested change
var peerMutex = locks.NewMutexKV()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


func resourceDatabricksVirtualNetworkPeering() *pluginsdk.Resource {
return &pluginsdk.Resource{
Create: resourceDatabricksVirtualNetworkPeeringCreateUpdate,
Read: resourceDatabricksVirtualNetworkPeeringRead,
Update: resourceDatabricksVirtualNetworkPeeringCreateUpdate,
Copy link
Contributor

Choose a reason for hiding this comment

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

since there's differences in the codepaths (below) can we split the Create and Update method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Delete: resourceDatabricksVirtualNetworkPeeringDelete,
Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error {
_, err := vnetpeering.ParseVirtualNetworkPeeringID(id)
return err
}),

Timeouts: &pluginsdk.ResourceTimeout{
Create: pluginsdk.DefaultTimeout(30 * time.Minute),
Read: pluginsdk.DefaultTimeout(5 * time.Minute),
Update: pluginsdk.DefaultTimeout(30 * time.Minute),
Delete: pluginsdk.DefaultTimeout(30 * time.Minute),
},

Schema: map[string]*pluginsdk.Schema{
"name": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validate.DatabricksVirtualNetworkPeeringsName,
},

"resource_group_name": commonschema.ResourceGroupName(),

"workspace_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: vnetpeering.ValidateWorkspaceID,
},

"remote_address_space_prefixes": {
Type: pluginsdk.TypeList,
Required: true,
ForceNew: true,

Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
ValidateFunc: validate.CIDR,
},
},

"remote_virtual_network_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: networkValidate.VirtualNetworkID,
},

"address_space_prefixes": {
Type: pluginsdk.TypeList,
Computed: true,

Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
},
},

"virtual_network_id": {
Type: pluginsdk.TypeString,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be computed:

Suggested change
Computed: true,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is created by the RP and can only ever be one value which is determined by the API. What else should this be, optional Computed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Optional: true,
ValidateFunc: networkValidate.VirtualNetworkID,
},

"allow_virtual_network_access": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: true,
},

"allow_forwarded_traffic": {
Type: pluginsdk.TypeBool,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be computed and should probably have a default?

Suggested change
Computed: true,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Optional: true,
},

"allow_gateway_transit": {
Type: pluginsdk.TypeBool,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be computed and should probably have a default?

Suggested change
Computed: true,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Optional: true,
},

"use_remote_gateways": {
Type: pluginsdk.TypeBool,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be computed and should probably have a default?

Suggested change
Computed: true,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Optional: true,
},
},
}
}

func resourceDatabricksVirtualNetworkPeeringCreateUpdate(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).DataBricks.VnetPeeringsClient
subscriptionId := meta.(*clients.Client).Account.SubscriptionId
ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d)
defer cancel()

log.Printf("[INFO] preparing arguments for Azure ARM databricks virtual network peering creation.")
var id vnetpeering.VirtualNetworkPeeringId

if d.IsNewResource() {
// I need to include the workspace ID in the properties because I need the name
// of the workspace to create the peerings ID
workspaceId, err := vnetpeering.ParseWorkspaceID(d.Get("workspace_id").(string))
if err != nil {
return fmt.Errorf("unable to parse 'workspace_id': %+v", err)
}

id = vnetpeering.NewVirtualNetworkPeeringID(subscriptionId, d.Get("resource_group_name").(string), workspaceId.WorkspaceName, d.Get("name").(string))

existing, err := client.Get(ctx, id)
if err != nil {
if !response.WasNotFound(existing.HttpResponse) {
return fmt.Errorf("checking for presence of existing Databricks %s: %s", id, err)
}
}

if !response.WasNotFound(existing.HttpResponse) {
return tf.ImportAsExistsError("azurerm_databricks_virtual_network_peering", id.ID())
}
} else {
// For import and update, you need to parse the id else you will have an empty subscription,
// resource group name and workspace name in the PUT call causing a 400 Bad Request...
rawId, err := vnetpeering.ParseVirtualNetworkPeeringID(d.Id())
if err != nil {
return fmt.Errorf("unable to parse 'id': %+v", err)
}

id = *rawId
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) we should be splitting the Create and Update method here (for all new resources) - since these are different codepaths the Update should be using d.HasChanges("foo") to support users using ignore_changes on fields

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


peer := vnetpeering.VirtualNetworkPeering{
Name: &id.VirtualNetworkPeeringName,
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not valid to set the Name field per the ARM Spec (some APIs reject it, some ignore it) - so we can remove this:

Suggested change
Name: &id.VirtualNetworkPeeringName,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, this is required for the Databricks variant of the Network peering resource?

Properties: expandDatabricksVirtualNetworkPeeringProperties(d),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we inline this, since Update will need to conditionally patch these

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}

// The RP always creates the same vNet ID for the Databricks internal vNet in the below format:
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet'
fmtString := "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/workers-vnet"
virtualNetworkId := fmt.Sprintf(fmtString, id.SubscriptionId, id.ResourceGroupName)
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't be hard-coding Resource ID strings like this - we can use the Resource ID types to generate this without hard-coding the string here:

Suggested change
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format:
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet'
fmtString := "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/workers-vnet"
virtualNetworkId := fmt.Sprintf(fmtString, id.SubscriptionId, id.ResourceGroupName)
virtualNetworkId := networkParse.NewVirtualNetworkID(id.SubscriptionId, id.ResourceGroupName, "workers-vnet").ID()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


peer.Properties.DatabricksVirtualNetwork = &vnetpeering.VirtualNetworkPeeringPropertiesFormatDatabricksVirtualNetwork{
Id: utils.String(virtualNetworkId),
}

peerMutex.Lock(resourceLock)
defer peerMutex.Unlock(resourceLock)
Copy link
Contributor

Choose a reason for hiding this comment

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

this lock should be at the top of the create function, else the RequiresImport check won't be effective?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


if err := pluginsdk.Retry(300*time.Second, retryDatabricksVnetPeeringsClientCreateUpdate(d, id, peer, meta)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. Is this logic needed? The CreateOrUpdate method is a Future, so it should be handling this in the API?
  2. Whilst I appreciate this is a copy of the existing azurerm_virtual_network_peering resource (which is, unbeknown to me still using a hard-coded timeout) - we shouldn't be using a hard-coded timeout like this, instead we should use the timeout from the context to allow users to override this.

If this is still needed, can we instead update this to use a WaitForState func:

Suggested change
if err := pluginsdk.Retry(300*time.Second, retryDatabricksVnetPeeringsClientCreateUpdate(d, id, peer, meta)); err != nil {
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{},
Target: []string{"Created"},
Refresh: func() (interface{}, string, error) {
resp, err := client.CreateOrUpdate(ctx, *id, peer)
if err != nil {
return resp, "Failed" fmt.Errorf("deleting %s: %+v", id, err)
}
if err := resp.Poller.PollUntilDone(ctx); err != nil {
return resp, "Failed" fmt.Errorf("waiting for the creation of %s: %+v", id, err)
}
return res, "Created", nil
},
MinTimeout: 15 * time.Second,
Timeout: time.Until(deadline)
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("creating %s: %+v", id, err)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

return err
}

d.SetId(id.ID())

return resourceDatabricksVirtualNetworkPeeringRead(d, meta)
}

func resourceDatabricksVirtualNetworkPeeringRead(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).DataBricks.VnetPeeringsClient
ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d)
defer cancel()

id, err := vnetpeering.ParseVirtualNetworkPeeringID(d.Id())
if err != nil {
return err
}

// for the import scenario, once the peering is created I can generate the workspace ID from the peering ID
// NOTE: this may not be right because I don't know if it is valid to create a peering with a
// workspace across subscription boundaries?
workspaceId := vnetpeering.NewWorkspaceID(id.SubscriptionId, id.ResourceGroupName, id.WorkspaceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

(as below) this resource is nested under the workspace (per the ARM ID), so it can't be in a different subscription:

Suggested change
// for the import scenario, once the peering is created I can generate the workspace ID from the peering ID
// NOTE: this may not be right because I don't know if it is valid to create a peering with a
// workspace across subscription boundaries?
workspaceId := vnetpeering.NewWorkspaceID(id.SubscriptionId, id.ResourceGroupName, id.WorkspaceName)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


resp, err := client.Get(ctx, *id)
if err != nil {
if response.WasNotFound(resp.HttpResponse) {
d.SetId("")
return nil
}
return fmt.Errorf("making Read request on Databricks %s: %+v", *id, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw other resources use:

Suggested change
return fmt.Errorf("making Read request on Databricks %s: %+v", *id, err)
return fmt.Errorf("retrieving %s: %+v", *id, err)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}

d.Set("resource_group_name", id.ResourceGroupName)
d.Set("name", id.VirtualNetworkPeeringName)
d.Set("workspace_id", workspaceId.ID())
Copy link
Contributor

Choose a reason for hiding this comment

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

since this resource is nested under the Workspace ID, it has to exist in the same subscription:

Suggested change
d.Set("workspace_id", workspaceId.ID())
d.Set("workspace_id", vnetpeering.NewWorkspaceID(id.SubscriptionId, id.ResourceGroupName, id.WorkspaceName).ID())

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


if model := resp.Model; model != nil {
d.Set("allow_virtual_network_access", model.Properties.AllowVirtualNetworkAccess)
d.Set("allow_forwarded_traffic", model.Properties.AllowForwardedTraffic)
d.Set("allow_gateway_transit", model.Properties.AllowGatewayTransit)
d.Set("use_remote_gateways", model.Properties.UseRemoteGateways)

if model.Properties.DatabricksAddressSpace != nil && model.Properties.DatabricksAddressSpace.AddressPrefixes != nil {
d.Set("address_space_prefixes", model.Properties.DatabricksAddressSpace.AddressPrefixes)
}

if model.Properties.RemoteAddressSpace != nil && model.Properties.RemoteAddressSpace.AddressPrefixes != nil {
d.Set("remote_address_space_prefixes", model.Properties.RemoteAddressSpace.AddressPrefixes)
}

if model.Properties.DatabricksVirtualNetwork != nil {
if databricksNetwork := model.Properties.DatabricksVirtualNetwork.Id; databricksNetwork != nil {
d.Set("virtual_network_id", databricksNetwork)
}
}

if remoteNetwork := model.Properties.RemoteVirtualNetwork.Id; remoteNetwork != nil {
d.Set("remote_virtual_network_id", remoteNetwork)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we should always be setting a value into the state:

Suggested change
if model.Properties.DatabricksAddressSpace != nil && model.Properties.DatabricksAddressSpace.AddressPrefixes != nil {
d.Set("address_space_prefixes", model.Properties.DatabricksAddressSpace.AddressPrefixes)
}
if model.Properties.RemoteAddressSpace != nil && model.Properties.RemoteAddressSpace.AddressPrefixes != nil {
d.Set("remote_address_space_prefixes", model.Properties.RemoteAddressSpace.AddressPrefixes)
}
if model.Properties.DatabricksVirtualNetwork != nil {
if databricksNetwork := model.Properties.DatabricksVirtualNetwork.Id; databricksNetwork != nil {
d.Set("virtual_network_id", databricksNetwork)
}
}
if remoteNetwork := model.Properties.RemoteVirtualNetwork.Id; remoteNetwork != nil {
d.Set("remote_virtual_network_id", remoteNetwork)
}
addressSpacePrefixes := make([]string, 0)
if model.Properties.DatabricksAddressSpace != nil && model.Properties.DatabricksAddressSpace.AddressPrefixes != nil {
addressSpacePrefixes = *model.Properties.DatabricksAddressSpace.AddressPrefixes
}
d.Set("address_space_prefixes", addressSpacePrefixes)
remoteAddressSpacePrefixes := make([]string, 0)
if model.Properties.RemoteAddressSpace != nil && model.Properties.RemoteAddressSpace.AddressPrefixes != nil {
remoteAddressSpacePrefixes = *model.Properties.RemoteAddressSpace.AddressPrefixes
}
d.Set("remote_address_space_prefixes", remoteAddressSpacePrefixes)
databricksVirtualNetworkId := ""
if model.Properties.DatabricksVirtualNetwork != nil && model.Properties.DatabricksVirtualNetwork.Id != nil {
databricksVirtualNetworkId = *model.Properties.DatabricksVirtualNetwork.Id
}
d.Set("virtual_network_id", databricksVirtualNetworkId)
remoteVirtualNetworkId := ""
if model.Properties.RemoteVirtualNetwork.Id != nil {
remoteVirtualNetworkId = *model.Properties.RemoteVirtualNetwork.Id
}
d.Set("remote_virtual_network_id", remoteVirtualNetworkId)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}

return nil
}

func resourceDatabricksVirtualNetworkPeeringDelete(d *pluginsdk.ResourceData, meta interface{}) error {
client := meta.(*clients.Client).DataBricks.VnetPeeringsClient
ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d)
defer cancel()

id, err := vnetpeering.ParseVirtualNetworkPeeringID(d.Id())
if err != nil {
return err
}

// check to see if it exist first, this maybe a no op because when you delete a peering
// it auto deletes the corresponding peering in the other resource
existing, err := client.Get(ctx, *id)
if err != nil && response.WasNotFound(existing.HttpResponse) {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Terraform Plan will ensure this is resource is marked as gone if it's gone, so this just hides bugs:

Suggested change
// check to see if it exist first, this maybe a no op because when you delete a peering
// it auto deletes the corresponding peering in the other resource
existing, err := client.Get(ctx, *id)
if err != nil && response.WasNotFound(existing.HttpResponse) {
return nil
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


peerMutex.Lock(resourceLock)
defer peerMutex.Unlock(resourceLock)

// this is a workaround for the SDK issue: https://github.com/hashicorp/go-azure-sdk/issues/351
// and will be cleaned up once the fix has been implemented.
result, err := client.Delete(ctx, *id)
if err != nil && result.HttpResponse != nil {
return fmt.Errorf("deleting Databricks %s: %+v", *id, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is now available as-of #21004 so we can make this:

Suggested change
// this is a workaround for the SDK issue: https://github.com/hashicorp/go-azure-sdk/issues/351
// and will be cleaned up once the fix has been implemented.
result, err := client.Delete(ctx, *id)
if err != nil && result.HttpResponse != nil {
return fmt.Errorf("deleting Databricks %s: %+v", *id, err)
}
if err := client.DeleteThenPoll(ctx, *id); err != nil {
return fmt.Errorf("deleting %s: %+v", *id, err)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.


return nil
}

func expandDatabricksVirtualNetworkPeeringProperties(d *pluginsdk.ResourceData) vnetpeering.VirtualNetworkPeeringPropertiesFormat {
Copy link
Contributor

Choose a reason for hiding this comment

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

(per above) can we remove this method since the Create will need to set all fields and the Update will want to conditionally update these depending on if a change is detected via d.HasChanges

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

allowForwardedTraffic := d.Get("allow_forwarded_traffic").(bool)
allowGatewayTransit := d.Get("allow_gateway_transit").(bool)
allowVirtualNetworkAccess := d.Get("allow_virtual_network_access").(bool)
useRemoteGateways := d.Get("use_remote_gateways").(bool)
remoteVirtualNetwork := d.Get("remote_virtual_network_id").(string)
databricksAddressSpace := utils.ExpandStringSlice(d.Get("address_space_prefixes").([]interface{}))
remoteAddressSpace := utils.ExpandStringSlice(d.Get("remote_address_space_prefixes").([]interface{}))

return vnetpeering.VirtualNetworkPeeringPropertiesFormat{
AllowForwardedTraffic: &allowForwardedTraffic,
AllowGatewayTransit: &allowGatewayTransit,
AllowVirtualNetworkAccess: &allowVirtualNetworkAccess,
DatabricksAddressSpace: &vnetpeering.AddressSpace{
AddressPrefixes: databricksAddressSpace,
},
RemoteAddressSpace: &vnetpeering.AddressSpace{
AddressPrefixes: remoteAddressSpace,
},
RemoteVirtualNetwork: vnetpeering.VirtualNetworkPeeringPropertiesFormatRemoteVirtualNetwork{
Id: utils.String(remoteVirtualNetwork),
},
UseRemoteGateways: &useRemoteGateways,
}
}

func retryDatabricksVnetPeeringsClientCreateUpdate(d *pluginsdk.ResourceData, id vnetpeering.VirtualNetworkPeeringId, peer vnetpeering.VirtualNetworkPeering, meta interface{}) func() *pluginsdk.RetryError {
return func() *pluginsdk.RetryError {
vnetPeeringsClient := meta.(*clients.Client).DataBricks.VnetPeeringsClient
ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d)
defer cancel()

future, err := vnetPeeringsClient.CreateOrUpdate(ctx, id, peer)
if err != nil {
if utils.ResponseErrorIsRetryable(err) {
return pluginsdk.RetryableError(err)
} else if future.HttpResponse != nil && future.HttpResponse.StatusCode == 400 && strings.Contains(err.Error(), "ReferencedResourceNotProvisioned") {
// Resource is not yet ready, this may be the case if the Vnet was just created or another peering was just initiated.
return pluginsdk.RetryableError(err)
}

return pluginsdk.NonRetryableError(err)
}

if err = future.Poller.PollUntilDone(); err != nil {
return pluginsdk.NonRetryableError(err)
}

return nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

per above, is this still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. I have removed that func. Fixed.

Loading