diff --git a/bindata/network/frr-k8s/webhook.yaml b/bindata/network/frr-k8s/webhook.yaml index ef699535f1..c842892d14 100644 --- a/bindata/network/frr-k8s/webhook.yaml +++ b/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 diff --git a/pkg/apply/apply.go b/pkg/apply/apply.go index a98a06fe61..e85818b0dd 100644 --- a/pkg/apply/apply.go +++ b/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 diff --git a/pkg/apply/apply_test.go b/pkg/apply/apply_test.go new file mode 100644 index 0000000000..566895ae16 --- /dev/null +++ b/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") +} diff --git a/pkg/client/fake/fake_client.go b/pkg/client/fake/fake_client.go index be061d27ed..f9786f946d 100644 --- a/pkg/client/fake/fake_client.go +++ b/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!") diff --git a/pkg/controller/statusmanager/status_manager_test.go b/pkg/controller/statusmanager/status_manager_test.go index 9ae9ea9b44..bb25d49eae 100644 --- a/pkg/controller/statusmanager/status_manager_test.go +++ b/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") } diff --git a/pkg/names/names.go b/pkg/names/names.go index db0d7bf93f..f9a4a8018b 100644 --- a/pkg/names/names.go +++ b/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" diff --git a/pkg/network/render.go b/pkg/network/render.go index 0cda29961d..eee3bcefac 100644 --- a/pkg/network/render.go +++ b/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) diff --git a/pkg/network/render_test.go b/pkg/network/render_test.go index 10f7bf41a6..64d95967bf 100644 --- a/pkg/network/render_test.go +++ b/pkg/network/render_test.go @@ -9,6 +9,7 @@ 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" @@ -16,6 +17,7 @@ import ( 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,7 +646,7 @@ 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) } @@ -652,3 +654,42 @@ func Test_renderAdditionalRoutingCapabilities(t *testing.T) { }) } } + +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)) + }) +} diff --git a/pkg/network/testutil_test.go b/pkg/network/testutil_test.go index 3a71c075ca..caf993b841 100644 --- a/pkg/network/testutil_test.go +++ b/pkg/network/testutil_test.go @@ -3,7 +3,10 @@ package network import ( "context" "fmt" + "slices" + "testing" + . "github.com/onsi/gomega" "github.com/onsi/gomega/types" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-network-operator/pkg/bootstrap" @@ -11,6 +14,7 @@ import ( "github.com/openshift/cluster-network-operator/pkg/hypershift" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" ) // Fun matcher for testing the presence of Kubernetes objects @@ -108,3 +112,21 @@ func createProxy(client client.Client) error { } return client.Default().CRClient().Create(context.TODO(), proxy) } + +// mustFindRenderedObj finds and converts an unstructured object from a list of rendered objects. +// It uses Go generics to return the properly typed object. +func mustFindRenderedObj[T any](t *testing.T, objs []*uns.Unstructured, kind, name string) T { + t.Helper() + g := NewWithT(t) + + index := slices.IndexFunc(objs, func(obj *uns.Unstructured) bool { + return obj.GetKind() == kind && obj.GetName() == name + }) + g.Expect(index).NotTo(Equal(-1), "Could not find object with kind %q and name %q", kind, name) + + var result T + err := runtime.DefaultUnstructuredConverter.FromUnstructured(objs[index].Object, &result) + g.Expect(err).NotTo(HaveOccurred()) + + return result +}