can we disallow it being set and reject config if it is?

I originally had this change as using a oneof encapsulating the original xff_num_trusted_hops and the new xff_trusted_cidrs fields, but Mark told me that would be a breaking API change and to do it this way instead. Do you know if there's another way to handle rejecting it without it being a breaking change?

I don't think we want to disallow if both the old and new fields are set, since that makes it hard for control planes to start using this new feature.

If we reject when both fields are set, then a control plane must know whether any given client supports the new field. If it sets only the new field, old clients will not work properly, and if it sets both, new clients will break. So the only way to transition from the old field to the new field is to either (a) wait for all clients to be upgraded before starting to use the new field or (b) have some way for the control plane to know which client supports the new field and which clients support the old field so that it can send the right config to each one -- and doing that in a way that does not break cacheability requires some dances with dynamic parameter support, which means that the clients have to be configured to tell the control plane whether or not they support the new field.

In contrast, if we allow both fields to be set and specify the precedence order, then this becomes much easier. Control planes can simply set both fields. Old clients will use the old field, and new clients will prefer the new field.

I think the latter approach is much cleaner and should be preferred any time we add this kind of new field in xDS.

How do you feel about the change I made since Alyssa's feedback? d976354

She suggested it should either log or reject; I chose to reject by throwing an error with a message that clearly informs that only one field can be set.

What I'm saying is that I don't think that's the behavior we want. I think the way you had this originally was the right thing to do.

In that case I'm getting conflicting advice from two different maintainers :-)

@alyssawilk perhaps you can help decide which way I should take this based on Mark's feedback?

ooc when would we not want to recurse?

It's a good question! I added some commentary to the docs in 1f76584.

if every address in the XFF is trusted don't we want to return the last address in the list?

This one made me think! Originally I was assuming this might be some kind of undefined behaviour and so it was safer to return null, but after talking about this with a colleague, and looking over what other proxy servers do, I implemented your suggestion in a016a6c. Thanks for questioning my logic :-)

See, basically we only use the smart point as input parameter when we wants to change the ownership. But in this case, if we ensure this remote always be valid, then const Envoy::Network::Address::Instance& should be used.

If we can ensure this remote always be valid, then OptRef<const Envoy::Network::Address::Instance> should be used.

I think there should be the first case.

/wait

I think I understood you correctly. This is getting beyond my knowledge here so I wasn't quite sure. Please see the latest commit that now uses const Network::Address:Instance&.