Skip to content
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

Jump to bottom

[Prototype] Fix for PATCH without tier prefix #9445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Prototype] Fix for PATCH without tier prefix #9445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions apiserver/pkg/registry/projectcalico/globalpolicy/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
}

Expand Down
5 changes: 4 additions & 1 deletion apiserver/pkg/registry/projectcalico/networkpolicy/strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming this approach even has legs, we'll need to make this change for global policies as well.


func canonicalizePolicyName(obj runtime.Object) string {
Expand Down
22 changes: 20 additions & 2 deletions apiserver/pkg/storage/calico/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"os"
"reflect"
"strconv"
"strings"
"time"

aapierrors "k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably want to check the exact error message here.

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:
Copy link
Member Author

Choose a reason for hiding this comment

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

Should probably only execute this logic if the tier is "default"

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)
Expand Down
18 changes: 18 additions & 0 deletions apiserver/test/integration/clientset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
Loading