Support more properties for azurerm_kubernetes_cluster #5824

neil-yechenwei · 2020-02-20T06:52:02Z

fixes #5646

katbyte

Hi @neil-yechenwei,

Could we get a test that sets these new properties? thanks

neil-yechenwei · 2020-03-04T09:40:43Z

@katbyte , thanks for you comments. I added test case to support new properties.

katbyte

Thanks for adding the test @neil-yechenwei, i've given this a proper review and left some comments inline.

azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

website/docs/r/kubernetes_cluster.html.markdown

katbyte · 2020-03-05T03:58:57Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

+									"allocated_outbound_port": {
+										Type:         schema.TypeInt,
+										Optional:     true,
+										Default:      -1,


What is the reason for -1 here? can't we just do d.GetOk() and only pass along the value if it is specified?

Nope. Because d.GetOk() will return "0" for TypeInt when you didn't specify "allocated_outbound_port" in tf config. Then service side would set this property as "0" but actually I don't want to set it as "0". I just want to set it as nil.

GetOk returns a int, bool, if the property isn't set i believe the boolean will be false? @neil-yechenwei

Yes. The boolean will be false if the property isn't set in tf config. But actually sometimes we don't want to set it as "false". We just want to set it as nil when the property isn't set in tf config.

@neil-yechenwei, does this not work:

if port, ok := config["allocated_outbound_port"].(int); ok { profile.AllocatedOutboundPorts = utils.Int32(int32(port)) }

Sorry - poor wording there. I believe that a read-back value from the API for AllocatedOutboundPorts will contain the default value 0, not nil, so we should be fine to set Computed and let the zero value pass through? The user can not set nil explicitly, and omitting the setting should use the default since it must have a value between 0 and 64,000?

@jackofallops , sorry, I don't think so. Service API would return nil when the property isn't set in tfconfig. So I think user is able to set it as nil without breaking change. If we set this property must be between 0 and 64000, then breaking change would happen. So I have a concern. When breaking change is allowed in terraform? The breaking change in here is allowed?

tfconfig without outbound_ports_allocated:

provider "azurerm" { features {} } resource "azurerm_resource_group" "test" { name = "acctestRG-aks-%d" location = "westus2" } resource "azurerm_kubernetes_cluster" "test" { name = "acctestaks%d" location = azurerm_resource_group.test.location resource_group_name = azurerm_resource_group.test.name dns_prefix = "acctestaks%d" kubernetes_version = "%s" linux_profile { admin_username = "acctestuser%d" ssh_key { key_data = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCqaZoyiz1qbdOQ8xEf6uEu1cCwYowo5FHtsBhqLoDnnp7KUTEBN+L2NxRIfQ781rxV6Iq5jSav6b2Q8z5KiseOlvKA/RF2wqU0UPYqQviQhLmW6THTpmrv/YkUCuzxDpsH7DUDhZcwySLKVVe0Qm3+5N2Ta6UYH3lsDf9R9wTP2K/+vAnflKebuypNlmocIvakFWoZda18FOmsOoIVXQ8HWFNCuw9ZCunMSN62QGamCe3dL5cXlkgHYv7ekJE15IA9aOJcM7e90oeTqo+7HTcWfdu0qQqPWY5ujyMw/llas8tsXY85LFqRnr3gJ02bAscjc477+X+j/gkpFoN1QEmt [email protected]" } } default_node_pool { name = "default" node_count = 1 vm_size = "Standard_DS2_v2" } service_principal { client_id = "%s" client_secret = "%s" } network_profile { network_plugin = "kubenet" load_balancer_sku = "Standard" load_balancer_profile { managed_outbound_ip_count = 2 } } }

terraform schema:

"load_balancer_profile": { Type: schema.TypeList, MaxItems: 1, ForceNew: true, Optional: true, Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "outbound_ports_allocated": { Type: schema.TypeInt, Optional: true, Default: 0, ValidateFunc: validation.IntBetween(0, 64000), },

The result of outbound_ports_allocated from service API:

Hi @neil-yechenwei
I meant the following for the schema:

"load_balancer_profile": { Type: schema.TypeList, MaxItems: 1, ForceNew: true, Optional: true, Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "outbound_ports_allocated": { Type: schema.TypeInt, Optional: true, Computed: true, ValidateFunc: validation.IntBetween(0, 64000), },

And can I see the read comes back with a 0 from the API:

with config:

network_profile { network_plugin = "kubenet" load_balancer_sku = "Standard" load_balancer_profile { managed_outbound_ip_count = 2 idle_timeout_in_minutes = 10 }

Does that help explain what I mean?

@jackofallops , sorry, Do you mean user must set property "outbound_ports_allocated" as 0~64000 for all created services including already existing services by user? If yes, I still think your change would cause breaking change which would impact other already existing services created by user. Maybe some user doesn't expect this change impacts their already provisioned service. So I am asking does hashicorp allow breaking change for here this situation. If it's allowed, I can update this PR to force user to set this property as 0-64000.
And I've tried your above changes, you actually already set this property outbound_ports_allocated as 0 in request body before calling service API so that you would get 0 from service side. Actually service API would return nil as default when this property isn't set in request body. Please see my below example. If my understanding is incorrect, could you commit whole changes of your solution to this PR for better understanding? Thanks.

tfconfig without property outbound_ports_allocated:

provider "azurerm" { features {} } resource "azurerm_resource_group" "test" { name = "acctestRG-aks-%d" location = "westus2" } resource "azurerm_kubernetes_cluster" "test" { name = "acctestaks%d" location = azurerm_resource_group.test.location resource_group_name = azurerm_resource_group.test.name dns_prefix = "acctestaks%d" kubernetes_version = "%s" linux_profile { admin_username = "acctestuser%d" ssh_key { key_data = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCqaZoyiz1qbdOQ8xEf6uEu1cCwYowo5FHtsBhqLoDnnp7KUTEBN+L2NxRIfQ781rxV6Iq5jSav6b2Q8z5KiseOlvKA/RF2wqU0UPYqQviQhLmW6THTpmrv/YkUCuzxDpsH7DUDhZcwySLKVVe0Qm3+5N2Ta6UYH3lsDf9R9wTP2K/+vAnflKebuypNlmocIvakFWoZda18FOmsOoIVXQ8HWFNCuw9ZCunMSN62QGamCe3dL5cXlkgHYv7ekJE15IA9aOJcM7e90oeTqo+7HTcWfdu0qQqPWY5ujyMw/llas8tsXY85LFqRnr3gJ02bAscjc477+X+j/gkpFoN1QEmt [email protected]" } } default_node_pool { name = "default" node_count = 1 vm_size = "Standard_DS2_v2" } service_principal { client_id = "%s" client_secret = "%s" } network_profile { network_plugin = "kubenet" load_balancer_sku = "Standard" load_balancer_profile { managed_outbound_ip_count = 2 } } }

terraform schema:

"load_balancer_profile": { Type: schema.TypeList, MaxItems: 1, ForceNew: true, Optional: true, Computed: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "outbound_ports_allocated": { Type: schema.TypeInt, Optional: true, Computed: true, ValidateFunc: validation.IntBetween(0, 64000), },

Do not set property outbound_ports_allocated before calling service api:

Getting value of property outbound_ports_allocated from Service API:

So service API would return nil NOT 0 when you didn't set this property in tfconfig.

@jackofallops , I've double confirmed with this service owner. They said property "AllocatedOutboundPorts" would be set to “0” and property “IdleTimeoutInMinutes” would be set to “30” in service side when we don't set these two properties in request body. So I've updated code. Please have a look. Thanks.

The answer from service owner:
Today, if a customer doesn’t set them explicitly then we don’t fill the two properties from the AKS side but they do get filled on the SLB side with the defaults.

neil-yechenwei · 2020-03-05T11:05:17Z

@katbyte , Thanks for your comments. I've updated code.

katbyte

Thanks @neil-yechenwei, one last comment about use of GetOK

neil-yechenwei · 2020-03-06T00:35:40Z

@katbyte , I've answered your concern. Please have a look.

katbyte · 2020-03-09T20:10:51Z

azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go

+									"allocated_outbound_port": {
+										Type:         schema.TypeInt,
+										Optional:     true,
+										Default:      -1,


@neil-yechenwei, does this not work:

if port, ok := config["allocated_outbound_port"].(int); ok { profile.AllocatedOutboundPorts = utils.Int32(int32(port)) }

katbyte · 2020-03-09T20:16:53Z

website/docs/r/kubernetes_cluster.html.markdown

@@ -273,6 +273,10 @@ A `load_balancer_profile` block supports the following:

 ~> **NOTE:** These options are mutually exclusive. Note that when specifying `outbound_ip_address_ids` ([azurerm_public_ip](/docs/providers/azurerm/r/public_ip.html)) the SKU must be `Standard`.

+* `allocated_outbound_port` - (Optional) Count of desired allocated SNAT port per VM for the cluster load balancer. Must be between `0` and `64000` inclusive.


I'm not sure what this property is, an allocated port or the number of ports? count here, (and the API being ports) lead me to believe it's a count? if so we should change the name? maybe

Suggested change

* `allocated_outbound_port` - (Optional) Count of desired allocated SNAT port per VM for the cluster load balancer. Must be between `0` and `64000` inclusive.

* `outbound_ports_allocated` - (Optional) Number of desired SNAT port for each VM in the clusters load balancer. Must be between `0` and `64000` inclusive.

or maybe outbound_port_count?

Updated with outbound_ports_allocated

website/docs/r/kubernetes_cluster.html.markdown

azurerm/internal/services/containers/tests/resource_arm_kubernetes_cluster_network_test.go

neil-yechenwei · 2020-03-10T12:45:08Z

@katbyte , yes. below code you suggested doesn't work. Because the code "config["allocated_outbound_port"].(int)" would return "0" when tfconfig doesn't specify the property "allocated_outbound_port" and there is no "Default: -1". Then the property "profile.AllocatedOutboundPorts" would be set as "0" and pass to service side and “0” is meaningful for this property in service side. But actually I just want to set this property as "nil" when tf config doesn't specify this property. So if I use below code, I cannot distinguish user specified "0" or nil. Maybe it's a bug in terraform, right?

if port, ok := config["allocated_outbound_port"].(int); ok {
profile.AllocatedOutboundPorts = utils.Int32(int32(port))
}

evenh · 2020-03-30T10:53:23Z

Thanks for this work @neil-yechenwei! Is there more work that remains or just reviewing?

etiennetremel · 2020-04-30T13:10:17Z

Thanks for putting this through @neil-yechenwei, any chance to get this merged?

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

tombuildsstuff · 2020-06-05T14:41:53Z

hey @neil-yechenwei

Thanks for this PR - apologies for the delayed review here!

Since there's a number of other Pull Requests currently open for AKS - to be able to merge all of these without them all conflicting, I've pulled these commits locally, squashed them, fix the comments and then combined them into a single branch.

As such, whilst I'd like to thank you for opening this Pull Request, I'm going to close this in favour of #7233 which integrates these changes (and the other open AKS PR's - and includes some of the other open Feature Requests).

Thanks!

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

ghost · 2020-06-11T13:41:50Z

This has been released in version 2.14.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 = "~> 2.14.0"
}
# ... other configuration ...

ghost · 2020-07-05T14:50:13Z

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!

neil-yechenwei added 3 commits February 20, 2020 14:31

Fix bug for azurerm_kubernetes_cluster

afef5ca

Update code

3a7a78c

update code

004206a

ghost added size/XL dependencies documentation labels Feb 20, 2020

katbyte requested changes Mar 4, 2020

View reviewed changes

neil-yechenwei added 2 commits March 4, 2020 16:37

Update test case to support new properties

4c384aa

Update test code

43d12c2

neil-yechenwei requested a review from katbyte March 4, 2020 09:40

katbyte requested changes Mar 5, 2020

View reviewed changes

katbyte added service/kubernetes-cluster enhancement labels Mar 5, 2020

neil-yechenwei added 2 commits March 5, 2020 17:58

Merge remote-tracking branch 'upstream/master' into improve_aks_cluster

cc567ec

Update code

e1007f4

neil-yechenwei requested a review from katbyte March 5, 2020 11:05

katbyte added the waiting-response label Mar 5, 2020

katbyte added this to the v2.1.0 milestone Mar 5, 2020

katbyte requested changes Mar 5, 2020

View reviewed changes

neil-yechenwei requested a review from katbyte March 6, 2020 00:35

ghost removed the waiting-response label Mar 6, 2020

katbyte requested changes Mar 9, 2020

View reviewed changes

katbyte added the waiting-response label Mar 9, 2020

ghost removed the waiting-response label Mar 10, 2020

neil-yechenwei added 2 commits March 10, 2020 20:47

Update code

be49b8f

Update code

98ae7c7

tombuildsstuff self-assigned this Mar 27, 2020

tombuildsstuff modified the milestones: v2.5.0, v2.6.0 Apr 8, 2020

tombuildsstuff modified the milestones: v2.6.0, v2.7.0 Apr 16, 2020

katbyte modified the milestones: v2.7.0, v2.8.0 Apr 23, 2020

katbyte modified the milestones: v2.8.0, v2.9.0 Apr 30, 2020

tombuildsstuff modified the milestones: v2.9.0, v2.11.0 May 7, 2020

tombuildsstuff modified the milestones: v2.11.0, v2.12.0 May 19, 2020

tombuildsstuff modified the milestones: v2.12.0, v2.13.0 May 27, 2020

tombuildsstuff removed the request for review from katbyte May 27, 2020 09:29

tombuildsstuff added a commit that referenced this pull request Jun 3, 2020

r/kubernetes_cluster: support for load balancer settings

55ee2c3

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

tombuildsstuff added a commit that referenced this pull request Jun 4, 2020

r/kubernetes_cluster: support for load balancer settings

dc43ba8

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

tombuildsstuff modified the milestones: v2.13.0, v2.14.0 Jun 4, 2020

tombuildsstuff added a commit that referenced this pull request Jun 5, 2020

r/kubernetes_cluster: support for load balancer settings

6fc3e36

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

tombuildsstuff mentioned this pull request Jun 5, 2020

Azure Kubernetes Service: 2020-06 updates #7233

Merged

tombuildsstuff closed this Jun 5, 2020

tombuildsstuff added a commit that referenced this pull request Jun 8, 2020

r/kubernetes_cluster: support for load balancer settings

f25d744

Porting over the changes from #5824 X-Committed-With: neil-yechenwei

ghost locked and limited conversation to collaborators Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more properties for azurerm_kubernetes_cluster #5824

Support more properties for azurerm_kubernetes_cluster #5824

neil-yechenwei commented Feb 20, 2020 •

edited by tombuildsstuff

Loading

katbyte left a comment

neil-yechenwei commented Mar 4, 2020

katbyte left a comment

katbyte Mar 5, 2020

neil-yechenwei Mar 5, 2020

katbyte Mar 5, 2020

neil-yechenwei Mar 6, 2020

katbyte Mar 9, 2020

jackofallops Mar 19, 2020

neil-yechenwei Mar 20, 2020

jackofallops Mar 20, 2020

neil-yechenwei Mar 21, 2020 •

edited

Loading

neil-yechenwei Mar 24, 2020 •

edited

Loading

neil-yechenwei commented Mar 5, 2020

katbyte left a comment

neil-yechenwei commented Mar 6, 2020

katbyte Mar 9, 2020

katbyte Mar 9, 2020

neil-yechenwei Mar 10, 2020

neil-yechenwei commented Mar 10, 2020 •

edited

Loading

evenh commented Mar 30, 2020

etiennetremel commented Apr 30, 2020

tombuildsstuff commented Jun 5, 2020

ghost commented Jun 11, 2020

ghost commented Jul 5, 2020

		@@ -273,6 +273,10 @@ A `load_balancer_profile` block supports the following:

		~> NOTE: These options are mutually exclusive. Note that when specifying `outbound_ip_address_ids` ([azurerm_public_ip](/docs/providers/azurerm/r/public_ip.html)) the SKU must be `Standard`.

		* `allocated_outbound_port` - (Optional) Count of desired allocated SNAT port per VM for the cluster load balancer. Must be between `0` and `64000` inclusive.

	* `allocated_outbound_port` - (Optional) Count of desired allocated SNAT port per VM for the cluster load balancer. Must be between `0` and `64000` inclusive.
	* `outbound_ports_allocated` - (Optional) Number of desired SNAT port for each VM in the clusters load balancer. Must be between `0` and `64000` inclusive.

Support more properties for azurerm_kubernetes_cluster #5824

Support more properties for azurerm_kubernetes_cluster #5824

Conversation

neil-yechenwei commented Feb 20, 2020 • edited by tombuildsstuff Loading

katbyte left a comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 4, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei commented Mar 5, 2020

katbyte left a comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Mar 10, 2020 • edited Loading

evenh commented Mar 30, 2020

etiennetremel commented Apr 30, 2020

tombuildsstuff commented Jun 5, 2020

ghost commented Jun 11, 2020

ghost commented Jul 5, 2020

neil-yechenwei commented Feb 20, 2020 •

edited by tombuildsstuff

Loading

neil-yechenwei Mar 21, 2020 •

edited

Loading

neil-yechenwei Mar 24, 2020 •

edited

Loading

neil-yechenwei commented Mar 10, 2020 •

edited

Loading