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_application_load_balancer_subnet_association #23628

Merged
merged 10 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
package servicenetworking
Copy link
Collaborator

Choose a reason for hiding this comment

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

This filename needs to be updated to reflect the new name?


import (
"context"
"fmt"
"regexp"
"time"

"github.com/hashicorp/go-azure-helpers/lang/pointer"
"github.com/hashicorp/go-azure-helpers/lang/response"
"github.com/hashicorp/go-azure-helpers/resourcemanager/commonids"
"github.com/hashicorp/go-azure-helpers/resourcemanager/commonschema"
"github.com/hashicorp/go-azure-helpers/resourcemanager/location"
"github.com/hashicorp/go-azure-sdk/resource-manager/servicenetworking/2023-05-01-preview/associationsinterface"
"github.com/hashicorp/go-azure-sdk/resource-manager/servicenetworking/2023-05-01-preview/trafficcontrollerinterface"
"github.com/hashicorp/terraform-provider-azurerm/internal/sdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation"
)

type AssociationResource struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename it something like ApplicationLoadBalancerAssociationResource or even ApplicationLoadBalancerAssociationSubnetResource to be more specific?


type AssociationModel struct {
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 is specific to the Resource, can we make this:

Suggested change
type AssociationModel struct {
type AssociationResourceModel struct {

jackofallops marked this conversation as resolved.
Show resolved Hide resolved
Name string `tfschema:"name"`
ApplicationLoadBalancerId string `tfschema:"application_load_balancer_id"`
SubnetId string `tfschema:"subnet_id"`
Tags map[string]string `tfschema:"tags"`
}

var _ sdk.ResourceWithUpdate = AssociationResource{}

func (t AssociationResource) Arguments() map[string]*pluginsdk.Schema {
return map[string]*pluginsdk.Schema{
"name": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,62}[a-zA-Z0-9]$`), "the name must begin with a letter or number, end with a letter, number or underscore, and may contain only letters, numbers, underscores, periods, or hyphens. The value must be 1-64 characters long."),
jackofallops marked this conversation as resolved.
Show resolved Hide resolved
magodo marked this conversation as resolved.
Show resolved Hide resolved
},

"application_load_balancer_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: associationsinterface.ValidateTrafficControllerID,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the commonschema package has a ResourceIDReference schema property for this purpose:

Suggested change
"application_load_balancer_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: associationsinterface.ValidateTrafficControllerID,
},
"application_load_balancer_id": commonschema.ResourceIDReferenceForceNew(associationsinterface.TrafficControllerId{}),


"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: commonids.ValidateSubnetID,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

Suggested change
"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: commonids.ValidateSubnetID,
},
"subnet_id": commonschema.ResourceIDReferenceForceNew(commonids.SubnetId{}),


"tags": commonschema.Tags(),
}
}

func (t AssociationResource) Attributes() map[string]*pluginsdk.Schema {
return map[string]*pluginsdk.Schema{}
}

func (t AssociationResource) ModelObject() interface{} {
return &AssociationModel{}
}

func (t AssociationResource) ResourceType() string {
return "azurerm_application_load_balancer_association"
}

func (t AssociationResource) IDValidationFunc() pluginsdk.SchemaValidateFunc {
return associationsinterface.ValidateAssociationID
}
func (t AssociationResource) Create() sdk.ResourceFunc {
jackofallops marked this conversation as resolved.
Show resolved Hide resolved
return sdk.ResourceFunc{
Timeout: 30 * time.Minute,
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
trafficControllerClient := metadata.Client.ServiceNetworking.TrafficControllerInterface
client := metadata.Client.ServiceNetworking.AssociationsInterface

var config AssociationModel
if err := metadata.Decode(&config); err != nil {
return fmt.Errorf("decoding %v", err)
}

albId, err := trafficcontrollerinterface.ParseTrafficControllerID(config.ApplicationLoadBalancerId)
if err != nil {
return err
}

id := associationsinterface.NewAssociationID(albId.SubscriptionId, albId.ResourceGroupName, albId.TrafficControllerName, config.Name)
existing, err := client.Get(ctx, id)
if err != nil && !response.WasNotFound(existing.HttpResponse) {
return fmt.Errorf("checking for presence of exisiting %s: %+v", id, err)
}

if !response.WasNotFound(existing.HttpResponse) {
return metadata.ResourceRequiresImport(t.ResourceType(), id)
}

controller, err := trafficControllerClient.Get(ctx, *albId)
if err != nil {
return fmt.Errorf("retrieving %s: %+v", *albId, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("retrieving %s: %+v", *albId, err)
return fmt.Errorf("retrieving parent %s: %+v", *albId, err)

}
Copy link
Contributor

Choose a reason for hiding this comment

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

here we're assuming we're getting a model back:

Suggested change
}
}
if controller.Model == nil {
return fmt.Errorf("retrieving parent %s: model was nil", *albId)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ziyeqf could you address this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have already updated code per this comment?


association := associationsinterface.Association{
Properties: &associationsinterface.AssociationProperties{
Subnet: &associationsinterface.AssociationSubnet{
Id: config.SubnetId,
},
AssociationType: associationsinterface.AssociationTypeSubnets,
katbyte marked this conversation as resolved.
Show resolved Hide resolved
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
},
Tags: tags.FromTypedObject(config.Tags),
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cant use it because it returns map[string]*string, while sdk needs *map[string]string

Copy link
Contributor

Choose a reason for hiding this comment

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

There's two imports for tags - the legacy one within this provider (./internal/tags) and the one within go-azure-helpers (github.com/hashicorp/go-azure-helpers/resourcemanager/tags) - updating the reference to use go-azure-helpers should solve that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

please follow toms advice here @ziyeqf

Copy link
Contributor Author

@ziyeqf ziyeqf Nov 10, 2023

Choose a reason for hiding this comment

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

I'm already using "github.com/hashicorp/go-azure-helpers/resourcemanager/tags" now, is it correct?

}

if controller.Model != nil {
association.Location = location.Normalize(controller.Model.Location)
jackofallops marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the location here required as it typically should be the same as its parent resource? If that is the case, then we can fail early (prior to sending the requset) with telling the user that the required location from the parent model is not retrieved.

}

if len(config.Tags) > 0 {
association.Tags = &config.Tags
}
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 tags here, else you can't remove them?


if err := client.CreateOrUpdateThenPoll(ctx, id, association); err != nil {
return fmt.Errorf("creating %s: %+v", id, err)
}

metadata.SetID(id)
return nil
},
}
}

func (t AssociationResource) Read() sdk.ResourceFunc {
return sdk.ResourceFunc{
Timeout: 5 * time.Minute,
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
client := metadata.Client.ServiceNetworking.AssociationsInterface

id, err := associationsinterface.ParseAssociationID(metadata.ResourceData.Id())
if err != nil {
return err
}

resp, err := client.Get(ctx, *id)
if err != nil {
if response.WasNotFound(resp.HttpResponse) {
return metadata.MarkAsGone(id)
}
return fmt.Errorf("retreiving %s: %v", id.ID(), err)
}

trafficControllerId := associationsinterface.NewTrafficControllerID(id.SubscriptionId, id.ResourceGroupName, id.TrafficControllerName)
state := AssociationModel{
Name: id.AssociationName,
ApplicationLoadBalancerId: trafficControllerId.ID(),
}

if model := resp.Model; model != nil {
state.Tags = pointer.From(model.Tags)

if prop := model.Properties; prop != nil {
if prop.Subnet != nil {
parsedSubnetId, err := commonids.ParseSubnetID(prop.Subnet.Id)
Copy link
Contributor

Choose a reason for hiding this comment

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

When parsing IDs in the Read we need to parse these insensitively:

Suggested change
parsedSubnetId, err := commonids.ParseSubnetID(prop.Subnet.Id)
parsedSubnetId, err := commonids.ParseSubnetIDInsensitively(prop.Subnet.Id)

if err != nil {
return err
}
state.SubnetId = parsedSubnetId.ID()
}
}
}

return metadata.Encode(&state)
},
}
}

func (t AssociationResource) Update() sdk.ResourceFunc {
return sdk.ResourceFunc{
Timeout: 30 * time.Minute,
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
client := metadata.Client.ServiceNetworking.AssociationsInterface

var plan AssociationModel
if err := metadata.Decode(&plan); err != nil {
return fmt.Errorf("decoding %v", err)
}

id, err := associationsinterface.ParseAssociationID(metadata.ResourceData.Id())
if err != nil {
return fmt.Errorf("parsing id %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to wrap this one:

Suggested change
return fmt.Errorf("parsing id %v", err)
return err

}

// Thought `AssociationSubnetUpdate` defined in the SDK contains the `subnetId`, while per testing it can not be updated
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is there an issue on Azure/azure-rest-api-specs to track that?
  2. Is that also true using the CreateOrUpdate method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ziyeqf - have you opened an issue yet? could you add it to this comment? and address the question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the issue link to the code comment.
And yes for CreateOrUpdate it will get AppGwForContainersSubnetAssociationChangeOfSubnetNotAllowed error

associationUpdate := associationsinterface.AssociationUpdate{}

if metadata.ResourceData.HasChange("tags") {
associationUpdate.Tags = &plan.Tags
}

if _, err = client.Update(ctx, *id, associationUpdate); err != nil {
return fmt.Errorf("updating %s: %v", id.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.

(as below)

Suggested change
return fmt.Errorf("updating %s: %v", id.ID(), err)
return fmt.Errorf("updating %s: %v", *id, err)

}

return nil
},
}
}

func (t AssociationResource) Delete() sdk.ResourceFunc {
return sdk.ResourceFunc{
Timeout: 30 * time.Minute,
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
client := metadata.Client.ServiceNetworking.AssociationsInterface

id, err := associationsinterface.ParseAssociationID(metadata.ResourceData.Id())
if err != nil {
return err
}

if err = client.DeleteThenPoll(ctx, *id); err != nil {
return fmt.Errorf("deleting %s: %v", id.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.

this should be using the Resource ID itself, since the .String() methods is then automatically called:

Suggested change
return fmt.Errorf("deleting %s: %v", id.ID(), err)
return fmt.Errorf("deleting %s: %v", *id, err)

(rather than this which outputs the ID, which is less descriptive)

}

return nil
},
}
}
Loading