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

r/kubernetes_cluster: info on unexpected errors #1197

Merged
merged 4 commits into from
May 6, 2018
Merged

Conversation

tombuildsstuff
Copy link
Contributor

@tombuildsstuff tombuildsstuff commented May 3, 2018

This PR cleans up a few rough edges in the Kubernetes resource:

  • Whilst using the Kubernetes Cluster resource, I noticed there's an undocumented regex on the Agent Pool names - as such this PR adds validation to that affect (needs to start with a lower-case char, and be up to 12 chars of only a-z0-9. At the moment this fails with the unhelpful error message:
* azurerm_kubernetes_cluster.test: containerservice.ManagedClustersClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="" Message=""
  • Documenting there's a known issue with routing on nodes attached to an internal subnet
  • Documenting how to use the outputs with the Kubernetes Provider

@tombuildsstuff
Copy link
Contributor Author

@dcaro as discussed yesterday, would you mind confirming the statement about Subnets is correct?

@tombuildsstuff tombuildsstuff requested a review from a team May 3, 2018 17:28
@tombuildsstuff tombuildsstuff added this to the 1.5.0 milestone May 3, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

LGTM after fixing the doc typo 👍

@@ -148,6 +150,19 @@ The following attributes are exported:

* `password` - A password or token used to authenticate to the Kubernetes cluster.

-> **NOTE:** You can use this It's possible to use this information with [the Kubernetes Provider](/docs/providers/kubernetes/index.html) like so:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo here 😄 You can use this It's possible to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ good spot.

@tombuildsstuff
Copy link
Contributor Author

Associated test:

$ acctests azurerm TestAccAzureRMKubernetesCluster_agentPoolName
=== RUN   TestAccAzureRMKubernetesCluster_agentPoolName
--- PASS: TestAccAzureRMKubernetesCluster_agentPoolName (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	0.032s

@@ -119,6 +119,8 @@ The following arguments are supported:
* `os_type` - (Optional) The Operating System used for the Agents. Possible values are `Linux` and `Windows`. Changing this forces a new resource to be created. Defaults to `Linux`.
* `vnet_subnet_id` - (Optional) The ID of the Subnet where the Agents in the Pool should be provisioned. Changing this forces a new resource to be created.

~> **NOTE:** There's a known issue where Agents connected to an Internal Network (e.g. on a Subnet) have their network routing configured incorrectly; such that Pods cannot be located across nodes. This is a bug in the Azure API - and will be fixed there in the future.
Copy link
Contributor

@dcaro dcaro May 4, 2018

Choose a reason for hiding this comment

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

Suggested change: There's a known issue where Agents connected to a Network (e.g. on a Subnet) not provisioned as part of the creation of the AKS cluster have their network routing configured incorrectly; such that Pods cannot communicate across nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so does the issue only manifest when attaching an existing cluster to an internal subnet? at this time the resource requires a recreation for this kind of change

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue happens when you attach an existing or new cluster to an existing vnet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the vnet_subnet_id is ForceNew, this can only affect New clusters from within Terraform; so I think we're covered on that front. I'll update to include the comment about communication 👍

@tombuildsstuff tombuildsstuff merged commit 182b29c into master May 6, 2018
@tombuildsstuff tombuildsstuff deleted the docs-aks branch May 6, 2018 14:30
tombuildsstuff added a commit that referenced this pull request May 6, 2018
* Documenting some known issues with the vnet_subnet_id field

* Validating the agent pool name matches the regex in the error message from the API

* Fixing a typo

* Better explaining the routing issue
@exceptorr
Copy link

@tombuildsstuff Hello!

Documenting there's a known issue with routing on nodes attached to an internal subnet

Could you please share more info (maybe link to AKS issue tracker?) We've run into issue that looks similar - just want to ensure this is the same as issue described in Terraform's documentation.

Thanks!

@tombuildsstuff
Copy link
Contributor Author

👋 @exceptorr

Documenting there's a known issue with routing on nodes attached to an internal subnet

Could you please share more info (maybe link to AKS issue tracker?) We've run into issue that looks similar - just want to ensure this is the same as issue described in Terraform's documentation.

This came up in a conversation with @dcaro who should be able to give more information about it; I believe this may be fixed in the new non-Preview API version but I may be wrong, Damien should be able to give more information.

Thanks!

@dcaro
Copy link
Contributor

dcaro commented Jun 19, 2018

Thanks @exceptorr It was the case during the preview of AKS, with the GA of AKS last week using custom VNET is now supported as per https://docs.microsoft.com/en-us/azure/aks/networking-overview.
@tombuildsstuff we should update the documentation accordingly.

@tombuildsstuff
Copy link
Contributor Author

@dcaro I’m assuming that’s still going to be an issue for us until we upgrade to the GA API? I’ve asked the SDK team if they can ship this in the next release, which we can upgrade to when that’s out

@dcaro
Copy link
Contributor

dcaro commented Jun 19, 2018

Yes, we'll have to wait until the next release of the SDK.

@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.

4 participants