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

Update validation logic for kubenet network plugin. #1715

Merged
merged 10 commits into from
Aug 9, 2018
Merged

Update validation logic for kubenet network plugin. #1715

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0542c72
Remove validation logic for kubenet network plugin.
metacpp Aug 3, 2018
6397658
:bug: (Azure Kubernete resource.) Update the validation logic of netw…
metacpp Aug 3, 2018
073706d
:bug: (data source of AKS.) Fix the compiling error of tests of data …
metacpp Aug 3, 2018
104a9f0
:bug: (AKS resource.) Format error.
metacpp Aug 3, 2018
cf1401d
:memo: (AKS resource documentation.) Fix the error messsage for netwo…
metacpp Aug 4, 2018
386080d
:recycle: (AKS resource.) Refactor the validation logic throuhg remov…
metacpp Aug 7, 2018
8cd7bbc
:recycle: (Test code of AKS resource.) Rename test case name.
metacpp Aug 7, 2018
1b8e35a
:recycle: (AKS resource testing.) Refactor the test case configuratio…
metacpp Aug 8, 2018
625b9f2
:bug: (AKS data source testing.) The wrong arguments passed in.
metacpp Aug 8, 2018
4502449
Fixing the outstanding PR comments
tombuildsstuff Aug 9, 2018
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
24 changes: 0 additions & 24 deletions azurerm/resource_arm_kubernetes_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,6 @@ func resourceArmKubernetesCluster() *schema.Resource {
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},
CustomizeDiff: func(diff *schema.ResourceDiff, v interface{}) error {
if v, exists := diff.GetOk("network_profile"); exists {
rawProfiles := v.([]interface{})
if len(rawProfiles) == 0 {
return nil
}

// then ensure the conditionally-required fields are set
profile := rawProfiles[0].(map[string]interface{})
networkPlugin := profile["network_plugin"].(string)

if networkPlugin == "kubenet" {
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both azure and kubenet need to be taken care.

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

dockerBridgeCidr := profile["docker_bridge_cidr"].(string)
dnsServiceIP := profile["dns_service_ip"].(string)
serviceCidr := profile["service_cidr"].(string)

if dockerBridgeCidr == "" || dnsServiceIP == "" || serviceCidr == "" {
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

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.")
}
}
}

return nil
},

Schema: map[string]*schema.Schema{
"name": {
Expand Down
10 changes: 2 additions & 8 deletions azurerm/resource_arm_kubernetes_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,12 +461,9 @@ resource "azurerm_kubernetes_cluster" "test" {
client_id = "%s"
client_secret = "%s"
}

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"
}
}
`, rInt, location, rInt, rInt, rInt, rInt, rInt, clientId, clientSecret)
Expand Down Expand Up @@ -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"
}
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

}
`, rInt, location, rInt, rInt, rInt, rInt, rInt, clientId, clientSecret)
Expand Down Expand Up @@ -585,7 +579,7 @@ resource "azurerm_kubernetes_cluster" "test" {
client_id = "%s"
client_secret = "%s"
}

network_profile {
network_plugin = "azure"
}
Expand Down
14 changes: 5 additions & 9 deletions website/docs/r/kubernetes_cluster.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,19 +217,19 @@ The following arguments are supported:

`network_profile` supports the following:

* `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.
* `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.

-> **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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field can only bet set with other 2 fields.

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


~> **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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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

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

* `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.
Copy link
Contributor

@tombuildsstuff tombuildsstuff Aug 3, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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

Copy link

@evillgenius75 evillgenius75 Aug 6, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link

@evillgenius75 evillgenius75 Aug 7, 2018 •
edited by metacpp
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's take it offline.

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

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

* `pod_cidr` - (Optional) The CIDR to use for pod IP addresses. Changing this forces a new resource to be created.
* `pod_cidr` - (Optional) The CIDR to use for pod IP addresses. This field can only be set when `network_plugin` is set to `kubenet`. Changing this forces a new resource to be created.

Here's an example of configuring the `kubenet` Networking Profile:

Expand All @@ -248,10 +248,6 @@ resource "azurerm_kubernetes_cluster" "test" {

network_profile {
network_plugin = "kubenet"
pod_cidr = "10.244.0.0/24"
dns_service_ip = "10.10.0.10"
docker_bridge_cidr = "172.17.0.1/16"
service_cidr = "10.10.0.0/16"
}
}
```
Expand Down