Make resolver.Address.Attributes comparable

[jhump]

With the move from Metadata interface{} to Attributes *attributes.Attributes to store custom properties on an address, the technique that most balancers use (including all balancers built using google.golang.org/grpc/balancer/base) to determine if an address is new or not does not work.

They simply use the resolver.Address struct as a map key. But a resolver can return the same address with the same custom attributes, but with a different *attributes.Attributes pointer value. This causes the balancer to think it's a different address and end up churning the connection (e.g. creating a new sub-conn and closing the existing one).

Working around this in a resolver -- returning stable pointer values in the address for identical attribute values -- is a non-trivial implementation burden and attempts to do so could easily lead to inadvertent memory leaks.

[dfawley]

Just to update here: we have several options on the table, that fit into two categories:

1. Make Addresses, with their included Attributes hashable/comparable/serializable/etc.

2. Strip Attributes from Addresses when hashing/comparing/serializing/etc.

C currently does (1). Java currently does (2). It's important to note that Java is structured much more similarly to Go, and C does subchannel (SubConn) sharing between channels (ClientConns), unlike Go and Java.

My concern with (2) is that Attributes could impact things like security, since the credentials will have access to them. It feels wrong to me to assume any differences in attributes are unimportant if there is a chance the transport or credentials could read them and behave differently depending upon them.

On the other hand, if an attribute is set on an address that nothing downstream (transport/credentials) will observe, it's also not necessary to treat a change in that attribute as a new address.

cc @markdroth @ejona86

[jhump]

I think option 1 gives the most flexibility since it would not preclude 2: a balancer impl, for example, could still choose to strip attributes and achieve option 2 if it so wanted.

[menghanl]

Another concrete use case where this would cause problem is address hierarchy.

Even when the last hierarchy string is removed, the attributes in address isn't cleared. So from roundrobin's point of view, it's a different address with a different attributes, though it's an empty attributes. This will affect balancers using hierarchy (e.g. weighted_target).

[ejona86]

The problem with (1) is that all consumers may need to be aware the producer exists. The problem with (2) is a producer can't differentiate based on attributes. That problem is more severe in C core when mixed with subchannel sharing.

My concern with (2) is that Attributes could impact things like security, since the credentials will have access to them. It feels wrong to me to assume any differences in attributes are unimportant if there is a chance the transport or credentials could read them and behave differently depending upon them.

(2) assumes "it is okay to use old values with existing connections." New connections will use the new attributes. It is whether the subchannel needs to reconnect. Today all transport security information is only verified at the beginning of the connection; even if a certificate expires the connection continues being used and the identity considered valid.

[dfawley]

This finally became the highest item on our list of things discussed in our weekly meeting.

We decided we should require attribute entries to implement a comparator. If we don't have this feature, everything we have today could still work, but we wouldn't be able to add subchannel sharing in the future. In addition, we will need a bit (or two different maps?) to indicate whether a difference in that attribute could matter to the subchannel.

[easwars]

Added the fixit label so that we can finalize a plan for this.

[dfawley]

This was addressed by #4855.