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.

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

fix deep equal check failure on objects with runtime.RawExtension. #5940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions cmd/agent/app/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"

"github.com/spf13/cobra"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/dynamic"
Expand Down Expand Up @@ -235,6 +236,11 @@ func run(ctx context.Context, opts *options.Options) error {
crtlmetrics.Registry.MustRegister(metrics.ResourceCollectorsForAgent()...)
crtlmetrics.Registry.MustRegister(metrics.PoolCollectors()...)

if err := util.RegisterEqualityCheckFunctions(&equality.Semantic); err != nil {
klog.Errorf("Failed to register equality check functions: %v", err)
return err
}

if err = setupControllers(controllerManager, opts, ctx.Done()); err != nil {
return err
}
Expand Down
6 changes: 6 additions & 0 deletions cmd/controller-manager/app/controllermanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"time"

"github.com/spf13/cobra"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/discovery"
Expand Down Expand Up @@ -192,6 +193,11 @@ func Run(ctx context.Context, opts *options.Options) error {
crtlmetrics.Registry.MustRegister(metrics.ResourceCollectors()...)
crtlmetrics.Registry.MustRegister(metrics.PoolCollectors()...)

if err := util.RegisterEqualityCheckFunctions(&equality.Semantic); err != nil {
klog.Errorf("Failed to register equality check functions: %v", err)
return err
}

if err := helper.IndexWork(ctx, controllerManager); err != nil {
klog.Fatalf("Failed to index Work: %v", err)
}
Expand Down
89 changes: 89 additions & 0 deletions pkg/util/equality.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
Copyright 2024 The Karmada Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"encoding/json"

"k8s.io/apimachinery/pkg/conversion"
"k8s.io/apimachinery/pkg/runtime"

workv1alpha1 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha1"
workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
)

// RegisterEqualityCheckFunctions registers custom check functions to the equality checker.
// These functions help avoid performing deep-equality checks on workloads represented as byte slices.
func RegisterEqualityCheckFunctions(e *conversion.Equalities) error {
return e.AddFuncs(
Copy link
Member

Choose a reason for hiding this comment

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

One question, can we just add a comparison function for runtime.RawExtension:

                func(a, b runtime.RawExtension) bool {
			return rawExtensionDeepEqual(&a, &b, e)
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then if other parts (not even karmada code) also use deepEqual on runtime.RawExtension, they will also be affected by the new check functions, I think the impact will be larger and more unpredictable.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining!

func(a, b workv1alpha1.Manifest) bool {
return rawExtensionDeepEqual(&a.RawExtension, &b.RawExtension, e)
},
func(a, b workv1alpha1.ManifestStatus) bool {
return e.DeepEqual(a.Identifier, b.Identifier) &&
rawExtensionDeepEqual(a.Status, b.Status, e) &&
e.DeepEqual(a.Health, b.Health)
},
func(a, b workv1alpha1.AggregatedStatusItem) bool {
return e.DeepEqual(a.ClusterName, b.ClusterName) &&
rawExtensionDeepEqual(a.Status, b.Status, e) &&
e.DeepEqual(a.Applied, b.Applied) &&
e.DeepEqual(a.AppliedMessage, b.AppliedMessage)
},
func(a, b workv1alpha2.AggregatedStatusItem) bool {
return e.DeepEqual(a.ClusterName, b.ClusterName) &&
rawExtensionDeepEqual(a.Status, b.Status, e) &&
e.DeepEqual(a.Applied, b.Applied) &&
e.DeepEqual(a.AppliedMessage, b.AppliedMessage)
},
)
}

func rawExtensionDeepEqual(a, b *runtime.RawExtension, checker *conversion.Equalities) bool {
if a == nil && b == nil {
return true
}
if a == nil || b == nil {
return false
}
aObj, errA := parseRawExtension(*a)
bObj, errB := parseRawExtension(*b)
if errA != nil && errB != nil {
return checker.DeepEqual(a, b) // fallback to directly compare the object
}
if errA != nil || errB != nil {
return false
}
return checker.DeepEqual(aObj, bObj)
}

func parseRawExtension(e runtime.RawExtension) (map[string]any, error) {
j, err := json.Marshal(e)
if err != nil {
return nil, err
}
if string(j) == "null" {
return nil, nil
}

obj := make(map[string]any)
err = json.Unmarshal(j, &obj)
Comment on lines +75 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind me why not just Unmarshal e.Raw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the reasons is that I think we should not limit runtime.RawExtension to use only Raw field. Maybe one day we want to start using Object field, and even if there are any obstacles at that time, they should not appear here.
Based on this, I think we should prefer semantic checks rather than memory checks. From e.MarshalJSON(), we can see that only one of the Raw and Object fields will take effect, and the final effect will be based on the JSON it generate. Based on the perspective of semantic checks, I converted them back to map[string]any for checking. This is not concise, but I think it is necessary.

if err != nil {
return nil, err
}
return obj, nil
}
Loading