fix(e2e): make test-e2e actually pass

.github/workflows/e2e.yml

@@ -14,6 +14,8 @@ on:
 jobs:
   e2e-pr-blocking:
     runs-on: ubuntu-22.04
+    env:
+      GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
     steps:
     - name: Checkout code
       uses: actions/checkout@v3

apis/infrastructure/v1beta1/byocluster_types.go

@@ -22,7 +22,7 @@ type ByoClusterSpec struct {
 	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint"`
 
 	// BundleLookupBaseRegistry is the base Registry URL that is used for pulling byoh bundle images,
-	// if not set, the default will be set to https://projects.registry.vmware.com/cluster_api_provider_bringyourownhost
+	// if not set, the default will be set to https://quay.io/platform9
 	// +optional
 	BundleLookupBaseRegistry string `json:"bundleLookupBaseRegistry,omitempty"`
 }

apis/infrastructure/v1beta1/byohost_webhook.go

@@ -25,8 +25,18 @@ type ByoHostValidator struct {
 	decoder *admission.Decoder
 }
 
-// To allow byoh manager service account to patch ByoHost CR
-const managerServiceAccount = "system:serviceaccount:kaapi:byoh-controller-manager"
+// The byoh-controller-manager's namespace differs by deployment: "byoh-system" is the OSS
+// default (config/default, e2e), "kaapi" is the PF9 production deployment. Both identities
+// are allowlisted to bypass the per-agent host-ownership check below.
+const (
+	kaapiManagerServiceAccount      = "system:serviceaccount:kaapi:byoh-controller-manager"
+	byohSystemManagerServiceAccount = "system:serviceaccount:byoh-system:byoh-controller-manager"
+)
+
+var managerServiceAccounts = map[string]bool{
+	kaapiManagerServiceAccount:      true,
+	byohSystemManagerServiceAccount: true,
+}
 
 // Precompile email-like regex for efficiency
 var emailLikeUserRegex = regexp.MustCompile(`^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$`)
@@ -55,7 +65,7 @@ func (v *ByoHostValidator) handleCreateUpdate(req *admission.Request) admission.
 	}
 	userName := req.UserInfo.Username
 	// allow manager service account to patch ByoHost
-	if userName == managerServiceAccount {
+	if managerServiceAccounts[userName] {
 		return admission.Allowed("")
 	}
 

apis/infrastructure/v1beta1/byohost_webhook_internal_test.go

@@ -134,7 +134,7 @@ var _ = Describe("ByohostWebhook/Unit", func() {
 		It("Should allow update request from manager", func() {
 			admissionRequest := admissionv1.AdmissionRequest{
 				Operation: admissionv1.Update,
-				UserInfo:  v1.UserInfo{Username: managerServiceAccount},
+				UserInfo:  v1.UserInfo{Username: byohSystemManagerServiceAccount},
 				Object: runtime.RawExtension{
 					Raw:    byoHostRaw,
 					Object: byoHost,
@@ -270,8 +270,13 @@ func TestByoHostValidator_handleCreateUpdate(t *testing.T) {
 		wantMsg   string
 	}{
 		{
-			name:      "manager service account bypasses the ownership check",
-			userName:  managerServiceAccount,
+			name:      "byoh-system manager service account bypasses the ownership check",
+			userName:  byohSystemManagerServiceAccount,
+			wantAllow: true,
+		},
+		{
+			name:      "kaapi manager service account bypasses the ownership check",
+			userName:  kaapiManagerServiceAccount,
 			wantAllow: true,
 		},
 		{

config/crd/bases/infrastructure.cluster.x-k8s.io_byoclusters.yaml

@@ -52,7 +52,7 @@ spec:
                 bundleLookupBaseRegistry:
                   description: |-
                     BundleLookupBaseRegistry is the base Registry URL that is used for pulling byoh bundle images,
-                    if not set, the default will be set to https://projects.registry.vmware.com/cluster_api_provider_bringyourownhost
+                    if not set, the default will be set to https://quay.io/platform9
                   type: string
                 controlPlaneEndpoint:
                   description: ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.

config/crd/bases/infrastructure.cluster.x-k8s.io_byoclustertemplates.yaml

@@ -81,7 +81,7 @@ spec:
                         bundleLookupBaseRegistry:
                           description: |-
                             BundleLookupBaseRegistry is the base Registry URL that is used for pulling byoh bundle images,
-                            if not set, the default will be set to https://projects.registry.vmware.com/cluster_api_provider_bringyourownhost
+                            if not set, the default will be set to https://quay.io/platform9
                           type: string
                         controlPlaneEndpoint:
                           description: ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.

config/manager/manager.yaml

@@ -28,10 +28,12 @@ spec:
         env:
         - name: MANUAL_CSR_APPROVAL
           value: "${MANUAL_CSR_APPROVAL:=disable}"
+        - name: BYOH_SKIP_KERNEL_MODULE_CLEANUP
+          value: "${BYOH_SKIP_KERNEL_MODULE_CLEANUP:=disable}"
         args:
         - --enable-leader-election
         - "--metrics-bind-addr=127.0.0.1:8080"
-        image: docker.io/psarwate/pf9-cluster-api-byoh-controller:dev 
+        image: gcr.io/k8s-staging-cluster-api/cluster-api-byoh-controller:dev
         name: manager
         resources:
           limits:

controllers/infrastructure/k8sinstallerconfig_controller.go

@@ -35,6 +35,12 @@ import (
 type K8sInstallerConfigReconciler struct {
 	client.Client
 	Scheme *runtime.Scheme
+	// SkipKernelModuleCleanup disables the overlay/br_netfilter kernel module
+	// unload step in the generated uninstall script. Real BYO hosts own their
+	// kernel and must unload these modules; e2e's containerized hosts share
+	// Docker's kernel, and unloading them there breaks Docker's own bridge
+	// networking and hangs cluster deletion.
+	SkipKernelModuleCleanup bool
 }
 
 // k8sInstallerConfigScope defines a scope defined around a K8sInstallerConfig and its ByoMachine
@@ -149,7 +155,7 @@ func (r *K8sInstallerConfigReconciler) reconcileNormal(ctx context.Context, scop
 
 	k8sVersion := scope.Config.GetAnnotations()[infrav1.K8sVersionAnnotation]
 	downloader := installer.NewBundleDownloader(scope.Config.Spec.BundleType, scope.Config.Spec.BundleRepo, "{{.BUNDLE_DOWNLOAD_PATH}}", logger)
-	installerObj, err := installer.NewInstaller(ctx, scope.ByoMachine.Status.HostInfo.OSImage, scope.ByoMachine.Status.HostInfo.Architecture, k8sVersion, downloader)
+	installerObj, err := installer.NewInstaller(ctx, scope.ByoMachine.Status.HostInfo.OSImage, scope.ByoMachine.Status.HostInfo.Architecture, k8sVersion, downloader, r.SkipKernelModuleCleanup)
 	if err != nil {
 		logger.Error(err, "failed to create installer instance", "osImage", scope.ByoMachine.Status.HostInfo.OSImage, "architecture", scope.ByoMachine.Status.HostInfo.Architecture, "k8sVersion", k8sVersion)
 		return ctrl.Result{}, err

go.mod

@@ -11,6 +11,7 @@ replace golang.org/x/net => golang.org/x/net v0.17.0
 require (
 	github.com/docker/cli v24.0.7+incompatible
 	github.com/docker/docker v24.0.7+incompatible
+	github.com/docker/go-units v0.5.0
 	github.com/go-logr/logr v1.4.3
 	github.com/jackpal/gateway v1.0.7
 	github.com/kube-vip/kube-vip v0.5.5
@@ -60,7 +61,6 @@ require (
 	github.com/docker/go v1.5.1-1.0.20160303222718-d30aec9fd63c // indirect
 	github.com/docker/go-connections v0.4.0 // indirect
 	github.com/docker/go-metrics v0.0.1 // indirect
-	github.com/docker/go-units v0.5.0 // indirect
 	github.com/drone/envsubst/v2 v2.0.0-20210730161058-179042472c46 // indirect
 	github.com/emicklei/go-restful/v3 v3.9.0 // indirect
 	github.com/evanphx/json-patch v5.6.0+incompatible // indirect

hack/configure-byoh-host-remote.sh

@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+#
+# Runs ON a BYO host — piped in over SSH by configure-byoh-host.sh. Loads the
+# kernel modules and sets the sysctls kubeadm requires, persisting both across
+# reboot. Idempotent. Uses sudo per-command; assumes passwordless sudo.
+
+set -Eeuo pipefail
+
+modules_conf=/etc/modules-load.d/byoh.conf
+sysctl_conf=/etc/sysctl.d/99-byoh.conf
+
+# Load the required modules now and persist them for boot.
+printf '%s\n' overlay br_netfilter | sudo tee "$modules_conf" >/dev/null
+for mod in overlay br_netfilter; do
+  sudo modprobe "$mod"
+done
+
+# Persist and apply the kubeadm-required sysctls.
+#
+# net.netfilter.nf_conntrack_max is global, not per-netns: kube-proxy inside
+# a privileged byoh host container can read it but gets "permission denied"
+# trying to raise it, so the host must already meet or exceed whatever value
+# kube-proxy computes (observed 524288; set well above it for headroom).
+sudo tee "$sysctl_conf" >/dev/null <<'SYSCTL'
+net.bridge.bridge-nf-call-iptables  = 1
+net.bridge.bridge-nf-call-ip6tables = 1
+net.ipv4.ip_forward                 = 1
+net.netfilter.nf_conntrack_max      = 1048576
+SYSCTL
+sudo sysctl --system >/dev/null
+
+# Report final state (|| true: display only; the modprobe above is the assertion).
+echo "modules:"
+lsmod | grep -E '^overlay|^br_netfilter' || true
+echo "sysctls:"
+sudo sysctl net.bridge.bridge-nf-call-iptables net.bridge.bridge-nf-call-ip6tables net.ipv4.ip_forward net.netfilter.nf_conntrack_max

hack/configure-byoh-host.sh

@@ -0,0 +1,59 @@
+#!/usr/bin/env bash
+#
+# configure-byoh-host.sh — enable the kernel/network prerequisites kubeadm needs
+# on a BYO host, applied over SSH against a target VM.
+#
+# When the workload cluster runs on a single machine, the privileged byoh/node
+# containers mount /lib/modules:ro and share the host kernel, so the host must
+# have the overlay + br_netfilter modules and the bridge-netfilter / ip_forward
+# sysctls loaded. The same script preps a standalone BYO host VM.
+#
+# The commands that run on the target live in configure-byoh-host-remote.sh (a
+# sibling file) so they can be linted independently; this script just pipes that
+# file to the host over SSH.
+#
+# Usage:
+#   hack/configure-byoh-host.sh <VM_IP>
+#
+# SSH user defaults to "ubuntu"; override with BYOH_SSH_USER.
+# Assumes passwordless sudo on the target.
+
+set -Eeuo pipefail
+shopt -s nullglob
+
+SSH_USER=${BYOH_SSH_USER:-ubuntu}
+
+log() { printf '%s %s\n' "$(date -u +%FT%TZ)" "$*"; }
+
+usage() {
+  cat >&2 <<'EOF'
+Usage: configure-byoh-host.sh <VM_IP>
+
+Loads overlay + br_netfilter and sets the kubeadm-required sysctls on the
+target host over SSH, persisting both across reboot. Idempotent.
+
+SSH user defaults to "ubuntu" (override with BYOH_SSH_USER).
+EOF
+}
+
+main() {
+  if [[ $# -ne 1 || "$1" == "-h" || "$1" == "--help" ]]; then
+    usage
+    exit 1
+  fi
+  local vm_ip="$1"
+
+  local script_dir remote_file
+  script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+  remote_file="${script_dir}/configure-byoh-host-remote.sh"
+  [[ -f "$remote_file" ]] || {
+    log "remote script not found: ${remote_file}"
+    exit 1
+  }
+
+  log "configuring byoh host prerequisites on ${SSH_USER}@${vm_ip}"
+  ssh "${SSH_USER}@${vm_ip}" 'bash -s' <"$remote_file"
+  log "done on ${vm_ip}: overlay/br_netfilter loaded, sysctls applied and persisted"
+}
+
+main "$@"

installer/installer.go

@@ -53,7 +53,7 @@ var archOldNameMap = map[string]string{
 }
 
 // NewInstaller will return a new installer
-func NewInstaller(ctx context.Context, osDist, arch, k8sVersion string, downloader *bundleDownloader) (K8sInstaller, error) {
+func NewInstaller(ctx context.Context, osDist, arch, k8sVersion string, downloader *bundleDownloader, skipKernelModuleCleanup bool) (K8sInstaller, error) {
 	bundleArchName := arch
 	// replacing the arch name to old name to match with the bundle name
 	if _, exists := archOldNameMap[arch]; exists {
@@ -74,9 +74,9 @@ func NewInstaller(ctx context.Context, osDist, arch, k8sVersion string, download
 	var err error
 
 	if strings.Contains(osbundle, "Ubuntu_22.04") {
-		installer, err = algo.NewUbuntu22_04Installer(ctx, arch, addrs)
+		installer, err = algo.NewUbuntu22_04Installer(ctx, arch, addrs, skipKernelModuleCleanup)
 	} else {
-		installer, err = algo.NewUbuntu20_04Installer(ctx, arch, addrs)
+		installer, err = algo.NewUbuntu20_04Installer(ctx, arch, addrs, skipKernelModuleCleanup)
 	}
 
 	if err != nil {

installer/installer_test.go

@@ -28,23 +28,23 @@ var _ = Describe("Byohost Installer Tests", func() {
 
 	Context("When installer object is created for valid OS and arch", func() {
 		It("should create the object successfully", func() {
-			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader)
+			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader, false)
 			Expect(err).ShouldNot(HaveOccurred())
 		})
 	})
 
 	Context("When installer object is created for invalid arch", func() {
 		It("should fail create the object", func() {
 			arch = "arm64"
-			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader)
+			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader, false)
 			Expect(err).To(MatchError(installer.ErrOsK8sNotSupported))
 		})
 	})
 
 	Context("When installer object is created for invalid OS", func() {
 		It("should fail create the object", func() {
 			os = "rhel"
-			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader)
+			_, err := installer.NewInstaller(context.TODO(), os, arch, k8sversion, downloader, false)
 			Expect(err).To(MatchError(installer.ErrOsK8sNotSupported))
 		})
 	})

installer/internal/algo/common_ubuntu.go

@@ -39,7 +39,7 @@ func (s *BaseUbuntuInstaller) Uninstall() string {
 }
 
 // NewBaseUbuntuInstaller creates a new base Ubuntu installer
-func NewBaseUbuntuInstaller(ctx context.Context, arch, bundleAddrs, containerdConfig string) (*BaseUbuntuInstaller, error) {
+func NewBaseUbuntuInstaller(ctx context.Context, arch, bundleAddrs, containerdConfig string, skipKernelModuleCleanup bool) (*BaseUbuntuInstaller, error) {
 	// Validate embedded templates
 	if commonUbuntuInstallTemplate == "" {
 		return nil, fmt.Errorf("install template is empty - template file may be missing")
@@ -48,12 +48,13 @@ func NewBaseUbuntuInstaller(ctx context.Context, arch, bundleAddrs, containerdCo
 		return nil, fmt.Errorf("uninstall template is empty - template file may be missing")
 	}
 
-	data := map[string]string{
-		"BundleAddrs":        bundleAddrs,
-		"Arch":               arch,
-		"ImgpkgVersion":      ImgpkgVersion,
-		"ContainerdConfig":   containerdConfig,
-		"BundleDownloadPath": "/var/lib/byoh/bundles",
+	data := map[string]interface{}{
+		"BundleAddrs":             bundleAddrs,
+		"Arch":                    arch,
+		"ImgpkgVersion":           ImgpkgVersion,
+		"ContainerdConfig":        containerdConfig,
+		"BundleDownloadPath":      "/var/lib/byoh/bundles",
+		"SkipKernelModuleCleanup": skipKernelModuleCleanup,
 	}
 
 	// Parse and validate templates

installer/internal/algo/common_ubuntu_test.go

@@ -0,0 +1,46 @@
+// Copyright 2022 VMware, Inc. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package algo_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/installer/internal/algo"
+)
+
+func TestBaseUbuntuInstallerUninstallKernelModuleCleanup(t *testing.T) {
+	testCases := []struct {
+		name                    string
+		skipKernelModuleCleanup bool
+		wantModprobeLine        bool
+	}{
+		{
+			name:                    "kernel modules unloaded when cleanup is not skipped",
+			skipKernelModuleCleanup: false,
+			wantModprobeLine:        true,
+		},
+		{
+			name:                    "kernel modules left alone when cleanup is skipped",
+			skipKernelModuleCleanup: true,
+			wantModprobeLine:        false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			installer, err := algo.NewBaseUbuntuInstaller(context.Background(), "amd64", "test-bundle", "", tc.skipKernelModuleCleanup)
+			require.NoError(t, err)
+
+			uninstallScript := installer.Uninstall()
+
+			hasModprobeLine := strings.Contains(uninstallScript, "modprobe -rq overlay")
+			assert.Equal(t, tc.wantModprobeLine, hasModprobeLine)
+		})
+	}
+}

installer/internal/algo/ubuntu-templates/uninstall.sh.tmpl

@@ -26,7 +26,7 @@ else
 fi
 
 ## remove kernal modules
-modprobe -rq overlay || true && modprobe -r br_netfilter || true
+{{if not .SkipKernelModuleCleanup}}modprobe -rq overlay || true && modprobe -r br_netfilter || true{{end}}
 
 ## restore firewall to its pre-install state
 if command -v ufw >>/dev/null; then

installer/internal/algo/ubuntu20_4k8s.go

@@ -13,8 +13,8 @@ type Ubuntu20_04Installer struct {
 }
 
 // NewUbuntu20_04Installer will return new Ubuntu20_04Installer instance
-func NewUbuntu20_04Installer(ctx context.Context, arch, bundleAddrs string) (*Ubuntu20_04Installer, error) {
-	base, err := NewBaseUbuntuInstaller(ctx, arch, bundleAddrs, "") // No special containerd config needed for 20.04
+func NewUbuntu20_04Installer(ctx context.Context, arch, bundleAddrs string, skipKernelModuleCleanup bool) (*Ubuntu20_04Installer, error) {
+	base, err := NewBaseUbuntuInstaller(ctx, arch, bundleAddrs, "", skipKernelModuleCleanup) // No special containerd config needed for 20.04
 	if err != nil {
 		return nil, err
 	}

installer/internal/algo/ubuntu22_04k8s.go

@@ -18,8 +18,8 @@ type Ubuntu22_04Installer struct {
 }
 
 // NewUbuntu22_04Installer will return new Ubuntu22_04Installer instance
-func NewUbuntu22_04Installer(ctx context.Context, arch, bundleAddrs string) (*Ubuntu22_04Installer, error) {
-	base, err := NewBaseUbuntuInstaller(ctx, arch, bundleAddrs, systemdCgroupConfig)
+func NewUbuntu22_04Installer(ctx context.Context, arch, bundleAddrs string, skipKernelModuleCleanup bool) (*Ubuntu22_04Installer, error) {
+	base, err := NewBaseUbuntuInstaller(ctx, arch, bundleAddrs, systemdCgroupConfig, skipKernelModuleCleanup)
 	if err != nil {
 		return nil, err
 	}