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

Support for Azure CNI Overlay #354

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

zioproto
Copy link
Collaborator

Describe your changes

Support for Azure CNI Overlay
https://learn.microsoft.com/en-us/azure/aks/azure-cni-overlay

The product is now GA for Linux nodes only. The terraform preconditions take care of preventing invalid or preview configurations.

Issue number

Closes #319

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

@lonegunmanb
Copy link
Member

Hi @zioproto, thanks for opening this pr, it seems like network_plugin_mode feature is also in preview right? If so, I'm afraid we need defer this pr to its GA.

@lonegunmanb lonegunmanb removed this from the 7.1.0 milestone May 23, 2023
@zioproto
Copy link
Collaborator Author

The feature is GA:
Azure/AKS#3208
The Terraform docs are not updated.

What is in preview is the upgrade of a cluster from Azure CNI to Azure CNI Overlay

main.tf Outdated
}

precondition {
condition = (var.network_plugin_mode == "overlay" && !regex("^Standard_DC[0-9]+s?_v2$", each.value.vm_size)) || (var.network_plugin_mode != "overlay")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made a mistake here.
https://developer.hashicorp.com/terraform/language/functions/regex

I assumed regex would return false if it did not find a match, but instead regex returns an error.

@lonegunmanb do you think I can fix this using can() ?
https://developer.hashicorp.com/terraform/language/functions/can

Copy link
Member

Choose a reason for hiding this comment

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

Yes, can() function is designed for such scenarios.

@zioproto
Copy link
Collaborator Author

Updating provider docs here:
hashicorp/terraform-provider-azurerm#21917

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @zioproto for the update, a few comments.

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Outdated
@@ -678,6 +678,12 @@ variable "network_plugin" {
nullable = false
}

variable "network_plugin_mode" {
type = string
description = "(Optional) Specifies the network plugin mode used for building the Kubernetes network. Possible value is Overlay. Changing this forces a new resource to be created."
Copy link
Member

Choose a reason for hiding this comment

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

Could be:

description = "(Optional) Specifies the network plugin mode used for building the Kubernetes network. Possible value is `Overlay`. Changing this forces a new resource to be created."

@zioproto
Copy link
Collaborator Author

I pushed I new commit.

I also corrected overlay to Overlay in all the preconditions

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

We've got the following error in e2e test:

TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ Error: creating/updating "Resource: (ResourceId \"/subscriptions/xxx/resourceGroups/c91104ca6615093b-rg/providers/Microsoft.ContainerService/managedClusters/test-cluster\" / Api Version \"2023-01-02-preview\")": PUT https://management.azure.com/subscriptions/xxx/resourceGroups/c91104ca6615093b-rg/providers/Microsoft.ContainerService/managedClusters/test-cluster
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ --------------------------------------------------------------------------------
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ RESPONSE 409: 409 Conflict
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ ERROR CODE: EtagMismatch
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ --------------------------------------------------------------------------------
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ {
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   "code": "EtagMismatch",
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   "details": [
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │     {
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │       "code": "Unspecified",
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │       "message": "rpc error: code = FailedPrecondition desc = Etag mismatched"
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │     }
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   ],
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   "message": "Failed to save managed cluster test-cluster in subscription xxx, resource group c91104ca6615093b-rg. Operation is not allowed: Another operation is in progress",
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   "subcode": "PutManagedClusterAndAgentPools_FailedPrecondition"
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ }
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ --------------------------------------------------------------------------------
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ 
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ 
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   with module.aks_cluster_name.azapi_update_resource.aks_cluster_post_create,
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │   on ../../main.tf line 468, in resource "azapi_update_resource" "aks_cluster_post_create":
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │  468: resource "azapi_update_resource" "aks_cluster_post_create" {
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: │ 
TestExamplesNamedCluster 2023-05-25T02:01:28Z command.go:185: ╵

Need further investigation.

@zioproto
Copy link
Collaborator Author

@lonegunmanb can we run the e2e tests again please ?

@lonegunmanb
Copy link
Member

Hi @zioproto the pr failed version upgrade test, would you please merge your branch with the latest main branch? Thanks!

@zioproto zioproto temporarily deployed to acctests May 31, 2023 08:33 — with GitHub Actions Inactive
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks for the update @zioproto, LGTM! 🚀

@lonegunmanb lonegunmanb merged commit 724e3b8 into Azure:main Jun 1, 2023
4 checks passed
skolobov pushed a commit to skolobov/terraform-azurerm-aks that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for network_plugin_mode
2 participants