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

Add support for vnet rules to CosmosDB accounts. #1961

Merged
merged 2 commits into from
Sep 21, 2018

Conversation

pgavlin
Copy link
Contributor

@pgavlin pgavlin commented Sep 20, 2018

These changes add support for virtual network filters to CosmosDB
accounts. Using these filters requires two new properties:

  • is_virtual_network_filter_enabled, which enables virutal network
    filters for the account
  • virtual_network_rule, which specifies the subnets that are allowed
    to access the account

Fixes #1342.

These changes add support for virtual network filters to CosmosDB
accounts. Using these filters requires two new prooperties:
- `is_virtual_network_filter_enabled`, which enables virutal network
  filters for the account
- `virtual_network_rules`, which specifies the subnets that are allowed
  to access the account
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @pgavlin,

Thank you for this resource, it mostly LGTM outside some missing nil checks. I do wonder if it is worth simplifying the schema as mentioned in my comment. Really all we are after is a set of ID strings. What do you think?

azurerm/data_source_cosmos_db_account.go Outdated Show resolved Hide resolved
Default: false,
},

"virtual_network_rule": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this make more sense as a simple set of strings virtual_network_rule_subnet_ids rather then a set of blocks containing id?

virtual_network_rule_subnet_ids  =[
"${azurerm_subnet.subnet1.id}",
"${azurerm_subnet.subnet2.id}",
]

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 that we should err on the side of leaving this as-is: if the Azure API adds additional fields to these rules in the future, having a resource here rather than a simple ID makes such a change easier to accommodate. This would also align better with a future cosmosdb_account_virtual_network_rule resource as suggested in #1342.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good points 👍

azurerm/resource_arm_cosmos_db_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_cosmos_db_account.go Outdated Show resolved Hide resolved
@ghost ghost added the size/L label Sep 21, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

@pgavlin,

Thank you for the PR updates, LGTM now 💯

@katbyte katbyte merged commit 9b9995b into hashicorp:master Sep 21, 2018
katbyte added a commit that referenced this pull request Sep 21, 2018
@pgavlin pgavlin deleted the virtualNetworkRules branch September 21, 2018 18:04
@ghost
Copy link

ghost commented Mar 6, 2019

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 6, 2019
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.

None yet

2 participants