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.

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

Kubernetes AdvancedNetworking: Added validation for pod_cidr when network_plugin is set to 'azure'. #1763

Merged

Conversation

lfshr
Copy link
Contributor

@lfshr lfshr commented Aug 14, 2018

Kubernetes AdvancedNetworking: Added validation for pod_cidr when network_profile is set to 'azure'.

This fixes #1723

@lfshr lfshr changed the title Kubernetes AdvancedNetworking: Added validation for pod_cidr when network_profile is set to 'azure'. Kubernetes AdvancedNetworking: Added validation for pod_cidr when network_plugin is set to 'azure'. Aug 14, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

one minor 🤔 about the error message - but this otherwise LGTM 👍

podCidr := profile["pod_cidr"].(string)

if networkPlugin == "azure" && podCidr != "" {
return fmt.Errorf("`network_profile.pod_cidr` can not be used when `network_profile.network_plugin` is set to `azure`. Please remove `network_profile.pod_cidr` or set `network_profile.network_plugin` to `kubenet`.")
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 since this is a list containing a single item, would this be clearer by either making:

network_profile.pod_cidr -> network_profile.0.pod_cidr
network_profile.network_plugin -> network_profile.0.network_plugin

or by updating this to:

the `pod_cidr` field in the `network_profile` block can not be specified when `network_plugin` is set to `azure`. Please remove `pod_cidr` or set `network_plugin` to `kubenet`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't know about adding the index (mostly because when creating the terraform you don't really worry about the fact that it's a list so it makes it a little unclear)

I prefer your second option over all three. Thanks Tom!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this 👍

@tombuildsstuff tombuildsstuff merged commit 6daf5e2 into hashicorp:master Aug 14, 2018
@tombuildsstuff tombuildsstuff deleted the containerservices-podcidr-validation branch August 14, 2018 12:30
@tombuildsstuff tombuildsstuff added this to the 1.13.0 milestone Aug 14, 2018
tombuildsstuff added a commit that referenced this pull request Aug 14, 2018
@yves-vogl
Copy link
Contributor

Thanks!

@tombuildsstuff
Copy link
Contributor

hey @lfshr @yves-vogl

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!

@pedrohdz
Copy link
Contributor

Seems like there might be a bug in the validation. The validation fails when changing the network_plugin type from kubenet to azure on an existing azurerm_kubernetes_cluster resource.

The work around is to destroy the azurerm_kubernetes_cluster resource first, then create it again.

I can provide a Terraform plan to recreate the issue if needed.

@lfshr
Copy link
Contributor Author

lfshr commented Aug 16, 2018

@pedrohdz could you open up another issue and I'll take a look in a few days? I'm currently confined to bed rest 😞
@tombuildsstuff any inside knowledge into why this is happening? Also, do you happen to have a secret way to attach a debugger to the go file? (would be much appreciated if you did)

@pedrohdz
Copy link
Contributor

@lfshr,
Created #1783.

Hope you feel better!

@tombuildsstuff
Copy link
Contributor

@lfshr

I'm currently confined to bed rest 😞
hope you feel better soon :)

@tombuildsstuff any inside knowledge into why this is happening?

CustomizeDiff's is more designed for changing the Plan rather than validation (so we're stepping around the edges of it's intended scope) - so it could be worth reverting this and instead document this instead?

Also, do you happen to have a secret way to attach a debugger to the go file? (would be much appreciated if you did)

Due to the way Terraform and the Provider plugin works this is hard (since this is across process boundaries), but you should be able to debug Acceptance Tests using something like Goland if you can write one to match this?

@pedrohdz thanks for opening #1783 :)

Thanks!

@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pod_cidr seems to be ignored and set to the subnet of vnet_subnet_id
4 participants