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 broken controllerutils.ObjectsEqual #199

Closed
ashleywang1 opened this issue Jan 16, 2021 · 1 comment · Fixed by #200
Closed

Fix broken controllerutils.ObjectsEqual #199

ashleywang1 opened this issue Jan 16, 2021 · 1 comment · Fixed by #200
Assignees

Comments

@ashleywang1
Copy link
Contributor

This is for the case where proto.Message types are passed in by value rather than by pointer.
For example:

var a interface{}
a = &fed_types.FailoverSchemeStatus{}
b, ok := a.(proto.Message) // ok == true, this will work
var a interface{}
a = fed_types.FailoverSchemeStatus{}
b, ok := a.(proto.Message) // ok == false, this will not work

The upgrade to using gogo/protobufs resulted in several changes around how protos are considered equal.

Previously, we made a change to fix this:
// DeepEqual should be used in place of reflect.DeepEqual when the type of an object is unknown and may be a proto message.
// see golang/protobuf#1173 for details on why reflect.DeepEqual no longer works for proto messages

The way our reconcilers pass in objects into Upsert may result in these items being passed in by value

// in some cases:
type GlooInstance struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Metadata *ObjectMeta               `protobuf:"bytes,1,opt,name=metadata,proto3" json:"metadata,omitempty"`
	Spec     *types.GlooInstanceSpec   `protobuf:"bytes,2,opt,name=spec,proto3" json:"spec,omitempty"`
	Status   *types.GlooInstanceStatus `protobuf:"bytes,3,opt,name=status,proto3" json:"status,omitempty"`
}

// In other cases:
// GlooInstance is the Schema for the glooInstance API
type GlooInstance struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   GlooInstanceSpec   `json:"spec,omitempty"`
	Status GlooInstanceStatus `json:"status,omitempty"`
}

Solution: be more flexible in the controllerutils.ObjectsEqual, and be able to deal with both cases.
See: gogo/protobuf#476

@ashleywang1
Copy link
Contributor Author

ashleywang1 commented Jan 22, 2021

Additional notes:

The previous comparison was being done here:

equalityutils.DeepEqual:

func DeepEqual(val1, val2 interface{}) bool {
	protoVal1, isProto := val1.(proto.Message)
	if isProto {
		protoVal2, isProto := val2.(proto.Message)
		if !isProto {
			return false // different types
		}
		return proto.Equal(protoVal1, protoVal2)
	}
	return reflect.DeepEqual(val1, val2)
}

The issue only happened when we were comparing two objects with a field that was passed in by value, e.g.

type GlooInstance struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`
	Spec   GlooInstanceSpec   `json:"spec,omitempty"`
	Status GlooInstanceStatus `json:"status,omitempty"`
}

The GlooInstanceSpec passed in by value would sometimes have a .status field (usually ignored) that had metadata information from kubernetes. We were not correctly casting it to (proto.Message) in the DeepEqual function. As a result, the DeepEqual function would use reflect.DeepEqual - which would take the hidden .status field into account, and incorrectly return false - that these two objects were not equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant