From 4543b410aa2f4d36cc190bf7ff45a45ab05f9290 Mon Sep 17 00:00:00 2001 From: Casey Davenport Date: Tue, 5 Nov 2024 15:43:32 -0800 Subject: [PATCH] Prototype fix for PATCH without tier prefix --- .../projectcalico/globalpolicy/storage.go | 4 ++-- .../projectcalico/networkpolicy/strategy.go | 5 ++++- apiserver/pkg/storage/calico/resource.go | 22 +++++++++++++++++-- apiserver/test/integration/clientset_test.go | 18 +++++++++++++++ 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/apiserver/pkg/registry/projectcalico/globalpolicy/storage.go b/apiserver/pkg/registry/projectcalico/globalpolicy/storage.go index 23f95fc186f..57619d4ee38 100644 --- a/apiserver/pkg/registry/projectcalico/globalpolicy/storage.go +++ b/apiserver/pkg/registry/projectcalico/globalpolicy/storage.go @@ -128,7 +128,8 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, val rest.Validate } func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, - updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { + updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions, +) (runtime.Object, bool, error) { tierName, _ := util.GetTierFromPolicyName(name) err := r.authorizer.AuthorizeTierOperation(ctx, name, tierName) if err != nil { @@ -145,7 +146,6 @@ func (r *REST) Get(ctx context.Context, name string, options *metav1.GetOptions) if err != nil { return nil, err } - return r.Store.Get(ctx, name, options) } diff --git a/apiserver/pkg/registry/projectcalico/networkpolicy/strategy.go b/apiserver/pkg/registry/projectcalico/networkpolicy/strategy.go index a7ea92d6edd..8f33d3a63ab 100644 --- a/apiserver/pkg/registry/projectcalico/networkpolicy/strategy.go +++ b/apiserver/pkg/registry/projectcalico/networkpolicy/strategy.go @@ -47,7 +47,10 @@ func (policyStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) } func (policyStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { - obj.(*calico.NetworkPolicy).Name = canonicalizePolicyName(old) + // Canonicalize the old object's name as well to make sure it's consistent. + // This typically shouldn't be needed for update requests, but is necessary in PATCH + // requests because we may need to strip the tier earlier in the pipeline to pass earlier validation. + old.(*calico.NetworkPolicy).Name = canonicalizePolicyName(old) } func canonicalizePolicyName(obj runtime.Object) string { diff --git a/apiserver/pkg/storage/calico/resource.go b/apiserver/pkg/storage/calico/resource.go index bb0c3d832b4..b51020f42fe 100644 --- a/apiserver/pkg/storage/calico/resource.go +++ b/apiserver/pkg/storage/calico/resource.go @@ -9,6 +9,7 @@ import ( "os" "reflect" "strconv" + "strings" "time" aapierrors "k8s.io/apimachinery/pkg/api/errors" @@ -22,6 +23,7 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/klog/v2" + v3 "github.com/projectcalico/api/pkg/apis/projectcalico/v3" "github.com/projectcalico/calico/libcalico-go/lib/apiconfig" "github.com/projectcalico/calico/libcalico-go/lib/clientv3" "github.com/projectcalico/calico/libcalico-go/lib/errors" @@ -417,11 +419,27 @@ func (rs *resourceStore) GuaranteedUpdate( klog.Errorf("checking preconditions (%s)", err) return err } + // update the object by applying the userUpdate func & encode it updatedObj, ttl, err := userUpdate(curState.obj, *curState.meta) if err != nil { - klog.Errorf("applying user update: (%s)", err) - return err + // Try updating the object name to remove the tier - it's possible that the user send the + // object without the tier in the name, in which case we need to retry the update against the + // object without the tier in the name. + switch curState.obj.(type) { + case *v3.GlobalNetworkPolicy: + tier := curState.obj.(*v3.GlobalNetworkPolicy).Spec.Tier + curState.obj.(*v3.GlobalNetworkPolicy).Name = strings.TrimPrefix(curState.obj.(*v3.GlobalNetworkPolicy).Name, tier+".") + updatedObj, ttl, err = userUpdate(curState.obj, *curState.meta) + case *v3.NetworkPolicy: + tier := curState.obj.(*v3.NetworkPolicy).Spec.Tier + curState.obj.(*v3.NetworkPolicy).Name = strings.TrimPrefix(curState.obj.(*v3.NetworkPolicy).Name, tier+".") + updatedObj, ttl, err = userUpdate(curState.obj, *curState.meta) + } + if err != nil { + klog.Errorf("applying user update: (%s)", err) + return err + } } updatedData, err := runtime.Encode(rs.codec, updatedObj) diff --git a/apiserver/test/integration/clientset_test.go b/apiserver/test/integration/clientset_test.go index 62b4150d0f8..5f1e4da599f 100644 --- a/apiserver/test/integration/clientset_test.go +++ b/apiserver/test/integration/clientset_test.go @@ -33,6 +33,7 @@ import ( "github.com/projectcalico/api/pkg/lib/numorstring" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" "github.com/projectcalico/calico/libcalico-go/lib/apiconfig" @@ -199,6 +200,16 @@ func testNetworkPolicyClient(client calicoclient.Interface, name string) error { return fmt.Errorf("policy name prefix wasn't defaulted by the apiserver on update: %v", policyServer) } + // Patch the policy. We should be able to use the same name that we used to create it (i.e., without the "default" prefix). + patch := []byte(`{"metadata": {"labels": {"foo": "baz"}}}`) + policyServer, err = policyClient.Patch(ctx, name, types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + return fmt.Errorf("error patching the policy without the tier prefix '%v' (%v)", policyServer, err) + } + if defaultTierPolicyName != policyServer.Name { + return fmt.Errorf("policy name prefix wasn't defaulted by the apiserver on patch: %v", policyServer) + } + // Delete that policy. We should be able to use the same name that we used to create it (i.e., without the "default" prefix). err = policyClient.Delete(ctx, name, metav1.DeleteOptions{}) if err != nil { @@ -224,6 +235,13 @@ func testNetworkPolicyClient(client calicoclient.Interface, name string) error { return fmt.Errorf("didn't get the same policy back from the server \n%+v\n%+v", policy, policyServer) } + // Patch the policy with the tiered prefix. + patch = []byte(`{"metadata": {"labels": {"foo": "baz"}}}`) + policyServer, err = policyClient.Patch(ctx, defaultTierPolicyName, types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + return fmt.Errorf("error patching the policy with the tier prefix '%v' (%v)", policyServer, err) + } + // For testing out Tiered Policy tierClient := client.ProjectcalicoV3().Tiers() order := float64(100.0)