[release-4.20] OCPBUGS-113989: frr-k8s: use Recreate strategy for webhook server deployment on SNO

bindata/network/frr-k8s/webhook.yaml

@@ -51,7 +51,20 @@ metadata:
     component: frr-k8s-webhook-server
   annotations:
     release.openshift.io/version: "{{.ReleaseVersion}}"
+{{- if .IsSNO }}
+    networkoperator.openshift.io/pre-patch: '{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}'
+{{- end }}
 spec:
+{{- if .IsSNO }}
+  strategy:
+    type: Recreate
+{{- else }}
+  strategy:
+    type: RollingUpdate
+    rollingUpdate:
+      maxSurge: 25%
+      maxUnavailable: 25%
+{{- end }}
   selector:
     matchLabels:
       component: frr-k8s-webhook-server

pkg/apply/apply.go

@@ -110,6 +110,23 @@ func ApplyObject(ctx context.Context, client cnoclient.Client, obj Object, subco
 		fieldManager = fmt.Sprintf("%s/%s", fieldManager, subcontroller)
 	}
 
+	// If pre-patch is specified, apply it as a strategic-merge-patch to the live
+	// object before SSA. This handles cases where SSA cannot remove a field it
+	// does not own (e.g. defaulted rollingUpdate when switching to Recreate).
+	// The pre-patch is silently skipped if the object does not exist yet, making
+	// this mainly relevant on upgrades where defaulted fields must be removed.
+	if prePatch, ok := obj.GetAnnotations()[names.PrePatchAnnotation]; ok {
+		log.Printf("Object %s has pre-patch annotation, attempting strategic-merge-patch before SSA", objDesc)
+		patchOptions := metav1.PatchOptions{FieldManager: fieldManager}
+		_, err := clusterClient.Dynamic().Resource(rm.Resource).Namespace(namespace).Patch(
+			ctx, name, types.StrategicMergePatchType, []byte(prePatch), patchOptions)
+		if apierrors.IsNotFound(err) {
+			log.Printf("Object %s not found, skipping pre-patch", objDesc)
+		} else if err != nil {
+			return fmt.Errorf("failed to pre-patch %s: %w", objDesc, err)
+		}
+	}
+
 	// Use server-side apply to merge the desired object with the object on disk
 	patchOptions := metav1.PatchOptions{
 		// It is considered best-practice for controllers to force

pkg/apply/apply_test.go

@@ -0,0 +1,130 @@
+package apply
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/openshift/cluster-network-operator/pkg/client/fake"
+	"github.com/openshift/cluster-network-operator/pkg/names"
+
+	. "github.com/onsi/gomega"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	fakedynamic "k8s.io/client-go/dynamic/fake"
+	clienttesting "k8s.io/client-go/testing"
+)
+
+const prePatchValue = `{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}`
+
+func newTestDeployment(annotations map[string]string) *unstructured.Unstructured {
+	obj := &unstructured.Unstructured{}
+	obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})
+	obj.SetName("test-deployment")
+	obj.SetNamespace("test-ns")
+	obj.SetAnnotations(annotations)
+	return obj
+}
+
+func patchRecorder(patchTypes *[]types.PatchType, prePatchBody *string, prePatchErr error) func(action clienttesting.Action) (bool, runtime.Object, error) {
+	return func(action clienttesting.Action) (bool, runtime.Object, error) {
+		patchAction := action.(clienttesting.PatchAction)
+		*patchTypes = append(*patchTypes, patchAction.GetPatchType())
+		if patchAction.GetPatchType() == types.StrategicMergePatchType {
+			*prePatchBody = string(patchAction.GetPatch())
+			if prePatchErr != nil {
+				return true, nil, prePatchErr
+			}
+		}
+		return true, newTestDeployment(nil), nil
+	}
+}
+
+func TestApplyObjectPrePatchRunsBeforeSSA(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, nil)
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(t.Context(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(2))
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch should be strategic-merge-patch")
+	g.Expect(patchTypes[1]).To(Equal(types.ApplyPatchType), "second patch should be SSA apply")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectPrePatchNotFoundAllowsSSA(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, apierrors.NewNotFound(
+		schema.GroupResource{Group: "apps", Resource: "deployments"}, "test-deployment"))
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(t.Context(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(2))
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch was attempted")
+	g.Expect(patchTypes[1]).To(Equal(types.ApplyPatchType), "SSA apply still proceeded")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectPrePatchErrorStopsReconciliation(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, fmt.Errorf("API server error"))
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(map[string]string{
+		names.PrePatchAnnotation: prePatchValue,
+	})
+
+	err := ApplyObject(t.Context(), client, obj, "test-controller")
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err.Error()).To(ContainSubstring("failed to pre-patch"))
+	g.Expect(patchTypes).To(HaveLen(1), "SSA apply should not have been reached")
+	g.Expect(patchTypes[0]).To(Equal(types.StrategicMergePatchType), "pre-patch should be strategic-merge-patch")
+	g.Expect(prePatchBody).To(Equal(prePatchValue), "pre-patch body should match annotation value")
+}
+
+func TestApplyObjectNoPrePatchAnnotationSkipsPrePatch(t *testing.T) {
+	g := NewWithT(t)
+
+	client := fake.NewFakeClient()
+
+	var patchTypes []types.PatchType
+	var prePatchBody string
+	pr := patchRecorder(&patchTypes, &prePatchBody, nil)
+	client.Default().Dynamic().(*fakedynamic.FakeDynamicClient).PrependReactor("patch", "deployments", pr)
+
+	obj := newTestDeployment(nil)
+
+	err := ApplyObject(t.Context(), client, obj, "test-controller")
+	g.Expect(err).To(Succeed())
+	g.Expect(patchTypes).To(HaveLen(1))
+	g.Expect(patchTypes[0]).To(Equal(types.ApplyPatchType), "only SSA apply should have run")
+	g.Expect(prePatchBody).To(BeEmpty(), "no pre-patch should have been sent")
+}

pkg/client/fake/fake_client.go

@@ -5,9 +5,9 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 	fakedynamic "k8s.io/client-go/dynamic/fake"
 	"k8s.io/client-go/kubernetes"
@@ -113,42 +113,6 @@ func NewFakeClient(objs ...crclient.Object) cnoclient.Client {
 	}
 }
 
-type fakeRESTMapper struct {
-	kindForInput schema.GroupVersionResource
-}
-
-func (f *fakeRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) {
-	f.kindForInput = resource
-	return schema.GroupVersionKind{
-		Group:   "test",
-		Version: "test",
-		Kind:    "test"}, nil
-}
-
-func (f *fakeRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) {
-	return schema.GroupVersionResource{}, nil
-}
-
-func (f *fakeRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) {
-	return nil, nil
-}
-
-func (f *fakeRESTMapper) ResourceSingularizer(resource string) (singular string, err error) {
-	return "", nil
-}
-
 func (fc *FakeClusterClient) Kubernetes() kubernetes.Interface {
 	return fc.kClient
 }
@@ -170,11 +134,11 @@ func (fc *FakeClusterClient) CRClient() crclient.Client {
 }
 
 func (fc *FakeClusterClient) RESTMapper() meta.RESTMapper {
-	return &fakeRESTMapper{}
+	return testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme)
 }
 
 func (fc *FakeClusterClient) Scheme() *runtime.Scheme {
-	panic("not implemented!")
+	return scheme.Scheme
 }
 func (fc *FakeClusterClient) OperatorHelperClient() operatorv1helpers.OperatorClient {
 	panic("not implemented!")

pkg/controller/statusmanager/status_manager_test.go

@@ -250,30 +250,33 @@ func TestStatusManager_set(t *testing.T) {
 
 	obj := &uns.Unstructured{}
 	gvk := schema.GroupVersionKind{
-		Group:   "test",
-		Version: "test",
-		Kind:    "test",
+		Group:   "",
+		Version: "v1",
+		Kind:    "ConfigMap",
 	}
 	obj.SetGroupVersionKind(gvk)
 	obj.SetName("current")
+	obj.SetNamespace("default")
 	set(t, client, obj)
 
 	co.Status.RelatedObjects = []configv1.ObjectReference{
 		{
-			Group:    "test",
-			Resource: "test",
-			Name:     "current",
+			Group:     "",
+			Resource:  "configmaps",
+			Name:      "current",
+			Namespace: "default",
 		},
 	}
 	status.relatedObjects = []configv1.ObjectReference{
 		{
-			Group:    "test",
-			Resource: "test",
-			Name:     "related",
+			Group:     "",
+			Resource:  "configmaps",
+			Name:      "related",
+			Namespace: "default",
 		},
 	}
 	status.deleteRelatedObjectsNotRendered(co)
-	err = status.client.ClientFor("").CRClient().Get(context.TODO(), types.NamespacedName{Name: "current"}, obj)
+	err = status.client.ClientFor("").CRClient().Get(context.TODO(), types.NamespacedName{Namespace: "default", Name: "current"}, obj)
 	if err == nil {
 		t.Fatalf("unexpected related object in ClusterOperator object was not deleted")
 	}

pkg/names/names.go

@@ -48,6 +48,12 @@ const CreateOnlyAnnotation = "networkoperator.openshift.io/create-only"
 // tells the CNO reconciliation engine to ignore creating this object until conditions are met.
 const CreateWaitAnnotation = "networkoperator.openshift.io/create-wait"
 
+// PrePatchAnnotation is an annotation whose value is a JSON object applied as a
+// strategic-merge-patch to the live object before the SSA apply. This is needed
+// when SSA cannot remove a field it does not own (e.g. removing defaulted
+// rollingUpdate when switching a Deployment strategy to Recreate).
+const PrePatchAnnotation = "networkoperator.openshift.io/pre-patch"
+
 // NonCriticalAnnotation is an annotation on Deployments/DaemonSets to indicate
 // that they are not critical to the functioning of the pod network
 const NonCriticalAnnotation = "networkoperator.openshift.io/non-critical"

pkg/network/render.go

@@ -147,7 +147,7 @@ func Render(operConf *operv1.NetworkSpec, clusterConf *configv1.NetworkSpec, man
 	}
 	objs = append(objs, o...)
 
-	o, err = renderAdditionalRoutingCapabilities(operConf, manifestDir)
+	o, err = renderAdditionalRoutingCapabilities(operConf, bootstrapResult, manifestDir)
 	if err != nil {
 		return nil, progressing, err
 	}
@@ -978,7 +978,7 @@ func isSupportedDualStackPlatform(platformType configv1.PlatformType) bool {
 	return dualStackPlatforms.Has(string(platformType))
 }
 
-func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Unstructured, error) {
+func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, bootstrapResult *bootstrap.BootstrapResult, manifestDir string) ([]*uns.Unstructured, error) {
 	if conf == nil || conf.AdditionalRoutingCapabilities == nil {
 		return nil, nil
 	}
@@ -990,6 +990,7 @@ func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir s
 			data.Data["FRRK8sImage"] = os.Getenv("FRR_K8S_IMAGE")
 			data.Data["KubeRBACProxyImage"] = os.Getenv("KUBE_RBAC_PROXY_IMAGE")
 			data.Data["ReleaseVersion"] = os.Getenv("RELEASE_VERSION")
+			data.Data["IsSNO"] = bootstrapResult.OVN.ControlPlaneReplicaCount == 1
 			objs, err := render.RenderDir(filepath.Join(manifestDir, "network/frr-k8s"), &data)
 			if err != nil {
 				return nil, fmt.Errorf("failed to render frr-k8s manifests: %w", err)

pkg/network/render_test.go

@@ -9,13 +9,15 @@ import (
 	"github.com/openshift/cluster-network-operator/pkg/client/fake"
 	"github.com/openshift/cluster-network-operator/pkg/hypershift"
 	"github.com/stretchr/testify/assert"
+	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
 	operv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-network-operator/pkg/bootstrap"
+	"github.com/openshift/cluster-network-operator/pkg/names"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -644,11 +646,50 @@ func Test_renderAdditionalRoutingCapabilities(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := renderAdditionalRoutingCapabilities(tt.args.operConf, manifestDir)
+			got, err := renderAdditionalRoutingCapabilities(tt.args.operConf, fakeBootstrapResult(), manifestDir)
 			if !reflect.DeepEqual(tt.expectedErr, err) {
 				t.Errorf("renderAdditionalRoutingCapabilities() err = %v, want %v", err, tt.expectedErr)
 			}
 			assert.Equalf(t, tt.want, len(got), "renderAdditionalRoutingCapabilities(%v, %v)", tt.args.operConf, manifestDir)
 		})
 	}
 }
+
+func Test_renderFRRWebhookServerStrategy(t *testing.T) {
+	// On 4.20 the hostNetwork workload is frr-k8s-webhook-server; statuscleaner was split out in 4.21.
+	frrConf := &operv1.NetworkSpec{
+		AdditionalRoutingCapabilities: &operv1.AdditionalRoutingCapabilities{
+			Providers: []operv1.RoutingCapabilitiesProvider{
+				operv1.RoutingCapabilitiesProviderFRR,
+			},
+		},
+	}
+
+	render := func(replicaCount int) *appsv1.Deployment {
+		g := NewWithT(t)
+		br := fakeBootstrapResult()
+		br.OVN.ControlPlaneReplicaCount = replicaCount
+		objs, err := renderAdditionalRoutingCapabilities(frrConf, br, manifestDir)
+		g.Expect(err).NotTo(HaveOccurred())
+		return mustFindRenderedObj[*appsv1.Deployment](t, objs, "Deployment", "frr-k8s-webhook-server")
+	}
+
+	t.Run("SNO: strategy is Recreate", func(t *testing.T) {
+		g := NewWithT(t)
+		d := render(1)
+		g.Expect(d.Spec.Strategy.Type).To(Equal(appsv1.RecreateDeploymentStrategyType))
+		g.Expect(d.Annotations).To(
+			HaveKeyWithValue(
+				names.PrePatchAnnotation,
+				`{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}`,
+			),
+		)
+	})
+
+	t.Run("HA: strategy is RollingUpdate", func(t *testing.T) {
+		g := NewWithT(t)
+		d := render(3)
+		g.Expect(d.Spec.Strategy.Type).To(Equal(appsv1.RollingUpdateDeploymentStrategyType))
+		g.Expect(d.Annotations).NotTo(HaveKey(names.PrePatchAnnotation))
+	})
+}