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_ip_group_cidr #17321

Closed
wants to merge 2 commits into from
Closed

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Jun 21, 2022

This adds a new resource that allows us to manage the CIDRs/IPs in an existing IP Group

Example Usage

resource "azurerm_resource_group" "example" {
  name     = "test-rg"
  location = "West Europe"
}

resource "azurerm_ip_group" "example" {
  name                = "test-ipgroup"
  location            = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
}

resource "azurerm_ip_group_cidr" "example" {
  ip_group_id = azurerm_ip_group.example.id
  cidr        = "10.10.10.0/24"
}

CleanShot 2022-06-21 at 3 59 29

@tiwood
Copy link
Contributor Author

tiwood commented Aug 10, 2022

@katbyte @tombuildsstuff
can some1 take a look?

We need a way to add/remove from CIDR groups programmatically and might need to use a custom built provider if its unclear if this gets merged into the official provider.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @tiwood

Thanks for this PR - apologies for the delayed review here.

I've taken a look through and whilst on the whole this is looking pretty good, we're going to need to fix the azurerm_ip_group resource such that we don't introduce a breaking change to users configurations here - I've left some notes inline about this.

However if we can fix this up then we should be able to take another look 👍

Thanks!

Comment on lines +51 to +53
ConfigMode: pluginsdk.SchemaConfigModeAttr,
Optional: true,
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.

unfortunately this'd be a breaking configuration change, instead we'll need to revert this:

Suggested change
ConfigMode: pluginsdk.SchemaConfigModeAttr,
Optional: true,
Computed: true,
Optional: true,

and instead split the Create and Update methods here such that we retrieve the existing value for cidrs from the API and reuse this during the update, unless the cidrs block has changed, for example:

existing, err := client.Get(...)
if err != nil { .. }

update.Properties.Cidrs = existing.Properties.Cidrs
if d.HasChange("cidrs") {
    update.Properties.Cidrs = expandCidrs(d.Get("cidrs").([]interface{})
}

which'd allow users to make use of ignore_changes to ignore changes to the cidrs block on this resource, and use the separate resource instead - which'd need to be called out in the documentation for both of these resources

katbyte

This comment was marked as off-topic.

@katbyte
Copy link
Collaborator

katbyte commented Jan 30, 2023

Superseded by #17321

@katbyte katbyte closed this Jan 30, 2023
@github-actions
Copy link

github-actions bot commented Mar 2, 2023

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants