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

Container Service Api server authorized ip ranges #3262

Merged

Conversation

fraserdarwent
Copy link
Contributor

@fraserdarwent fraserdarwent commented Apr 16, 2019

Updated the container service SDK version, made necessary changes for compatibility and added additional field from SDK

Fixes #3262

Fraser Darwent added 2 commits April 17, 2019 09:26
@ghost ghost added size/XXL dependencies and removed size/M labels Apr 17, 2019
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 @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?

azurerm/resource_arm_container_service_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
Copy link
Member

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

Thanks for pushing those changes - this now LGTM 👍

Copy link
Member

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

azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
azurerm/resource_arm_kubernetes_cluster.go Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff dismissed stale reviews from katbyte and themself April 18, 2019 09:27

dismissing since changes have been pushed

@katbyte katbyte added this to the v1.28.0 milestone Apr 29, 2019
@katbyte
Copy link
Collaborator

katbyte commented Apr 29, 2019

@fraserdarwent,

This is ready to merge, however we are having a hard time getting the tests to pass:

------- Stdout: -------
=== RUN   TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== PAUSE TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== CONT  TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
--- FAIL: TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges (69.26s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: Error creating/updating Managed Kubernetes Cluster "acctestaks190429172120205848" (Resource Group "acctestRG-190429172120205848"): containerservice.ManagedClustersClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="BadRequest" Message="APIServer Authorized IP Ranges is not allowed since feature \"Microsoft.ContainerService/APIServerSecurityPreview\" is not enabled."

However checking from cloudshell:

PS Azure:\> Get-AzureRmProviderFeature

FeatureName                                  ProviderName               RegistrationState
-----------                                  ------------               -----------------
APIServerSecurityPreview                     Microsoft.ContainerService Registered
...

Is there anything else we need to do on our end to enable the feature?

@jlpedrosa
Copy link
Contributor

jlpedrosa commented May 1, 2019

@fraserdarwent, @katbyte

This is ready to merge, however we are having a hard time getting the tests to pass:

------- Stdout: -------
=== RUN   TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== PAUSE TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== CONT  TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
--- FAIL: TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges (69.26s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: Error creating/updating Managed Kubernetes Cluster "acctestaks190429172120205848" (Resource Group "acctestRG-190429172120205848"): containerservice.ManagedClustersClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="BadRequest" Message="APIServer Authorized IP Ranges is not allowed since feature \"Microsoft.ContainerService/APIServerSecurityPreview\" is not enabled."

However checking from cloudshell:

PS Azure:\> Get-AzureRmProviderFeature

FeatureName                                  ProviderName               RegistrationState
-----------                                  ------------               -----------------
APIServerSecurityPreview                     Microsoft.ContainerService Registered
...

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.
Once the feature 'APIServerSecurityPreview' is registered, invoking 'az provider register -n Microsoft.ContainerService' is required to get the change propagated

Probably in PS after the command you already ran:
Register-AzureRmProviderFeature -FeatureName APIServerSecurityPreview -ProviderNamespace Microsoft.ContainerService

you'd need to run:
'Register-AzureRmResourceProvider -ProviderNamespace Microsoft.ContainerService'

@ghost ghost removed the waiting-response label May 1, 2019
@katbyte
Copy link
Collaborator

katbyte commented May 2, 2019

Thanks @jlpedrosa! that did the trick 🙂

Now we are blocked on a real test failures here:

------- Stdout: -------
=== RUN   TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== PAUSE TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
=== CONT  TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges
--- FAIL: TestAccAzureRMKubernetesCluster_apiServerAuthorizedIPRanges (3594.49s)
    testing.go:568: Step 0 error: Check failed: Check 14/16 error: azurerm_kubernetes_cluster.test: Attribute 'api_server_authorized_ip_ranges' expected to be set

@fraserdarwent

@fraserdarwent
Copy link
Contributor Author

@katbyte
Removed the failing line. I am not sure it was correct of me to check the set itself is set, and I should instead be checking the elements of the set.

@ghost ghost removed the waiting-response label May 5, 2019
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.

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.
Copy link
Collaborator

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

@katbyte katbyte merged commit 002ea7e into hashicorp:master May 6, 2019
katbyte added a commit that referenced this pull request May 6, 2019
@fraserdarwent fraserdarwent deleted the api-server-authorized-ip-ranges branch May 6, 2019 17:57
@ghost
Copy link

ghost commented May 17, 2019

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 ...

@ghost
Copy link

ghost commented Jun 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 Jun 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

4 participants