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 Cilium #398

Merged
merged 4 commits into from
Jul 7, 2023
Merged

Support for Azure CNI Cilium #398

merged 4 commits into from
Jul 7, 2023

Conversation

JitseHijlkema
Copy link
Contributor

@JitseHijlkema JitseHijlkema commented Jun 21, 2023

Describe your changes

Support for Azure CNI - Cilium

Azure CNI powered by Cilium in now generally available.
Cilium is a comprehensive networking solution that combines the control plane of Azure CNI with the dataplane capabilities of Cilium.

Issue number

Closes #397

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

@JitseHijlkema
Copy link
Contributor Author

@microsoft-github-policy-service agree

@zioproto
Copy link
Collaborator

Please run the tests locally as described in the Readme:
https://github.com/Azure/terraform-azurerm-aks#pre-commit--pr-check--test

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 @JitseHijlkema for opening this pr! Some comments.

main.tf Outdated
error_message = "When ebpf_data_plane is set to cilium, the network_plugin field can only be set to azure."
}
precondition {
condition = !(var.ebpf_data_plane == "cilium") || (var.network_plugin_mode == "Overlay" && var.pod_subnet_id == null || var.network_plugin_mode == null && var.pod_subnet_id != null)
Copy link
Member

Choose a reason for hiding this comment

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

According to the message, I think we can simplify the condition to:

condition     = (var.ebpf_data_plane != "cilium") || (var.network_plugin_mode == "Overlay" || var.pod_subnet_id != null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly modified, to still comply to 'one of either'

variables.tf Outdated
variable "ebpf_data_plane" {
type = string
default = null
description = "(Optional) Specifies the eBPF data plane used for building the Kubernetes network."
Copy link
Member

Choose a reason for hiding this comment

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

Description could be

(Optional) Specifies the eBPF data plane used for building the Kubernetes network. Possible value is `cilium`. Changing this forces a new resource to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, modified :)

main.tf Outdated
@@ -483,6 +484,14 @@ resource "azurerm_kubernetes_cluster" "main" {
condition = var.network_plugin_mode != "Overlay" || var.network_plugin == "azure"
error_message = "When network_plugin_mode is set to Overlay, the network_plugin field can only be set to azure."
}
precondition {
condition = !(var.ebpf_data_plane == "cilium" && var.network_plugin != "azure")
Copy link
Member

Choose a reason for hiding this comment

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

Could we simplify the condition to var.ebpf_data_plan != "cilium" || var.network_plugin == "azure"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, modified :)

main.tf Outdated
error_message = "When ebpf_data_plane is set to cilium, the network_plugin field can only be set to azure."
}
precondition {
condition = (var.ebpf_data_plane != "cilium") || (var.network_plugin_mode == "Overlay" && var.pod_subnet_id == null || var.pod_subnet_id != null && var.network_plugin_mode == null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can have var.network_plugin_mode == "Overlay" and specify a value for example var.pod_subnet_id == 10.100.0.0/16. This is a valid configuration with Cilium but the current precondition would not allow it.

Can you please double check ?

Would it work this one? :

(var.ebpf_data_plane != "cilium") || var.network_plugin_mode == "Overlay" || var.pod_subnet_id != null

Also here you are just checking var.pod_subnet_id for the default node pool, but the additional nodepools are not part for the precondition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I updated the precondition.

@JitseHijlkema JitseHijlkema temporarily deployed to acctests July 7, 2023 02:58 — 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 @JitseHijlkema for the update, LGTM!

@lonegunmanb lonegunmanb merged commit 56d9235 into Azure:main Jul 7, 2023
skolobov pushed a commit to skolobov/terraform-azurerm-aks that referenced this pull request Oct 29, 2023
* Support for Azure CNI Cilium
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 Azure CNI - Cilium
3 participants