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

PodCIDR patch for Overlay. #633

Merged
merged 10 commits into from
Nov 17, 2023
Merged

PodCIDR patch for Overlay. #633

merged 10 commits into from
Nov 17, 2023

Conversation

samaea
Copy link
Contributor

@samaea samaea commented Aug 10, 2023

PR Summary

Resolves #617. PodCIDR was not being passed when the network plugin is set to Overlay.

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not Work in Progress
  • Link to a filed issue
  • Screenshot of UI changes (if PR includes UI changes)

@samaea samaea enabled auto-merge (squash) August 10, 2023 13:18
@pjlewisuk
Copy link
Contributor

Hey @samaea
I've been looking at this today. When I try to deploy a cluster with CNI Overlay, I get the following error:

/workspaces/AKS-Construction/bicep/network.bicep(173,5) : Error BCP353: The variables "aks_podSubnet", "aks_podsubnet" differ only in casing. The ARM deployments engine is not case sensitive and will not be able to distinguish between them.
/workspaces/AKS-Construction/bicep/network.bicep(192,5) : Error BCP353: The variables "aks_podSubnet", "aks_podsubnet" differ only in casing. The ARM deployments engine is not case sensitive and will not be able to distinguish between them.
/workspaces/AKS-Construction/bicep/network.bicep(409,9) : Warning BCP334: The provided value can have a length as small as 0 and may be too short to assign to a target with a configured minimum length of 3.
/workspaces/AKS-Construction/bicep/main.bicep(130,16) : Error BCP104: The referenced module has errors.

The aks_podSubnet and aks_podsubnet appear in https://github.com/Azure/AKS-Construction/blob/se-podCIDR/bicep/network.bicep with different cases.

It looks like this change was introduced in early May in this commit, but I don't know why it's only showing as a problem now.

Suggestions for best way to proceed? @khowling for input too :)

@pjlewisuk
Copy link
Contributor

FWIW, I think the error is being triggered by one or more of these (URL) parameters, which get added when the "CNI Overlay" option is enabled:

net.networkPluginMode=true
net.vnetAksSubnetAddressPrefix=10.240.0.0%2F24
net.podCidr=10.244.0.0%2F16

This causes the podCidr=10.244.0.0/16 parameter to be added to the az deployment group create command. However, if I manually remove this parameter, the az deployment group create command still fails with the error shown above.

@pjlewisuk
Copy link
Contributor

My codespace was running bicep v0.21.1, Sam created the PR against v0.20.4. When I downgraded my codespace to bicep v0.21.1 the error went away, so seems this is related to the bicep version.

From the 0.21.1 release notes, this was called out as a bug that was fixed:
"Catch case-insensitive clashes of type property names (Azure/bicep#11457)"

Copy link
Contributor

@pjlewisuk pjlewisuk left a comment

Choose a reason for hiding this comment

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

Given the default configuration on the helper is to use the Azure Application Gateway Ingress Controller add-on, I don't think we should merge this into main until we have addressed #653

@pjlewisuk
Copy link
Contributor

Issue #653 has been address in PR #672. Will review this again once that PR has merged.

@pjlewisuk pjlewisuk removed the request for review from khowling November 17, 2023 06:39
Copy link
Contributor

@pjlewisuk pjlewisuk left a comment

Choose a reason for hiding this comment

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

LGTM, ready to merge

@samaea samaea merged commit 39f41a9 into main Nov 17, 2023
79 checks passed
@samaea samaea deleted the se-podCIDR branch November 17, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: AKS Construction does not pass necessary parameters for CNI Overlay networking
2 participants