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

azure: fix BYOCNI install with Cilium >= 1.12 #953

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

nbusseneau
Copy link
Member

@nbusseneau nbusseneau commented Jul 1, 2022

We introduced support for BYOCNI with the aks-byocni datapath mode in 2e1d3b5, and properly guarded the previously existing Service Principal creation to only be done when using Azure IPAM (azure datapath mode), however we forgot to do the same for the newly-introduced AKS secrets in the Cilium 1.12 Helm chart.

This went undetected during the initial implementation as the Helm chart used was either the stable 1.11.5 or the development version 1.11.90 when directly using the Helm chart from upstream master branch, in both cases skipping the existing 1.12 check for creating AKS secrets.

Now, after the 1.12 branch was properly created and the development version bumped to 1.12.90, this does not work anymore, and explains why the AKS BYOCNI CI in upstream was initially working but then started to fail after the version bump.

@nbusseneau nbusseneau requested a review from a team as a code owner July 1, 2022 16:33
@nbusseneau nbusseneau requested a review from ldelossa July 1, 2022 16:33
@nbusseneau nbusseneau temporarily deployed to ci July 1, 2022 16:33 Inactive
We introduced support for BYOCNI with the `aks-byocni` datapath mode in
2e1d3b5, and properly guarded the
previously existing Service Principal creation to only be done when
using Azure IPAM (`azure` datapath mode), however we forgot to do the
same for the newly-introduced AKS secrets in the Cilium 1.12 Helm chart.

This went undetected during the initial implementation as the
Helm chart used was either the stable 1.11.5 or the development version
1.11.90 when directly using the Helm chart from upstream `master`
branch, in both cases skipping the existing 1.12 check for creating AKS
secrets.

Now, after the 1.12 branch was properly created and the development
version bumped to 1.12.90, this does not work anymore, and explains why
the AKS BYOCNI CI in upstream was initially working but then started to
fail after the version bump.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau
Copy link
Member Author

nbusseneau commented Jul 1, 2022

Before fix (install/kubernetes/cilium == Helm chart from upstream master branch, ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 == current upstream master branch):

~/.local/bin/cilium install \
  --chart-directory=install/kubernetes/cilium \
  --agent-image=quay.io/cilium/cilium-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --operator-image=quay.io/cilium/operator-generic-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --version=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --azure-resource-group nicolas-test2 \
  --wait=false \
  --rollback=false \
  --config monitor-aggregation=none \
  --helm-set=debug.enabled=true
🔮 Auto-detected Kubernetes kind: AKS
✨ Running "AKS" validation checks
✅ Detected az binary
ℹ️  Using Cilium version 1.12.90
🔮 Auto-detected cluster name: nicolas-test2
✅ Derived Azure subscription ID 22716d91-fb67-4a07-ac5f-d36ea49d6167 from subscription cilium-dev
✅ Detected Azure AKS cluster in BYOCNI mode (no CNI plugin pre-installed)
🔮 Auto-detected datapath mode: aks-byocni
ℹ️  helm template --namespace kube-system cilium "install/kubernetes/cilium" --version 1.12.90 --set azure.resourceGroup=nicolas-test2,cluster.id=0,cluster.name=nicolas-test2,clustermesh.apiserver.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,clustermesh.apiserver.image.useDigest=false,debug.enabled=true,encryption.nodeEncryption=false,extraConfig.monitor-aggregation=none,hubble.relay.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,hubble.relay.image.useDigest=false,image.override=quay.io/cilium/cilium-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,image.useDigest=false,ipam.mode=cluster-pool,kubeProxyReplacement=disabled,nodeinit.enabled=true,operator.image.override=quay.io/cilium/operator-generic-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,operator.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,operator.image.useDigest=false,operator.replicas=1,preflight.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,preflight.image.useDigest=false,serviceAccounts.cilium.name=cilium,serviceAccounts.operator.name=cilium-operator,tunnel=vxlan
ℹ️  Storing helm values file in kube-system/cilium-cli-helm-values Secret
🔑 Generated AKS secret cilium-azure
ℹ️  Rollback disabled with '--rollback=false', leaving installed resources behind

Error: Unable to install Cilium: unable to create AKS secret kube-system/cilium-azure: Secret "" is invalid: metadata.name: Required value: name or generateName is required

After fix:

~/.local/bin/cilium install \   
  --chart-directory=install/kubernetes/cilium \
  --agent-image=quay.io/cilium/cilium-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --operator-image=quay.io/cilium/operator-generic-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --version=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4 \
  --azure-resource-group nicolas-test2 \
  --wait=false \
  --rollback=false \
  --config monitor-aggregation=none \
  --helm-set=debug.enabled=true
🔮 Auto-detected Kubernetes kind: AKS
✨ Running "AKS" validation checks
✅ Detected az binary
ℹ️  Using Cilium version 1.12.90
🔮 Auto-detected cluster name: nicolas-test2
✅ Derived Azure subscription ID 22716d91-fb67-4a07-ac5f-d36ea49d6167 from subscription cilium-dev
✅ Detected Azure AKS cluster in BYOCNI mode (no CNI plugin pre-installed)
🔮 Auto-detected datapath mode: aks-byocni
ℹ️  helm template --namespace kube-system cilium "install/kubernetes/cilium" --version 1.12.90 --set azure.resourceGroup=nicolas-test2,cluster.id=0,cluster.name=nicolas-test2,clustermesh.apiserver.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,clustermesh.apiserver.image.useDigest=false,debug.enabled=true,encryption.nodeEncryption=false,extraConfig.monitor-aggregation=none,hubble.relay.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,hubble.relay.image.useDigest=false,image.override=quay.io/cilium/cilium-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,image.useDigest=false,ipam.mode=cluster-pool,kubeProxyReplacement=disabled,nodeinit.enabled=true,operator.image.override=quay.io/cilium/operator-generic-ci:ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,operator.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,operator.image.useDigest=false,operator.replicas=1,preflight.image.tag=ae43e20c2defb23b2c4c16efdec9f26aa8aec1c4,preflight.image.useDigest=false,serviceAccounts.cilium.name=cilium,serviceAccounts.operator.name=cilium-operator,tunnel=vxlan
ℹ️  Storing helm values file in kube-system/cilium-cli-helm-values Secret
🔑 Created CA in secret cilium-ca
🔑 Generating certificates for Hubble...
🚀 Creating Service accounts...
🚀 Creating Cluster roles...
🚀 Creating ConfigMap for Cilium version 1.12.90...
ℹ️ Manual overwrite in ConfigMap: monitor-aggregation=none
🚀 Creating AKS Node Init DaemonSet...
🚀 Creating Agent DaemonSet...
🚀 Creating Operator Deployment...
⌛ Waiting for Cilium to be installed and ready...
✅ Cilium was successfully installed! Run 'cilium status' to view installation health

@nbusseneau nbusseneau force-pushed the pr/fix-aks-byocni-with-1-12 branch from 93f1747 to 2c9d4bf Compare July 5, 2022 13:54
@nbusseneau nbusseneau temporarily deployed to ci July 5, 2022 13:54 Inactive
@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 5, 2022
@nbusseneau
Copy link
Member Author

Fixed typos in git commit message, no changes otherwise, marking as ready-to-merge.

@tklauser tklauser merged commit 3ad7321 into master Jul 6, 2022
@tklauser tklauser deleted the pr/fix-aks-byocni-with-1-12 branch July 6, 2022 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants