Containerservices addonprofile #1502

lfshr · 2018-07-05T21:56:46Z

Added addon_profile functionality to kubernetes clusters

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments in-line but this mostly LGTM; if we can fix up the comments then we should be able to run the tests and get this merged

Thanks!

tombuildsstuff · 2018-07-06T07:49:29Z

azurerm/resource_arm_kubernetes_cluster.go

+		addonProfile := map[string]interface{}{
+			"name":    k,
+			"enabled": *v.Enabled,
+			"config":  flattenAzureRmKubernetesClusterAddonProfileConfig(v.Config),


minor we can just inline this method; e.g.

addonProfile := map[string]interface{}{ ... } config := make(map[string]string, 0) for k, v := range v.Config { config[k] = v } addonProfile["config"] = config

tombuildsstuff · 2018-07-06T07:50:19Z

azurerm/resource_arm_kubernetes_cluster.go

+	for k, v := range profile {
+		addonProfile := map[string]interface{}{
+			"name":    k,
+			"enabled": *v.Enabled,


there's a potential crash if this isn't returned, can we nil-check this? e.g.

addonProfile := map[string]interface{}{ ... } if enabled := v.Enabled; enabled != nil { addonProfile["enabled"] = *enabled }

Is that still the case on a Required field, should I do some kind of error handling here if it's nil or is the nil check all that's needed? Good spot. I totally missed that.

this is the response coming back from the API, which can be nil due to bugs in the API / changes in the schema over time (this is something we've learnt over time, unfortunately) - so we need to check it either way unfortunately

tombuildsstuff · 2018-07-06T08:06:02Z

azurerm/resource_arm_kubernetes_cluster.go

+		configValue := make(map[string]*string)
+		for k, v := range addonConfig {
+			configEntry := v.(string)
+			configValue[k] = &configEntry


minor we can combine these to make: utils.String(v.(string))

tombuildsstuff · 2018-07-06T08:06:31Z

azurerm/resource_arm_kubernetes_cluster.go

@@ -209,6 +209,33 @@ func resourceArmKubernetesCluster() *schema.Resource {
 				Set: resourceAzureRMKubernetesClusterServicePrincipalProfileHash,
 			},

+			"addon_profile": {


can we document these new fields in the docs? this file's located at ./website/docs/r/kubernetes_cluster.html

can we also add an acceptance test covering these new fields?

lfshr · 2018-07-06T17:30:10Z

@tombuildsstuff Code updated to reflect your comments. This PR should be good to go.

lfshr · 2018-07-10T07:23:44Z

@tombuildsstuff following further on our discussion, the lack of documentation on addonProfiles is due to a known issue. Do I take what I know at face value for the minute, and revisit the schema after the documentation is finished, or do you want to hold off until then?

MicrosoftDocs/azure-docs#11090

lfshr · 2018-07-26T17:33:39Z

@tombuildsstuff now that advanced networking is done I'm turning my attention to this. Can you have a look at the latest commits (cea7faa and 1217732) and tell me if that's more what you're looking for? Only have HttpApplicationRouting enabled at the moment.

I'm going on holiday on Saturday for two weeks. I'll try my best to finish this but if I fail, feel free to continue.

…merge

tombuildsstuff · 2018-08-10T08:31:34Z

hey @lfshr

Sorry for the delayed re-review here - I've taken a look into this and merged master into this branch / have resolved the merge conflicts by adding the changes from this PR back in. Since I don't have permission to push to your fork - I've had to push to this repo and have opened #1751 which includes your original commits plus the updates required to get this merged - I hope you don't mind!

Thanks!

lfshr · 2018-08-10T10:17:32Z

@tombuildsstuff no problem at all. Thanks very much for continuing this. I ran out of time before my holiday I'm afraid 😞

Arrived back last night so let me know if there's anything else you need me to do.

tombuildsstuff · 2018-08-10T10:52:57Z

@lfshr all good - apologies for the delay getting this merged; we've been heads-down trying to work through some larger cross-provider items (e.g. #1746 / #1728) over the past couple of weeks

* Added kubernetes_cluster advanced network creation * Changed network_profile from TypeSet to TypeList * Added kubernetes_cluster advanced network creation * Changed network_profile from TypeSet to TypeList * Updated ForceNew attributes on kubernetes_cluster.network_profile params * Added advanced network read functionality * Added tests for kubernetes_cluster.network_profile * Fixed indentations * Fixed function name in tests * Fixed property name in test * Fixed variable name * working on data source for kubernetes cluster * Fixed issue with datasource not working * Added network_policy and fixed read * Removed network_policy as unsupported * Added example of advanced networking * Added test for data_source * Made address space more sensible * Removing kubenet test. This is default when network_profile is not specified. We already test this. * Made network_plugin a mandatory field. Better to make this explicit. * Updated documentation to include network_profile * Fixing the build * Added ForceNew to network_profile * Made the docs a little clearer * Documenting a required field * Rephrasing the data sources * Making the `network_profile` block computed 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 ``` * Fixing the field we're asserting on * Conditionally expanding the network_profile block. The ip fields are now Computed too * Validation to ensure that a Subnet ID is assigned to the cluster when the network_profile blocks specified Example: ``` Error: Error running plan: 1 error(s) occurred: * azurerm_kubernetes_cluster.test: 1 error(s) occurred: * azurerm_kubernetes_cluster.test: If a `network_profile` block is specified, the Agent Pools must be attached to a Subnet ``` * Updating the documentation to mention the new behaviour * Adding independent tests for Kubenet and Azure networking profiles * Conditional validation of the `docker_bridge_cidr`, `dns_service_ip` and `service_cidr` fields. * Updating the docs * Fixing the tests * Adding back in the changes from PR #1502 since they were lost in the merge * Renaming the methods to `addonProfiles` to match the sdk * fixing the quotes

ghost · 2020-03-30T15:04:22Z

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 added 8 commits July 5, 2018 16:29

Added addon_profile to schema

d03426d

Added addon_profile code - best guess at it without syntax highlighting

8410473

Added to create (best guess without syntax highlighting)

1854824

Working version of addon_profile

af0f2bd

Added Application HTTP Routing to example

45749a7

Fixed parameter name

a86334e

Added http application routing zone name to output example

f350c44

Fixed names

c15431f

lfshr mentioned this pull request Jul 5, 2018

Feature Request : addonProfiles and networkProfile for Azure Kubernetes Service (AKS) #1434

Closed

tombuildsstuff added service/kubernetes-cluster enhancement labels Jul 6, 2018

tombuildsstuff suggested changes Jul 6, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 6, 2018

lfshr added 7 commits July 6, 2018 16:02

Updated code as per comments

0c27f11

Fixed read function (oops)

15f60a9

Added addon_profile to docs

9fcd4da

Fixing attribute name in d.Set

e08d076

Added addon_profile to data_source

b4af391

Added resource test coverage

c605682

Slight cleanup on docs

15f1d6e

lfshr mentioned this pull request Jul 6, 2018

Support rbac and azure ad integration for Azure Kubernetes Service #1429

Closed

Made enabled implicit

dee54e5

tombuildsstuff removed the waiting-response label Jul 19, 2018

Merge branch 'master' into containerservices-addonprofile

b8f636c

lfshr added 3 commits July 26, 2018 19:29

Changed addon_profiles schema to a static schema

cea7faa

Added config to http_application_routing addon_profile

1217732

Working on oms agent - config expansion not working - WIP

b00d640

lfshr added 4 commits July 27, 2018 12:14

Fixing config maps

e560ffd

Removed development code

2782949

Updated data source to reflect resource

1772b28

Updated test and added httpApplicationRouting (untested)

e55aa30

tombuildsstuff self-assigned this Aug 2, 2018

metacpp self-requested a review August 3, 2018 00:53

tombuildsstuff removed the request for review from metacpp August 8, 2018 16:34

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

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

403bc1d

…merge

tombuildsstuff mentioned this pull request Aug 10, 2018

AKS: Add-On Profiles #1751

Merged

tombuildsstuff closed this Aug 10, 2018

lfshr deleted the containerservices-addonprofile branch August 14, 2018 12:51

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Containerservices addonprofile #1502

Containerservices addonprofile #1502

lfshr commented Jul 5, 2018

tombuildsstuff left a comment

tombuildsstuff Jul 6, 2018

tombuildsstuff Jul 6, 2018

lfshr Jul 6, 2018

tombuildsstuff Jul 6, 2018

tombuildsstuff Jul 6, 2018

tombuildsstuff Jul 6, 2018

tombuildsstuff Jul 6, 2018

lfshr commented Jul 6, 2018

lfshr commented Jul 10, 2018

lfshr commented Jul 26, 2018 •

edited

Loading

tombuildsstuff commented Aug 10, 2018

lfshr commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

ghost commented Mar 30, 2020

Containerservices addonprofile #1502

Containerservices addonprofile #1502

Conversation

lfshr commented Jul 5, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

lfshr Jul 6, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 6, 2018

Choose a reason for hiding this comment

lfshr commented Jul 6, 2018

lfshr commented Jul 10, 2018

lfshr commented Jul 26, 2018 • edited Loading

tombuildsstuff commented Aug 10, 2018

lfshr commented Aug 10, 2018

tombuildsstuff commented Aug 10, 2018

ghost commented Mar 30, 2020

lfshr commented Jul 26, 2018 •

edited

Loading