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

azurerm_network_security_group - support for augmented security rules #781

Merged
merged 1 commit into from
Mar 1, 2018
Merged

azurerm_network_security_group - support for augmented security rules #781

merged 1 commit into from
Mar 1, 2018

Conversation

svanharmelen
Copy link

@svanharmelen svanharmelen commented Feb 1, 2018

This support was already added to the azurerm_network_security_rule resource in PR #692, but it wasn’t added to the azurerm_network_security_group resource at that time.

It did need a bit of additional logic to make it work properly in the azurerm_network_security_group resource as well, but it all worked out nicely.

All related unit- and acceptance tests were successfully run before creating this PR.

This support was already added to the `azurerm_network_security_rule` resource in PR #692, but it wasn’t added to the `azurerm_network_security_group` resource at that time.

It did needed a bit of additional logic to make it work properly in the `azurerm_network_security_group` resource as well, but it all worked out nicely.

All related unit- and acceptance tests were successfully run before creating this PR.
properties := network.SecurityRulePropertiesFormat{
SourcePortRange: &source_port_range,
DestinationPortRange: &destination_port_range,
SourceAddressPrefix: &source_address_prefix,

Choose a reason for hiding this comment

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

SourceAddressPrefixes should be expanded too, if not during an update of an existing nsg we will drop enhanced rules.

Copy link
Author

Choose a reason for hiding this comment

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

Could you explain what you mean exactly? I'm afraid I don't understand what you mean by SourceAddressPrefixes should be expanded too...

Copy link
Author

Choose a reason for hiding this comment

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

Maybe you missed that I check and set those fields (if needed) as well? See below starting from line 288...

Choose a reason for hiding this comment

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

I'm sorry, I made a mistake during my test, your PR fix that problem with #692... And it works as described 👍

Copy link
Author

Choose a reason for hiding this comment

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

Check, cool...

@tombuildsstuff tombuildsstuff added this to the 1.1.3 milestone Feb 16, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 1.1.3, 1.1.4 Feb 28, 2018
@tombuildsstuff
Copy link
Contributor

Tests pass:

screen shot 2018-02-28 at 17 53 01

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 @svanharmelen

Thanks for this PR - apologies for the delay in reviewing it! I've taken a look through and this LGTM - I've run the tests and they pass too :)

Thanks!

@tombuildsstuff tombuildsstuff modified the milestones: 1.2.0, 1.1.3 Mar 1, 2018
@tombuildsstuff tombuildsstuff changed the title Add support for augmented security rules azurerm_network_security_group - support for augmented security rules Mar 1, 2018
@tombuildsstuff tombuildsstuff merged commit 0db754b into hashicorp:master Mar 1, 2018
tombuildsstuff added a commit that referenced this pull request Mar 1, 2018
@svanharmelen
Copy link
Author

Thanks! And no problem...

@tombuildsstuff
Copy link
Contributor

👋 hey @svanharmelen

Just to let you know that this has just been released in v1.2.0 of the AzureRM Provider - full details of what's included are available here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/v1.2.0/CHANGELOG.md#120-march-02-2018

Thanks!

@svanharmelen
Copy link
Author

Cool... Thanks for the heads up!

@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
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.

3 participants