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.

Sign up for GitHub

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

Support more properties for azurerm_kubernetes_cluster #5824

Closed
wants to merge 14 commits into from
Closed

Support more properties for azurerm_kubernetes_cluster #5824

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
afef5ca
Fix bug for azurerm_kubernetes_cluster
Feb 20, 2020
3a7a78c
Update code
Feb 20, 2020
004206a
update code
Feb 20, 2020
4c384aa
Update test case to support new properties
Mar 4, 2020
43d12c2
Update test code
Mar 4, 2020
cc567ec
Merge remote-tracking branch 'upstream/master' into improve_aks_cluster
Mar 5, 2020
e1007f4
Update code
Mar 5, 2020
be49b8f
Update code
Mar 10, 2020
98ae7c7
Update code
Mar 10, 2020
8dbeb96
Update code
Mar 11, 2020
3fee1bc
Update code
Mar 21, 2020
2b2e530
Merge remote-tracking branch 'upstream/master' into improve_aks_cluster
Mar 24, 2020
29e124a
Update code
Mar 24, 2020
77bf90c
Update code
Mar 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azurerm/internal/services/containers/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package client
import (
"github.com/Azure/azure-sdk-for-go/services/containerinstance/mgmt/2018-10-01/containerinstance"
"github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2018-09-01/containerregistry"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/common"
)

Expand Down
2 changes: 1 addition & 1 deletion azurerm/internal/services/containers/data_source_kubernetes_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"strings"
"time"

"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes"
Expand Down
2 changes: 1 addition & 1 deletion azurerm/internal/services/containers/kubernetes_addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package containers
import (
"strings"

"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
Expand Down
2 changes: 1 addition & 1 deletion azurerm/internal/services/containers/kubernetes_nodepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package containers
import (
"fmt"

"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
Expand Down
50 changes: 38 additions & 12 deletions azurerm/internal/services/containers/resource_arm_kubernetes_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"strings"
"time"

"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
Expand Down Expand Up @@ -424,6 +424,18 @@ func resourceArmKubernetesCluster() *schema.Resource {
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"allocated_outbound_port": {
Type: schema.TypeInt,
Optional: true,
Default: -1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@neil-yechenwei, does this not work:

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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:
image

Copy link
Member

Choose a reason for hiding this comment

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

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:
image

image

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?

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Mar 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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:
image

Getting value of property outbound_ports_allocated from Service API:
image

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

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

ValidateFunc: validation.IntBetween(0, 64000),
},
"idle_timeout_in_minutes": {
Type: schema.TypeInt,
Optional: true,
Default: -1,
ValidateFunc: validation.IntBetween(4, 120),
},
"managed_outbound_ip_count": {
Type: schema.TypeInt,
Optional: true,
Expand Down Expand Up @@ -1487,29 +1499,31 @@ func expandLoadBalancerProfile(d []interface{}, loadBalancerType string) (*conta

config := d[0].(map[string]interface{})

var managedOutboundIps *containerservice.ManagedClusterLoadBalancerProfileManagedOutboundIPs
var outboundIpPrefixes *containerservice.ManagedClusterLoadBalancerProfileOutboundIPPrefixes
var outboundIps *containerservice.ManagedClusterLoadBalancerProfileOutboundIPs
loadBalancerProfile := &containerservice.ManagedClusterLoadBalancerProfile{}

if port := config["allocated_outbound_port"].(int); port >= 0 {
loadBalancerProfile.AllocatedOutboundPorts = utils.Int32(int32(port))
}

if idleTimeout := config["idle_timeout_in_minutes"].(int); idleTimeout >= 0 {
loadBalancerProfile.IdleTimeoutInMinutes = utils.Int32(int32(idleTimeout))
}

if ipCount := config["managed_outbound_ip_count"]; ipCount != nil {
if c := int32(ipCount.(int)); c > 0 {
managedOutboundIps = &containerservice.ManagedClusterLoadBalancerProfileManagedOutboundIPs{Count: &c}
loadBalancerProfile.ManagedOutboundIPs = &containerservice.ManagedClusterLoadBalancerProfileManagedOutboundIPs{Count: &c}
}
}

if ipPrefixes := idsToResourceReferences(config["outbound_ip_prefix_ids"]); ipPrefixes != nil {
outboundIpPrefixes = &containerservice.ManagedClusterLoadBalancerProfileOutboundIPPrefixes{PublicIPPrefixes: ipPrefixes}
loadBalancerProfile.OutboundIPPrefixes = &containerservice.ManagedClusterLoadBalancerProfileOutboundIPPrefixes{PublicIPPrefixes: ipPrefixes}
}

if outIps := idsToResourceReferences(config["outbound_ip_address_ids"]); outIps != nil {
outboundIps = &containerservice.ManagedClusterLoadBalancerProfileOutboundIPs{PublicIPs: outIps}
loadBalancerProfile.OutboundIPs = &containerservice.ManagedClusterLoadBalancerProfileOutboundIPs{PublicIPs: outIps}
}

return &containerservice.ManagedClusterLoadBalancerProfile{
ManagedOutboundIPs: managedOutboundIps,
OutboundIPPrefixes: outboundIpPrefixes,
OutboundIPs: outboundIps,
}, nil
return loadBalancerProfile, nil
}

func idsToResourceReferences(set interface{}) *[]containerservice.ResourceReference {
Expand Down Expand Up @@ -1581,6 +1595,18 @@ func flattenKubernetesClusterNetworkProfile(profile *containerservice.NetworkPro
if lbp := profile.LoadBalancerProfile; lbp != nil {
lb := make(map[string]interface{})

port := int32(-1)
if v := lbp.AllocatedOutboundPorts; v != nil {
port = *v
}
lb["allocated_outbound_port"] = port

idleTimeout := int32(-1)
if v := lbp.IdleTimeoutInMinutes; v != nil {
idleTimeout = *v
}
lb["idle_timeout_in_minutes"] = idleTimeout

if ips := lbp.ManagedOutboundIPs; ips != nil {
if count := ips.Count; count != nil {
lb["managed_outbound_ip_count"] = count
Expand Down
2 changes: 1 addition & 1 deletion azurerm/internal/services/containers/resource_arm_kubernetes_cluster_node_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"log"
"time"

"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-10-01/containerservice"
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2019-11-01/containerservice"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
Expand Down
21 changes: 14 additions & 7 deletions ...2019-10-01/containerservice/agentpools.go → ...2019-11-01/containerservice/agentpools.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 ...gmt/2019-10-01/containerservice/client.go → ...gmt/2019-11-01/containerservice/client.go
View file
Open in desktop
File renamed without changes.
0 ...-01/containerservice/containerservices.go → ...-01/containerservice/containerservices.go
View file
Open in desktop
File renamed without changes.
Loading