From be37633f8b3eafbc7647c2c4a40df675cb5f8080 Mon Sep 17 00:00:00 2001 From: Konstantinos Karampogias Date: Wed, 26 Nov 2025 16:53:07 +0100 Subject: [PATCH 1/4] Allowlist controller: refactor names in controller code Rename constants and functions to make it easier for new users to read the code. Variables within the package do not need to be prefixed with "allowlist" since the package context already provides that scope. Also name in GO should not use ALL_CAPS but use CamelCase instead. Assisted-By: Claude Signed-off-by: Konstantinos Karampogias --- .../allowlist/allowlist_controller.go | 72 +++++++++---------- pkg/names/names.go | 8 +-- 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/pkg/controller/allowlist/allowlist_controller.go b/pkg/controller/allowlist/allowlist_controller.go index ee05ce437e..a4afcf14b4 100644 --- a/pkg/controller/allowlist/allowlist_controller.go +++ b/pkg/controller/allowlist/allowlist_controller.go @@ -1,3 +1,5 @@ +// Package allowlist implements a Kubernetes controller that distributes CNI +// sysctl allowlist configuration to cluster nodes. package allowlist import ( @@ -36,21 +38,15 @@ import ( ) const ( - allowlistDsName = "cni-sysctl-allowlist-ds" - allowlistAnnotation = "app=cni-sysctl-allowlist-ds" - manifestDir = "../../bindata/allowlist/daemonset" - allowlistManifestDir = "../../bindata/network/multus/004-sysctl-configmap.yaml" + dsName = "cni-sysctl-allowlist-ds" + dsAnnotation = "app=cni-sysctl-allowlist-ds" + dsManifestDir = "../../bindata/allowlist/daemonset" + // Note: The default values come from default-cni-sysctl-allowlist which multus creates. + defaultCMManifest = "../../bindata/network/multus/004-sysctl-configmap.yaml" ) -func Add(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client, _ featuregates.FeatureGate) error { - return add(mgr, newReconciler(mgr, status, c)) -} - -func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client) *ReconcileAllowlist { - return &ReconcileAllowlist{client: c, scheme: mgr.GetScheme(), status: status} -} - -func add(mgr manager.Manager, r *ReconcileAllowlist) error { +func Add(mgr manager.Manager, status *statusmanager.StatusManager, client cnoclient.Client, _ featuregates.FeatureGate) error { + r:= &ReconcileAllowlist{client: client, status: status} c, err := controller.New("allowlist-controller", mgr, controller.Options{Reconciler: r}) if err != nil { return err @@ -59,7 +55,7 @@ func add(mgr manager.Manager, r *ReconcileAllowlist) error { // watch for changes in all configmaps in our namespace cmInformer := v1coreinformers.NewConfigMapInformer( r.client.Default().Kubernetes(), - names.MULTUS_NAMESPACE, + names.MultusNamespace, 0, // don't resync cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) @@ -73,7 +69,8 @@ func add(mgr manager.Manager, r *ReconcileAllowlist) error { predicate.NewPredicateFuncs(func(object crclient.Object) bool { // Only care about cni-sysctl-allowlist, but also watching for default-cni-sysctl-allowlist // as a trigger for creating cni-sysctl-allowlist if it doesn't exist - return (strings.Contains(object.GetName(), names.ALLOWLIST_CONFIG_NAME)) + // NOTE: the cni-sysctl-allowlist is hardcoded in pkg/network/multus.go:91 + return (strings.Contains(object.GetName(), names.AllowlistConfigName)) }), }, }) @@ -89,8 +86,8 @@ type ReconcileAllowlist struct { func (r *ReconcileAllowlist) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { defer utilruntime.HandleCrash(r.status.SetDegradedOnPanicAndCrash) - if exists, err := daemonsetConfigExists(ctx, r.client); !exists { - err = createObjects(ctx, r.client, allowlistManifestDir) + if exists, err := allowlistConfigMapExists(ctx, r.client); !exists { + err = createObjectsFrom(ctx, r.client, defaultCMManifest) if err != nil { klog.Errorf("Failed to create allowlist config map: %v", err) return reconcile.Result{}, err @@ -100,23 +97,27 @@ func (r *ReconcileAllowlist) Reconcile(ctx context.Context, request reconcile.Re return reconcile.Result{}, err } - if request.Name != names.ALLOWLIST_CONFIG_NAME { + if request.Name != names.AllowlistConfigName { return reconcile.Result{}, nil } klog.Infof("Reconcile allowlist for %s/%s", request.Namespace, request.Name) - configMap, err := getConfig(ctx, r.client, request.NamespacedName) + configMap, err := getConfigMap(ctx, r.client, request.NamespacedName) if err != nil { klog.Errorf("Failed to get config map: %v", err) return reconcile.Result{}, err } + // Deletion handling: If user deletes the ConfigMap, we do nothing. + // The allowlist file persists on nodes and pods continue working. + // The auto-create check above will recreate the ConfigMap on next reconcile. + // This prevents accidental deletion from breaking pod creation. // No action to be taken if user deletes the config map. The sysctl's will stay unmodified until config map is recreated if configMap == nil { return reconcile.Result{}, nil } - defer cleanup(ctx, r.client) + defer cleanupDaemonSet(ctx, r.client) // If daemonset still exists, delete it and reconcile again ds, err := getDaemonSet(ctx, r.client) @@ -129,7 +130,7 @@ func (r *ReconcileAllowlist) Reconcile(ctx context.Context, request reconcile.Re return reconcile.Result{}, errors.New("retrying") } - err = createObjects(ctx, r.client, manifestDir) + err = createObjectsFrom(ctx, r.client, dsManifestDir) if err != nil { klog.Errorf("Failed to create allowlist daemonset: %v", err) return reconcile.Result{}, err @@ -150,17 +151,16 @@ func (r *ReconcileAllowlist) Reconcile(ctx context.Context, request reconcile.Re return reconcile.Result{}, nil } -func createObjects(ctx context.Context, client cnoclient.Client, manifestDir string) error { +func createObjectsFrom(ctx context.Context, client cnoclient.Client, manifestPath string) error { data := render.MakeRenderData() data.Data["MultusImage"] = os.Getenv("MULTUS_IMAGE") - data.Data["CniSysctlAllowlist"] = names.ALLOWLIST_CONFIG_NAME + data.Data["CniSysctlAllowlist"] = names.AllowlistConfigName data.Data["ReleaseVersion"] = os.Getenv("RELEASE_VERSION") - manifests, err := render.RenderDir(manifestDir, &data) + manifests, err := render.RenderDir(manifestPath, &data) if err != nil { return err } for _, obj := range manifests { - err = createObject(ctx, client, obj) if err != nil { return err @@ -169,7 +169,7 @@ func createObjects(ctx context.Context, client cnoclient.Client, manifestDir str return nil } -func getConfig(ctx context.Context, client cnoclient.Client, namespacedName types.NamespacedName) (*corev1.ConfigMap, error) { +func getConfigMap(ctx context.Context, client cnoclient.Client, namespacedName types.NamespacedName) (*corev1.ConfigMap, error) { configMap := &corev1.ConfigMap{} err := client.Default().CRClient().Get(ctx, namespacedName, configMap) if err != nil { @@ -199,8 +199,8 @@ func checkDsPodsReady(ctx context.Context, client cnoclient.Client) error { return false, fmt.Errorf("failed to get UID of daemon set") } - podList, err := client.Default().Kubernetes().CoreV1().Pods(names.MULTUS_NAMESPACE).List( - ctx, metav1.ListOptions{LabelSelector: allowlistAnnotation}) + podList, err := client.Default().Kubernetes().CoreV1().Pods(names.MultusNamespace).List( + ctx, metav1.ListOptions{LabelSelector: dsAnnotation}) if err != nil { return false, err } @@ -223,7 +223,7 @@ func checkDsPodsReady(ctx context.Context, client cnoclient.Client) error { }) } -func cleanup(ctx context.Context, client cnoclient.Client) { +func cleanupDaemonSet(ctx context.Context, client cnoclient.Client) { ds, err := getDaemonSet(ctx, client) if err != nil { klog.Errorf("Error looking up allowlist daemonset : %+v", err) @@ -238,8 +238,8 @@ func cleanup(ctx context.Context, client cnoclient.Client) { } func deleteDaemonSet(ctx context.Context, client cnoclient.Client) error { - err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MULTUS_NAMESPACE).Delete( - ctx, allowlistDsName, metav1.DeleteOptions{}) + err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MultusNamespace).Delete( + ctx, dsName, metav1.DeleteOptions{}) if err != nil { return err } @@ -247,8 +247,8 @@ func deleteDaemonSet(ctx context.Context, client cnoclient.Client) error { } func getDaemonSet(ctx context.Context, client cnoclient.Client) (*appsv1.DaemonSet, error) { - ds, err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MULTUS_NAMESPACE).Get( - ctx, allowlistDsName, metav1.GetOptions{}) + ds, err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MultusNamespace).Get( + ctx, dsName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { return nil, nil @@ -258,9 +258,9 @@ func getDaemonSet(ctx context.Context, client cnoclient.Client) (*appsv1.DaemonS return ds, nil } -func daemonsetConfigExists(ctx context.Context, client cnoclient.Client) (bool, error) { - cm, err := client.Default().Kubernetes().CoreV1().ConfigMaps(names.MULTUS_NAMESPACE).Get( - ctx, names.ALLOWLIST_CONFIG_NAME, metav1.GetOptions{}) +func allowlistConfigMapExists(ctx context.Context, client cnoclient.Client) (bool, error) { + cm, err := client.Default().Kubernetes().CoreV1().ConfigMaps(names.MultusNamespace).Get( + ctx, names.AllowlistConfigName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { return false, nil diff --git a/pkg/names/names.go b/pkg/names/names.go index db0d7bf93f..c2b34451b9 100644 --- a/pkg/names/names.go +++ b/pkg/names/names.go @@ -27,13 +27,13 @@ const APPLIED_PREFIX = "applied-" // Should match 00_namespace.yaml const APPLIED_NAMESPACE = "openshift-network-operator" -// MULTUS_NAMESPACE is the namespace where applied configuration +// MultusNamespace is the namespace where applied configuration // configmaps are stored. // Should match 00_namespace.yaml -const MULTUS_NAMESPACE = "openshift-multus" +const MultusNamespace = "openshift-multus" -// ALLOWLIST_CONFIG_NAME is the name of the allowlist ConfigMap -const ALLOWLIST_CONFIG_NAME = "cni-sysctl-allowlist" +// AllowlistConfigName is the name of the allowlist ConfigMap +const AllowlistConfigName = "cni-sysctl-allowlist" // IgnoreObjectErrorAnnotation is an annotation we can set on objects // to signal to the reconciler that we don't care if they fail to create From f6225d7c47b7d311534c4528ef664dcb5ed4d7ff Mon Sep 17 00:00:00 2001 From: Konstantinos Karampogias Date: Wed, 26 Nov 2025 17:15:32 +0100 Subject: [PATCH 2/4] Allowlist controller: remove unused configmap template The bindata/allowlist/config/configmap.yaml file is not used by any code. The controller uses bindata/network/multus/004-sysctl-configmap.yaml as the template for creating the cni-sysctl-allowlist ConfigMap when it doesn't exist. This template is shared with multus which uses it to create the default-cni-sysctl-allowlist ConfigMap. Assisted-By: Claude Signed-off-by: Konstantinos Karampogias --- bindata/allowlist/config/configmap.yaml | 37 ------------------------- 1 file changed, 37 deletions(-) delete mode 100644 bindata/allowlist/config/configmap.yaml diff --git a/bindata/allowlist/config/configmap.yaml b/bindata/allowlist/config/configmap.yaml deleted file mode 100644 index 557a6a792d..0000000000 --- a/bindata/allowlist/config/configmap.yaml +++ /dev/null @@ -1,37 +0,0 @@ -apiVersion: v1 -kind: ConfigMap -metadata: - name: cni-sysctl-allowlist - namespace: openshift-multus - annotations: - kubernetes.io/description: | - Sysctl allowlist for nodes - release.openshift.io/version: "{{.ReleaseVersion}}" -data: - # - # Safe sysctls - # ------------- - # add to this list only sysctls that - # * must not have any influence on any other pod on the node - # * must not allow to harm the node's health - # * must not allow to gain CPU or memory resources outside of the resource limit of the pod - # - allowlist.conf: |- - ^net.ipv4.conf.IFNAME.accept_ra$ - ^net.ipv4.conf.IFNAME.accept_redirects$ - ^net.ipv4.conf.IFNAME.accept_source_route$ - ^net.ipv4.conf.IFNAME.arp_accept$ - ^net.ipv4.conf.IFNAME.arp_notify$ - ^net.ipv4.conf.IFNAME.disable_policy$ - ^net.ipv4.conf.IFNAME.rp_filter$ - ^net.ipv4.conf.IFNAME.secure_redirects$ - ^net.ipv4.conf.IFNAME.send_redirects$ - ^net.ipv6.conf.IFNAME.accept_ra$ - ^net.ipv6.conf.IFNAME.accept_redirects$ - ^net.ipv6.conf.IFNAME.accept_source_route$ - ^net.ipv6.conf.IFNAME.arp_accept$ - ^net.ipv6.conf.IFNAME.arp_notify$ - ^net.ipv6.conf.IFNAME.disable_ipv6$ - ^net.ipv6.conf.IFNAME.disable_policy$ - ^net.ipv6.neigh.IFNAME.base_reachable_time_ms$ - ^net.ipv6.neigh.IFNAME.retrans_time_ms$ From 8bbdf70a02b4ade1a644e713f294840c32ba4c44 Mon Sep 17 00:00:00 2001 From: Konstantinos Karampogias Date: Thu, 27 Nov 2025 10:05:20 +0100 Subject: [PATCH 3/4] Allowlist controller: remove unused scheme field The runtime.Scheme field was never used - controller creates objects directly via client.Create() without needing type conversion or owner references. Assisted-By: Claude Signed-off-by: Konstantinos Karampogias --- pkg/controller/allowlist/allowlist_controller.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/controller/allowlist/allowlist_controller.go b/pkg/controller/allowlist/allowlist_controller.go index a4afcf14b4..9c60a25823 100644 --- a/pkg/controller/allowlist/allowlist_controller.go +++ b/pkg/controller/allowlist/allowlist_controller.go @@ -20,7 +20,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -80,7 +79,6 @@ var _ reconcile.Reconciler = &ReconcileAllowlist{} type ReconcileAllowlist struct { client cnoclient.Client - scheme *runtime.Scheme status *statusmanager.StatusManager } From dbc78378eaf6d51c4c6b4bc9e3f0fcc5f1c1598e Mon Sep 17 00:00:00 2001 From: Konstantinos Karampogias Date: Thu, 27 Nov 2025 10:24:58 +0100 Subject: [PATCH 4/4] Allowlist controller: inline single-use helper functions Remove createObject() and deleteDaemonSet() helper functions that were each called only once. Inlining them at their call sites improves readability by eliminating unnecessary indirection. This also removes the unused unstructured import. Signed-off-by: Konstantinos Karampogias --- .../allowlist/allowlist_controller.go | 28 ++++--------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/pkg/controller/allowlist/allowlist_controller.go b/pkg/controller/allowlist/allowlist_controller.go index 9c60a25823..ee215a3967 100644 --- a/pkg/controller/allowlist/allowlist_controller.go +++ b/pkg/controller/allowlist/allowlist_controller.go @@ -19,7 +19,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -45,7 +44,7 @@ const ( ) func Add(mgr manager.Manager, status *statusmanager.StatusManager, client cnoclient.Client, _ featuregates.FeatureGate) error { - r:= &ReconcileAllowlist{client: client, status: status} + r := &ReconcileAllowlist{client: client, status: status} c, err := controller.New("allowlist-controller", mgr, controller.Options{Reconciler: r}) if err != nil { return err @@ -159,9 +158,8 @@ func createObjectsFrom(ctx context.Context, client cnoclient.Client, manifestPat return err } for _, obj := range manifests { - err = createObject(ctx, client, obj) - if err != nil { - return err + if err := client.Default().CRClient().Create(ctx, obj); err != nil { + return errors.Wrapf(err, "error creating %s %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) } } return nil @@ -179,14 +177,6 @@ func getConfigMap(ctx context.Context, client cnoclient.Client, namespacedName t return configMap, nil } -func createObject(ctx context.Context, client cnoclient.Client, obj *unstructured.Unstructured) error { - err := client.Default().CRClient().Create(ctx, obj) - if err != nil { - return errors.Wrapf(err, "error creating %s %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName()) - } - return nil -} - func checkDsPodsReady(ctx context.Context, client cnoclient.Client) error { return wait.PollUntilContextTimeout(ctx, time.Second, time.Minute, false, func(ctx context.Context) (done bool, err error) { ds, err := getDaemonSet(ctx, client) @@ -228,22 +218,14 @@ func cleanupDaemonSet(ctx context.Context, client cnoclient.Client) { return } if ds != nil { - err = deleteDaemonSet(ctx, client) + err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MultusNamespace).Delete( + ctx, dsName, metav1.DeleteOptions{}) if err != nil { klog.Errorf("Error cleaning up allow list daemonset: %+v", err) } } } -func deleteDaemonSet(ctx context.Context, client cnoclient.Client) error { - err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MultusNamespace).Delete( - ctx, dsName, metav1.DeleteOptions{}) - if err != nil { - return err - } - return nil -} - func getDaemonSet(ctx context.Context, client cnoclient.Client) (*appsv1.DaemonSet, error) { ds, err := client.Default().Kubernetes().AppsV1().DaemonSets(names.MultusNamespace).Get( ctx, dsName, metav1.GetOptions{})