Update validation logic for kubenet network plugin. #1715

metacpp · 2018-08-03T05:09:00Z

After sync-up with AKS team, when network plugin is kubenet, dockerBridgeCidr, dnsServiceIP , and serviceCidr.
Update the test case for kubenet scenario.
Update the documentation for azure and kubenet.

The result of related acceptance tests (8/8/2018):

=== RUN   TestAccAzureRMKubernetesCluster_importBasic
--- PASS: TestAccAzureRMKubernetesCluster_importBasic (1347.68s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_basic
--- PASS: TestAccAzureRMKubernetesCluster_basic (1466.90s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_internalNetwork
--- PASS: TestAccAzureRMKubernetesCluster_internalNetwork (1505.84s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_advancedNetworkingAzureComplete
--- PASS: TestAccAzureRMKubernetesCluster_advancedNetworkingAzureComplete (1509.45s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_advancedNetworkingAzure
--- PASS: TestAccAzureRMKubernetesCluster_advancedNetworkingAzure (1509.38s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_advancedNetworkingKubenet
--- PASS: TestAccAzureRMKubernetesCluster_advancedNetworkingKubenet (1520.81s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_advancedNetworkingKubenetComplete
--- PASS: TestAccAzureRMKubernetesCluster_advancedNetworkingKubenetComplete (1566.04s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_addAgent
--- PASS: TestAccAzureRMKubernetesCluster_addAgent (1966.13s)
PASS
=== RUN   TestAccAzureRMKubernetesCluster_upgradeConfig
--- PASS: TestAccAzureRMKubernetesCluster_upgradeConfig (2089.39s)
PASS
ok    ../azurerm/test-azurerm    2089.524s

tombuildsstuff

hi @metacpp

Thanks for this PR. I've left some comments in-line but we need to ensure we test both the optional + specified cases here. In addition since the service_cidr, dns_service_ip and docker_bridge_cidr fields are only applicable to the kubenet network profile - we should document this.

Thanks!

tombuildsstuff · 2018-08-03T07:27:07Z

azurerm/resource_arm_kubernetes_cluster.go

-					dnsServiceIP := profile["dns_service_ip"].(string)
-					serviceCidr := profile["service_cidr"].(string)
-
-					if dockerBridgeCidr == "" || dnsServiceIP == "" || serviceCidr == "" {


we should add this validation back in but ensure that if one of the fields is set then the other fields are set too #Resolved

They either need to be all empty or all non-empty, this is server side logic. Although we can add it here to improve the experience, but we also introduced the risk to guess the server logic. Anyway, I made the change as you like.

In reply to: 207461949 [](ancestors = 207461949)

tombuildsstuff · 2018-08-03T07:27:27Z

azurerm/resource_arm_kubernetes_cluster_test.go

@@ -526,9 +523,6 @@ resource "azurerm_kubernetes_cluster" "test" {

  network_profile {
    network_plugin     = "kubenet"
-    dns_service_ip     = "10.10.0.10"
-    docker_bridge_cidr = "172.18.0.1/16"
-    service_cidr       = "10.10.0.0/16"
  }


we should update to make two tests here, one for when this is not specified and one for when this is #Resolved

Yeah, 2 legacy tests will use different settings: one is all empty, the other is all non-empty.

In reply to: 207462036 [](ancestors = 207462036)

tombuildsstuff · 2018-08-03T07:28:02Z

website/docs/r/kubernetes_cluster.html.markdown


-* `docker_bridge_cidr` - (Optional) IP address (in CIDR notation) used as the Docker bridge IP address on nodes. This is required when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.


we should add in "This field can only be set when network_plugin is set to kubenet" #Resolved

Updated.

In reply to: 207462137 [](ancestors = 207462137)

It should also be noted that the docker_bridge_cidr must have the last octet noted or the RP will fail. e.g 172.26.0.1/16 not 172.26.0.0/16 The server side logic is testing for this last octet and will stop the deployment.

Also the logic above for dns_service_ip is incorrect. It is only required is serviceCidr is entered not docker_bride_cidr #WontFix

We tried this combination:

network_profile { network_plugin = "kubenet" dns_service_ip = "10.10.0.10" docker_bridge_cidr = "" service_cidr = "10.10.0.0/16" }

It still fails due to not all of 3 are set.

In reply to: 207924196 [](ancestors = 207924196)

For the octet, I don't quite understand, can you be more specific on the documentation change?

In reply to: 208059432 [](ancestors = 208059432,207924196)

If someone enters what they normally think of a CIDR address is such as 172.16.0.0/16 the RP will kick back an error saying that docker_bridge_Cidr is not a valid cidr address. It must be the first available address in the space and the prefix number, not the network name and prefix. #WontFix

Where can we find the these rules? I feel we probably can provide a link to how to define the addresses here instead. Anyway, feel free to send another PR to update this document, that will be more efficient.

In reply to: 208078669 [](ancestors = 208078669)

Let's take it offline.

In reply to: 208059738 [](ancestors = 208059738,208059432,207924196)

tombuildsstuff · 2018-08-03T07:28:07Z

website/docs/r/kubernetes_cluster.html.markdown


 ~> **NOTE:** This range should not be used by any network element on or connected to this VNet. Service address CIDR must be smaller than /12.

-* `dns_service_ip` - (Optional) IP address within the Kubernetes service address range that will be used by cluster service discovery (kube-dns). This is required when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.


we should add in "This field can only be set when network_plugin is set to kubenet" #Resolved

Updated.

In reply to: 207462154 [](ancestors = 207462154)

tombuildsstuff · 2018-08-03T07:28:09Z

website/docs/r/kubernetes_cluster.html.markdown


 -> **NOTE:** When `network_plugin` is set to `azure` - the `vnet_subnet_id` field in the `agent_pool_profile` block must be set.

-* `service_cidr` - (Optional) The Network Range used by the Kubernetes service. This is required when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.
+* `service_cidr` - (Optional) The Network Range used by the Kubernetes service. Changing this forces a new resource to be created.


we should add in "This field can only be set when network_plugin is set to kubenet" #Resolved

This field can only bet set with other 2 fields.

In reply to: 207462162 [](ancestors = 207462162)

tombuildsstuff · 2018-08-03T07:30:13Z

azurerm/resource_arm_kubernetes_cluster.go

-				profile := rawProfiles[0].(map[string]interface{})
-				networkPlugin := profile["network_plugin"].(string)
-
-				if networkPlugin == "kubenet" {


we can also invert this logic to ensure they're not set when the networkPlugin is "azure" #Resolved

Both azure and kubenet need to be taken care.

In reply to: 207462580 [](ancestors = 207462580)

…profile setting. The docker_bridge_cidr, dns_service_ip, service_cidr should be either all empty or all non-empty. Closes #1648

…ce for AKS. Updated the test confiuration code for data source of AKS to use new refactored functions.

metacpp · 2018-08-03T23:34:44Z

@jeffreyCline is added to the review. #Closed

Fix the formatting error while running gofmt on AKS resource related files.

WodansSon

Mostly LGTM, left one comment.

WodansSon · 2018-08-04T00:31:27Z

azurerm/resource_arm_kubernetes_cluster.go

-						return fmt.Errorf("If the `network_plugin` is set to `kubenet` then the fields `docker_bridge_cidr`, `dns_service_ip` and `service_cidr` must not be empty.")
+					if !((dockerBridgeCidr == "" && dnsServiceIP == "" && serviceCidr == "") ||
+						(dockerBridgeCidr != "" && dnsServiceIP != "" && serviceCidr != "")) {
+						return fmt.Errorf("`docker_bridge_cidr`, `dns_service_ip` and `service_cidr` should all be empty or neither should be empty.")


This is a confusing error msg, could we change it to something like:
docker_bridge_cidr, dns_service_ip and service_cidr should all be empty or all should be set.
#Resolved

Good suggestion, updated it.

In reply to: 207693716 [](ancestors = 207693716)

…ofile setting. Change the description of error message to be more readable.

tombuildsstuff

hey @metacpp

Thanks for pushing the updates - if we can fix the minor refactoring then this otherwise LGTM (I'm assuming the tests pass 😄) 👍

Thanks!

tombuildsstuff · 2018-08-06T16:05:21Z

azurerm/resource_arm_kubernetes_cluster_test.go

@@ -410,7 +441,9 @@ resource "azurerm_kubernetes_cluster" "test" {
 `, rInt, location, rInt, rInt, rInt, clientId, clientSecret)
 }

-func testAccAzureRMKubernetesCluster_advancedNetworkingKubenet(rInt int, clientId string, clientSecret string, location string) string {
+func testAccAzureRMKubernetesCluster_advancedNetworking(rInt int, clientId string, clientSecret string, location string,


given the values are going to be the same for the dnsServiceIp , dockerBridgeCidr and serviceCidr fields in both Kubenet and Azure in both cases - we can duplicate the test configuration and hard-code the values for both cases? #Resolved

Thanks for the suggestion.The extra parameters provide functionalities to add more testing combinations, besides all empty or all set. I would like to leave it here.

In reply to: 207946292 [](ancestors = 207946292)

@metacpp this should be refactored to match the other resources, since the values intentionally shouldn't be set to empty strings if they're unset #Resolved

I did the refactoring to align with your preference here.

In reply to: 208550336 [](ancestors = 208550336)

tombuildsstuff · 2018-08-06T16:09:07Z

azurerm/resource_arm_kubernetes_cluster.go

-					if dockerBridgeCidr == "" || dnsServiceIP == "" || serviceCidr == "" {
-						return fmt.Errorf("If the `network_plugin` is set to `kubenet` then the fields `docker_bridge_cidr`, `dns_service_ip` and `service_cidr` must not be empty.")
+					if !((dockerBridgeCidr == "" && dnsServiceIP == "" && serviceCidr == "") ||
+						(dockerBridgeCidr != "" && dnsServiceIP != "" && serviceCidr != "")) {


minor / not a blocker we could probably make this more readable by doing:

hasDockerCidr := dockerBridgeCidr != "" hasServiceIP := dnsServiceIP != "" hasServiceCidr := serviceCidr != "" allEmpty := !hasDockerCidr && !hasServiceIP && !hasServiceCidr allSet := hasDockerCidr && hasServiceIP && hasServiceCidr if !allEmpty && !allSet { return fmt.Errorf("...") } ``` #Resolved

Good suggestion, let me figure out a better way.

In reply to: 207947438 [](ancestors = 207947438)

…p nested code. Use early return to remove deeply nested if-else block.

TestAzureRMKubernetesCluster_agentPoolName was a unit test, which should not have Acc in the naming.

…ions. Ignore the setting of empty string values.

Updated the arguments to align with new refactored function for configuration.

tombuildsstuff · 2018-08-09T09:03:54Z

@metacpp heads up that I've pushed a commit to refactor the test case to match the other test cases

tombuildsstuff · 2018-08-09T14:23:53Z

Tests pass:

ghost · 2020-03-30T15:05:53Z

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!

Remove validation logic for kubenet network plugin.

0542c72

metacpp requested review from tombuildsstuff, katbyte, JunyiYi and WodansSon August 3, 2018 05:09

metacpp self-assigned this Aug 3, 2018

metacpp added service/kubernetes-cluster documentation labels Aug 3, 2018

metacpp added this to the 1.12.0 milestone Aug 3, 2018

metacpp added the enhancement label Aug 3, 2018

tombuildsstuff removed request for tombuildsstuff, katbyte, JunyiYi and WodansSon August 3, 2018 07:18

tombuildsstuff requested changes Aug 3, 2018

View reviewed changes

tombuildsstuff added waiting-response and removed documentation labels Aug 3, 2018

tombuildsstuff modified the milestones: 1.12.0, 1.13.0 Aug 3, 2018

tombuildsstuff mentioned this pull request Aug 3, 2018

resource_arm_kubernetes_cluster.go network profile logic is incorrect #1719

Closed

metacpp added 2 commits August 3, 2018 16:10

🐛 (Azure Kubernete resource.) Update the validation logic of network_…

6397658

…profile setting. The docker_bridge_cidr, dns_service_ip, service_cidr should be either all empty or all non-empty. Closes #1648

🐛 (data source of AKS.) Fix the compiling error of tests of data sour…

073706d

…ce for AKS. Updated the test confiuration code for data source of AKS to use new refactored functions.

metacpp requested review from katbyte and JunyiYi August 3, 2018 23:34

metacpp requested a review from WodansSon August 3, 2018 23:34

🐛 (AKS resource.) Format error.

104a9f0

Fix the formatting error while running gofmt on AKS resource related files.

WodansSon approved these changes Aug 4, 2018

View reviewed changes

📝 (AKS resource documentation.) Fix the error messsage for network pr…

cf1401d

…ofile setting. Change the description of error message to be more readable.

tombuildsstuff removed the request for review from JunyiYi August 6, 2018 13:10

tombuildsstuff approved these changes Aug 6, 2018

View reviewed changes

♻️ (AKS resource.) Refactor the validation logic throuhg removing dee…

386080d

…p nested code. Use early return to remove deeply nested if-else block.

metacpp changed the title ~~Remove validation logic for kubenet network plugin.~~ Update validation logic for kubenet network plugin. Aug 7, 2018

metacpp added 3 commits August 7, 2018 15:35

♻️ (Test code of AKS resource.) Rename test case name.

8cd7bbc

TestAzureRMKubernetesCluster_agentPoolName was a unit test, which should not have Acc in the naming.

♻️ (AKS resource testing.) Refactor the test case configuration funct…

1b8e35a

…ions. Ignore the setting of empty string values.

🐛 (AKS data source testing.) The wrong arguments passed in.

625b9f2

Updated the arguments to align with new refactored function for configuration.

tombuildsstuff removed the waiting-response label Aug 9, 2018

tombuildsstuff force-pushed the aks_fix branch from 68d05b2 to 66bb039 Compare August 9, 2018 09:06

Fixing the outstanding PR comments

4502449

tombuildsstuff force-pushed the aks_fix branch from 66bb039 to 4502449 Compare August 9, 2018 09:45

tombuildsstuff merged commit 8bad3aa into master Aug 9, 2018

tombuildsstuff deleted the aks_fix branch August 9, 2018 14:25

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

Updating to include #1715

92c2c51

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

Updating to include #1715

6def3d8

hashicorp locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update validation logic for kubenet network plugin. #1715

Update validation logic for kubenet network plugin. #1715

metacpp commented Aug 3, 2018 •

edited

tombuildsstuff left a comment

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

evillgenius75 Aug 6, 2018 •

edited by metacpp

metacpp Aug 6, 2018

metacpp Aug 6, 2018

evillgenius75 Aug 7, 2018 •

edited by metacpp

metacpp Aug 7, 2018

metacpp Aug 7, 2018

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

tombuildsstuff Aug 3, 2018 •

edited by metacpp

metacpp Aug 3, 2018

metacpp commented Aug 3, 2018

WodansSon left a comment

WodansSon Aug 4, 2018 •

edited by metacpp

metacpp Aug 4, 2018

tombuildsstuff left a comment

tombuildsstuff Aug 6, 2018 •

edited by metacpp

metacpp Aug 6, 2018

tombuildsstuff Aug 8, 2018 •

edited by metacpp

metacpp Aug 8, 2018

tombuildsstuff Aug 6, 2018 •

edited by metacpp

metacpp Aug 6, 2018

tombuildsstuff commented Aug 9, 2018

tombuildsstuff commented Aug 9, 2018

ghost commented Mar 30, 2020


		* `docker_bridge_cidr` - (Optional) IP address (in CIDR notation) used as the Docker bridge IP address on nodes. This is required when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.


		~> NOTE: This range should not be used by any network element on or connected to this VNet. Service address CIDR must be smaller than /12.

		* `dns_service_ip` - (Optional) IP address within the Kubernetes service address range that will be used by cluster service discovery (kube-dns). This is required when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.

Update validation logic for kubenet network plugin. #1715

Update validation logic for kubenet network plugin. #1715

Conversation

metacpp commented Aug 3, 2018 • edited

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evillgenius75 Aug 6, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evillgenius75 Aug 7, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 3, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Aug 3, 2018

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon Aug 4, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Aug 6, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 8, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Aug 6, 2018 • edited by metacpp

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Aug 9, 2018

tombuildsstuff commented Aug 9, 2018

ghost commented Mar 30, 2020

metacpp commented Aug 3, 2018 •

edited

tombuildsstuff Aug 3, 2018 •

edited by metacpp

tombuildsstuff Aug 3, 2018 •

edited by metacpp

tombuildsstuff Aug 3, 2018 •

edited by metacpp

evillgenius75 Aug 6, 2018 •

edited by metacpp

evillgenius75 Aug 7, 2018 •

edited by metacpp

tombuildsstuff Aug 3, 2018 •

edited by metacpp

tombuildsstuff Aug 3, 2018 •

edited by metacpp

tombuildsstuff Aug 3, 2018 •

edited by metacpp

WodansSon Aug 4, 2018 •

edited by metacpp

tombuildsstuff Aug 6, 2018 •

edited by metacpp

tombuildsstuff Aug 8, 2018 •

edited by metacpp

tombuildsstuff Aug 6, 2018 •

edited by metacpp