-
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
Container Service Api server authorized ip ranges #3262
Container Service Api server authorized ip ranges #3262
Conversation
# Conflicts: # azurerm/resource_arm_kubernetes_cluster.go
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 @fraserdarwent,
Thank you for the PR, there are a couple things that stand out:
- you have updated the container test, but only added the property to AKS, did you mean to add it to both?
- you are not reading the property back in on read, could we add that in?
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.
hey @fraserdarwent
Thanks for pushing those changes - this now LGTM 👍
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.
hey @fraserdarwent
Running the tests for those I noticed this should be a TypeSet
rather than a TypeMap
(since a Map is a Dictionary/key-value pair); as such I'm going to push these changes; I hope you don't mind!
Thanks!
dismissing since changes have been pushed
… into api-server-authorized-ip-ranges
This is ready to merge, however we are having a hard time getting the tests to pass:
However checking from cloudshell:
Is there anything else we need to do on our end to enable the feature? |
In the az cli you need to propagate the changes after registering the extension. Probably in PS after the command you already ran: you'd need to run: |
Thanks @jlpedrosa! that did the trick 🙂 Now we are blocked on a real test failures here:
|
@katbyte |
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.
Thanks for the updates @fraserdarwent!
Aside from one minor comment about adding to the docs the LGTM 👍
@@ -87,6 +87,10 @@ The following arguments are supported: | |||
|
|||
* `role_based_access_control` - (Optional) A `role_based_access_control` block. Changing this forces a new resource to be created. | |||
|
|||
* `api_server_authorized_ip_ranges` - (Optional) The IP ranges to whitelist for incoming traffic to the masters. | |||
|
|||
-> **Note:** `api_server_authorized_ip_ranges` Is currently in Preview on an opt-in basis. |
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.
Just a thought, should we detail how to opt in here? something like
-> **Note:** `api_server_authorized_ip_ranges` Is currently in Preview on an opt-in basis. You can enable this by running the following in Azure Cloudshell:
Register-AzureRmProviderFeature -FeatureName APIServerSecurityPreview -ProviderNamespace Microsoft.ContainerService
Get-AzureRmProviderFeature # Wait for feature to become registered and then
Register-AzureRmResourceProvider -ProviderNamespace Microsoft.ContainerService
This has been released in version 1.28.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.28.0"
}
# ... other configuration ... |
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! |
Updated the container service SDK version, made necessary changes for compatibility and added additional field from SDK
Fixes #3262