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

make pointer to value if needed #200

Merged
merged 3 commits into from
Jan 18, 2021
Merged

make pointer to value if needed #200

merged 3 commits into from
Jan 18, 2021

Conversation

ashleywang1
Copy link
Contributor

@ashleywang1 ashleywang1 commented Jan 16, 2021

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

BOT NOTES:
resolves #199

@solo-changelog-bot
Copy link

Issues linked to changelog:
#199

@ashleywang1 ashleywang1 marked this pull request as ready for review January 18, 2021 14:10
Copy link
Member

@EItanya EItanya left a comment

Choose a reason for hiding this comment

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

lgtm, please add test to verify

},
},
}
obj1.Spec.ProtoReflect().SetUnknown([]byte(""))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, when the Spec was being passed in by value, then line

protoVal1, isProto := val1.(proto.Message)

would return isProto as false, and use reflect.DeepEqual instead of the correct proto.Equal, which allows us to treat empty .status.atomicInfo fields as the same as nil .status.atomicInfo fields.

@soloio-bulldozer soloio-bulldozer bot merged commit 7b2a5dd into master Jan 18, 2021
@soloio-bulldozer soloio-bulldozer bot deleted the fix-equality branch January 18, 2021 16:47
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 this pull request may close these issues.

Fix broken controllerutils.ObjectsEqual
2 participants