Skip to content

OCPBUGS-60532: Allowlist controller: add node reconciler to sync allowlist on node creation #2858

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.

Sign up for GitHub

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

Open
karampok wants to merge 5 commits into
base: master
Choose a base branch
from
Open

OCPBUGS-60532: Allowlist controller: add node reconciler to sync allowlist on node creation #2858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09e6f85
Allowlist controller: refactor names in controller code
karampok Nov 26, 2025
b299f62
Allowlist controller: remove unused configmap template
karampok Nov 26, 2025
21c055d
Allowlist controller: remove unused scheme field
karampok Nov 27, 2025
74e0cb5
Allowlist controller: inline single-use helper functions
karampok Nov 27, 2025
f709c58
Allowlist controller: add node reconciler to sync allowlist on node c…
karampok Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 0 additions & 37 deletions bindata/allowlist/config/configmap.yaml
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ require (
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/fsnotify/fsnotify v1.9.0 // indirect
github.com/go-logr/logr v1.4.3 // indirect
github.com/go-logr/logr v1.4.3
github.com/go-openapi/jsonpointer v0.22.1 // indirect
github.com/go-openapi/jsonreference v0.21.2 // indirect
github.com/go-openapi/swag v0.25.1 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/google/go-cmp v0.7.0
github.com/google/uuid v1.6.0 // indirect
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.1-0.20210315223345-82c243799c99 // indirect
github.com/huandu/xstrings v1.4.0 // indirect
Expand Down Expand Up @@ -98,7 +98,7 @@ require (
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0 // indirect
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect
sigs.k8s.io/kube-storage-version-migrator v0.0.6-0.20230721195810-5c8923c5ff96 // indirect
sigs.k8s.io/yaml v1.6.0 // indirect
sigs.k8s.io/yaml v1.6.0
)

require (
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/add_networkconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func init() {
ingressconfig.Add,
infrastructureconfig.Add,
allowlist.Add,
allowlist.AddNodeReconciler,
dashboards.Add,
)
}
96 changes: 38 additions & 58 deletions pkg/controller/allowlist/allowlist_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Package allowlist implements a Kubernetes controller that distributes CNI
// sysctl allowlist configuration to cluster nodes.
package allowlist

import (
Expand All @@ -17,8 +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/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
Expand All @@ -36,21 +36,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
Expand All @@ -59,7 +53,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})

Expand All @@ -73,7 +67,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))
}),
},
})
Expand All @@ -83,14 +78,13 @@ var _ reconcile.Reconciler = &ReconcileAllowlist{}

type ReconcileAllowlist struct {
client cnoclient.Client
scheme *runtime.Scheme
status *statusmanager.StatusManager
}

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
Expand All @@ -100,23 +94,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)
Expand All @@ -129,7 +127,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
Expand All @@ -150,26 +148,24 @@ 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
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())
}
Comment on lines 160 to 163
Copy link

@coderabbitai coderabbitai bot Dec 11, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

createObjectsFrom fails on AlreadyExists instead of being idempotent.

When creating objects, if any already exists, the function returns an error. This could cause issues during reconciliation if the ConfigMap was partially created. Consider ignoring AlreadyExists errors to make the operation idempotent.

 	for _, obj := range manifests {
-		if err := client.Default().CRClient().Create(ctx, obj); err != nil {
+		if err := client.Default().CRClient().Create(ctx, obj); err != nil && !apierrors.IsAlreadyExists(err) {
 			return errors.Wrapf(err, "error creating %s %s/%s", obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName())
 		}
 	}
🤖 Prompt for AI Agents 
In pkg/controller/allowlist/allowlist_controller.go around lines 160 to 163, the
loop that creates manifests returns on any error causing AlreadyExists to fail
instead of being idempotent; change the error handling so that when
client.Default().CRClient().Create returns an AlreadyExists error you ignore it
and continue (use the API server error check for AlreadyExists), but for all
other errors return the wrapped error as before; this makes object creation
idempotent during reconciliation.

}
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 {
Expand All @@ -181,14 +177,6 @@ func getConfig(ctx context.Context, client cnoclient.Client, namespacedName type
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)
Expand All @@ -199,8 +187,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
}
Expand All @@ -223,32 +211,24 @@ 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)
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.MULTUS_NAMESPACE).Delete(
ctx, allowlistDsName, 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.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
Expand All @@ -258,9 +238,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
Expand Down
Loading