-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
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
There was a problem hiding this 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?
Default: false, | ||
}, | ||
|
||
"virtual_network_rule": { |
There was a problem hiding this comment.
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}",
]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR updates, LGTM now 💯
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 👉 [email protected]. Thanks! |
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 networkfilters for the account
virtual_network_rule
, which specifies the subnets that are allowedto access the account
Fixes #1342.