AKS: Add-On Profiles #1751

tombuildsstuff · 2018-08-10T08:30:18Z

Includes the changes originally contributed by @lfshr in #1502 - but updates this to resolve the merge conflicts

….com/lfshr/terraform-provider-azurerm into containerservices-advancednetworking

This is default when network_profile is not specified. We already test this.

Better to make this explicit.

Tests pass: ``` $ acctests azurerm TestAccAzureRMKubernetesCluster_basic === RUN TestAccAzureRMKubernetesCluster_basic --- PASS: TestAccAzureRMKubernetesCluster_basic (1329.57s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1329.613s ```

…merge

yves-vogl · 2018-08-10T09:09:26Z

Thanks for your work!

lfshr · 2018-08-10T10:27:30Z

azurerm/resource_arm_kubernetes_cluster.go

@@ -321,6 +372,7 @@ func resourceArmKubernetesClusterCreate(d *schema.ResourceData, meta interface{}
 	agentProfiles := expandAzureRmKubernetesClusterAgentProfiles(d)
 	servicePrincipalProfile := expandAzureRmKubernetesClusterServicePrincipal(d)
 	networkProfile := expandAzureRmKubernetesClusterNetworkProfile(d)
+	addOnProfile := expandAzureRmKubernetesClusterAddOnProfile(d)


Am I being nit-picky to request this to be addonProfiles instead of addOnProfile? (just to reflect AddonProfiles with camel-casing)

not at all - the schema field is addon_profile to match the general convention, but I can push a commit to update this :)

Thanks Tom! 😄

pushed: be893dc :)

tombuildsstuff · 2018-08-10T10:50:21Z

@yves-vogl this is mostly @lfshr's work/original PR - I've just pushed a couple of commits to finish it off, so thanks to @lfshr here :)

lfshr · 2018-08-10T10:54:28Z

#1488 has a dependency on this

tombuildsstuff · 2018-08-10T12:00:53Z

Resource Tests pass:

tombuildsstuff · 2018-08-10T12:48:57Z

Data Source tests pass:

radeksimko

Just a few nitpicks + 1 blocker (deduplication of code).

radeksimko · 2018-08-13T09:10:15Z

azurerm/data_source_kubernetes_cluster.go

@@ -419,3 +465,49 @@ func flattenKubernetesClusterDataSourceNetworkProfile(profile *containerservice.

 	return []interface{}{values}
 }
+
+func flattenKubernetesClusterDataSourceAddonProfiles(profile map[string]*containerservice.ManagedClusterAddonProfile) interface{} {


Can't we just rename either this function or flattenAzureRmKubernetesClusterAddonProfiles and reuse it? It's exactly the same function with the same interface, just a different name.

probably - but historically this has caused us issues where the schema's diverged - so we're trying to avoid that

I'm not sure I follow, how could it diverge if the output is coming from the exact same API call? If the API response is going to diverge, then both functions (as they're exactly the same except name) need fixing, AFAICT?

the schema being the Terraform schema here - since one is a Data Source and hence Computed (this one) vs the other being a Resource / not computed. We've had PR's in the past with folks adding items to each independently, which means there's a Set error when this happens - as such it appears preferable to dupe this logic?

ah, I see what do you mean 🤔That's tricky, yeah.

I wish we had a better way to reuse parts of schema between resources and data sources 😞

Thanks for this discussion -- this is something we can run into on AWS as well since we generally are pushing for separate schema definitions and read functions, but usually sharing the flatten functions. 🤔

radeksimko · 2018-08-13T09:13:58Z

azurerm/resource_arm_kubernetes_cluster_test.go

+					testCheckAzureRMKubernetesClusterExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "addon_profile.0.http_application_routing.#", "1"),
+					resource.TestCheckResourceAttrSet(resourceName, "addon_profile.0.http_application_routing.0.enabled"),
+					resource.TestCheckResourceAttrSet(resourceName, "addon_profile.0.http_application_routing.0.http_application_routing_zone_name"),


Nitpick: Can't/shouldn't we be testing the exact value, instead of just whether it's set?

it's a computed value from the API - so we're just ensuring it's returned

I meant mainly enabled (which presumably is just "true"), I'm not sure about http_application_routing_zone_name, but in theory TestMatchResourceAttrSet might work?

Again, it's just a nitpick, so feel free to ignore ;-)

radeksimko · 2018-08-13T09:15:38Z

azurerm/resource_arm_kubernetes_cluster_test.go

+  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]"


Nitpick: This could live in a separate file/dir (test-fixtures) and be reused in any other tests which need SSH pubkey.

radeksimko · 2018-08-13T09:16:37Z

website/docs/r/kubernetes_cluster.html.markdown

 * `linux_profile` - (Required) A Linux Profile block as documented below.

 * `agent_pool_profile` - (Required) One or more Agent Pool Profile's block as documented below.

 * `service_principal` - (Required) A Service Principal block as documented below.

+---


Just curious - what's the point of separating these arguments by line?

separating arguments by line means the diff's/merge conflicts are generally clearer
the dashes are to make it clearer to read - see the VM resource for an example: https://www.terraform.io/docs/providers/azurerm/r/virtual_machine.html - which makes it clearer what's required / makes it easier to understand

radeksimko · 2018-08-13T09:17:45Z

website/docs/r/kubernetes_cluster.html.markdown


-* `network_plugin` - (Required) Network plugin to use for networking. Currently supported values are `azure` and `kubenet`. Changing this forces a new resource to be created.


Nitpick: I'd actually argue that backticks were more appropriate here as they enable highlighting of the string and make it stand out.

tombuildsstuff · 2018-08-15T20:52:15Z

hey @yves-vogl @lfshr

Just to let you know that we've just shipped support for this as a part of v1.13.0 of the AzureRM Provider :)

Thanks!

thommaa · 2018-09-05T15:46:13Z

@tombuildsstuff Hi, thanks for implementing it.

For both addon_profile elements: http_application_routing and oms_agent, you have choosen ForceNew: true. What was the motivation or intension to force a new cluster, when changing the values?

The Azure CLI offers inplace update of an existing cluster. az aks enable-addons

lfshr · 2018-09-05T15:56:44Z

@thommaa I believe that the azure-sdk-for-go threw an error when trying to update the cluster.

yves-vogl · 2018-09-05T16:03:52Z

@thommaa @lfshr Oh, maybe a low hanging fruit for contribution to the SDK :-P

lfshr · 2018-09-05T16:06:06Z

@thommaa @yves-vogl I'll try and do some more testing at some point this week (before spider-man gets released ofc)

yves-vogl · 2018-09-05T16:10:41Z

I'll try to have a look at the sources, too. We currently make heavy use of those stuff and it seems to be just fair if we spend some customer paid hours for contribution. So if any help is required, let us know.

ghost · 2020-03-30T14:58:09Z

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!

lfshr and others added 30 commits July 2, 2018 19:31

Added kubernetes_cluster advanced network creation

5865322

Changed network_profile from TypeSet to TypeList

305c1d0

Added kubernetes_cluster advanced network creation

df237b2

Changed network_profile from TypeSet to TypeList

e3ec37e

Merge branch 'containerservices-advancednetworking' of https://github…

a3ff5b0

….com/lfshr/terraform-provider-azurerm into containerservices-advancednetworking

Updated ForceNew attributes on kubernetes_cluster.network_profile params

b7d274b

Added advanced network read functionality

11bed67

Added tests for kubernetes_cluster.network_profile

30f39ff

Fixed indentations

195e7d6

Fixed function name in tests

f8f5626

Fixed property name in test

25c6e6e

Fixed variable name

48cff90

working on data source for kubernetes cluster

2dd887c

Merge branch 'containerservices-advancednetworking' of https://github…

1bdf8b6

….com/lfshr/terraform-provider-azurerm into containerservices-advancednetworking

Fixed issue with datasource not working

33c84c6

Added network_policy and fixed read

f9786e1

Removed network_policy as unsupported

02b22e5

Added example of advanced networking

b62a643

Added test for data_source

377c52f

Made address space more sensible

7453f41

Removing kubenet test.

c90ea55

This is default when network_profile is not specified. We already test this.

Made network_plugin a mandatory field.

16e7d3e

Better to make this explicit.

Updated documentation to include network_profile

675d0b9

Fixing the build

7c21183

Added ForceNew to network_profile

1959c2f

Made the docs a little clearer

518cacc

Documenting a required field

9faeb69

Rephrasing the data sources

1ef6ccf

Fixing the field we're asserting on

48d5ed7

tombuildsstuff added 2 commits August 10, 2018 08:05

Merge branch 'master' into containerservices-advancednetworking

7d62f51

Adding back in the changes from PR #1502 since they were lost in the …

403bc1d

…merge

tombuildsstuff added enhancement service/kubernetes-cluster labels Aug 10, 2018

tombuildsstuff requested a review from a team August 10, 2018 08:30

tombuildsstuff mentioned this pull request Aug 10, 2018

Containerservices addonprofile #1502

Closed

tombuildsstuff added this to the 1.13.0 milestone Aug 10, 2018

lfshr reviewed Aug 10, 2018

View reviewed changes

Renaming the methods to addonProfiles to match the sdk

be893dc

radeksimko suggested changes Aug 13, 2018

View reviewed changes

fixing the quotes

018a6db

radeksimko approved these changes Aug 13, 2018

View reviewed changes

Merge branch 'master' into containerservices-advancednetworking

3106ee1

tombuildsstuff merged commit 531df6d into master Aug 14, 2018

tombuildsstuff deleted the containerservices-advancednetworking branch August 14, 2018 12:28

tombuildsstuff added a commit that referenced this pull request Aug 14, 2018

Updating to include #1751

8675821

flavioneri mentioned this pull request Aug 30, 2018

Terraform azurerm http_application_routing clarification #1846

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AKS: Add-On Profiles #1751

AKS: Add-On Profiles #1751

tombuildsstuff commented Aug 10, 2018

yves-vogl commented Aug 10, 2018

lfshr Aug 10, 2018

tombuildsstuff Aug 10, 2018

lfshr Aug 10, 2018

tombuildsstuff Aug 10, 2018 •

edited

Loading

tombuildsstuff commented Aug 10, 2018

lfshr commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

radeksimko left a comment

radeksimko Aug 13, 2018

tombuildsstuff Aug 13, 2018

radeksimko Aug 13, 2018

tombuildsstuff Aug 13, 2018 •

edited

Loading

radeksimko Aug 13, 2018

bflad Aug 13, 2018

radeksimko Aug 13, 2018

tombuildsstuff Aug 13, 2018

radeksimko Aug 13, 2018

radeksimko Aug 13, 2018

radeksimko Aug 13, 2018

tombuildsstuff Aug 13, 2018

radeksimko Aug 13, 2018

tombuildsstuff Aug 13, 2018

tombuildsstuff commented Aug 15, 2018

thommaa commented Sep 5, 2018

lfshr commented Sep 5, 2018

yves-vogl commented Sep 5, 2018

lfshr commented Sep 5, 2018

yves-vogl commented Sep 5, 2018 •

edited

Loading

ghost commented Mar 30, 2020


		* `network_plugin` - (Required) Network plugin to use for networking. Currently supported values are `azure` and `kubenet`. Changing this forces a new resource to be created.

AKS: Add-On Profiles #1751

AKS: Add-On Profiles #1751

Conversation

tombuildsstuff commented Aug 10, 2018

yves-vogl commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff commented Aug 10, 2018

lfshr commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

radeksimko 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

tombuildsstuff Aug 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Aug 15, 2018

thommaa commented Sep 5, 2018

lfshr commented Sep 5, 2018

yves-vogl commented Sep 5, 2018

lfshr commented Sep 5, 2018

yves-vogl commented Sep 5, 2018 • edited Loading

ghost commented Mar 30, 2020

tombuildsstuff Aug 10, 2018 •

edited

Loading

tombuildsstuff Aug 13, 2018 •

edited

Loading

yves-vogl commented Sep 5, 2018 •

edited

Loading