The update of v4_compat flag for the listener will be ignored

[soulxu]

When updating a listener with a different v4_compat flag value, the new value will be ignored. When updating an existing listener, we will clone the socket from the old listener: https://github.com/envoyproxy/envoy/blob/01fb798af4276da0e4b93ac1a60287fc477598ad/source/server/listener_manager_impl.cc#L505

And the old v4_compat flag value will be clone also. Then the new value is lost.

The possible fix are:

• changing the ListenerImpl::hasCompatibleAddresses

https://github.com/envoyproxy/envoy/blob/01fb798af4276da0e4b93ac1a60287fc477598ad/source/server/listener_impl.cc#L1047

Then compare the v4_compat flag, if the value is different, then consider that as non-compatible address, then will create a new socket for the listener update. This would be an easy fix but this shouldn't work for reuse_port with false.

• updating the v6only flag and IPV6_V6ONLY option on the old socket, that will change the sematic of clone https://github.com/envoyproxy/envoy/blob/01fb798af4276da0e4b93ac1a60287fc477598ad/source/server/listener_impl.h#L72-L74

Pass the new v4_compat value to the clone, and generate new address instance and set the new value for IPV6_V6ONLY socket option. But not sure this option can be changed for already bind socket. Even it can be changed, the old listener loose the v4 compact capabilities before new listener warmed.

Also not sure there is really someone who want to update the v4_compat value. So the first question should be whether we want to support this? if not we can reject this directly also.

[soulxu]

@mattklein123 is this something we want to support? I guess this is a rare case people want to update the v4_compat flag. I just found this issue when debugging the crash bug. I prefer the first possibly fix or just reject this kind of update.

[mattklein123]

I would probably just reject it for now and see if anyone complains? IMO this code is complicated enough as it is and I would rather not support things no one is likely to ever use.

[soulxu]

I would probably just reject it for now and see if anyone complains? IMO this code is complicated enough as it is and I would rather not support things no one is likely to ever use.

thanks, agree that it already complicated enough

[kfaseela]

@mattklein123 @soulxu : I am facing similar issues with in-place updates for DSCP socket_options on the listener. The same at the cluster level works fine. We have requirement to set DSCP for QoS(Quality Of Service) purposes on the socket (see https://github.com/istio/istio/issues/40397), and currently we are using EnvoyFilter for the same. While setting things for the first time works, if I try to dynamically update the DSCP value on the socket, it does not go through. The problem is only observed on listener updates, and cluster updates for socket_options seem to work fine.

[soulxu]

If we are going to fix this, one simple solution in my mind is to not clone socket when socket options was changed.

[kfaseela]

If we are going to fix this, one simple solution in my mind is to not clone socket when socket options was changed.

Would be really good if we get this fixed.

[soulxu]

If we are going to fix this, one simple solution in my mind is to not clone socket when socket options was changed.

Would be really good if we get this fixed.

I can probably work on it at the end of the week or the beginning of next week. If you are interested to fix it first, then feel free to go for it. Or free to ping me if you still want to wait on me.

Note, you probably should take care of v4_compat flag code at ConnectionHandlerImpl, it doesn't take care the update of v4_compat flag well.

[soulxu]

If we are going to fix this, one simple solution in my mind is to not clone socket when socket options was changed.

After digging more, there is no way to ensure all the options work this way. (an example reuseport option won't work). So probably we should add a specific field for the DSCP option, just like fields 'enable_mptcp' and 'transparent'. We are going to update DSCP option when its value changes. And we can still reject the update of socket_options field, but it also means we are going to add more fields if people have more requirements on different socket option update. @mattklein123 WDYT?

[mattklein123]

After digging more, there is no way to ensure all the options work this way. (an example reuseport option won't work). So probably we should add a specific field for the DSCP option, just like fields 'enable_mptcp' and 'transparent'. We are going to update DSCP option when its value changes. And we can still reject the update of socket_options field, but it also means we are going to add more fields if people have more requirements on different socket option update. @mattklein123 WDYT?

I'm torn. I think on one hand it makes sense to be explicit about the options that we think can be updated, but this will expand the API and not necessarily be applicable to all platforms. The other way would be to actually look through the socket options on a platform specific basis and do a live update of the ones that we know do support it. This would avoid expanding the API but have more subtle platform implications that might be hard to reason about.

cc @envoyproxy/api-shepherds @ggreenway any thoughts on this?

[ggreenway]

Maybe for socket_options, add a config field for whether it is expected to be able to update on an existing socket or if a new listener needs to be created. I'd rather not add specific socket options as new explicit fields in the config just for this purpose.

But in general, a lot of the stuff around socket options has platform-dependent behavior, and I don't see a good way around that. Some features are only supported on some platforms, different behavior around how to update/change, etc.

[soulxu]

Maybe for socket_options, add a config field for whether it is expected to be able to update on an existing socket or if a new listener needs to be created. I'd rather not add specific socket options as new explicit fields in the config just for this purpose.

thanks! +1 for this. Also check with option's SocketState field, if it is STATE_PREBIND, then reject to update option on the cloned socket. You only can create new socket when it is STATE_PREBIND.

But in general, a lot of the stuff around socket options has platform-dependent behavior, and I don't see a good way around that. Some features are only supported on some platforms, different behavior around how to update/change, etc.

Is it terrible we just let the platform decide whether the option can be updated? like we expect the setsockopt failed or a later bind failed when it doesn't support. Even though we maintain a list of valid update options, The API still show different behavior on the different platform, look like same.

Also thinking we only support update for socket_options, we don't support the update of resuse_port, v4_compat, free_bind and transparent. It didn't see any complaints about updating those, then we needn't add more complex code to the listener. We can change that in the future if someone really needs it.

[mattklein123]

Is it terrible we just let the platform decide whether the option can be updated?

No I don't think this is terrible and IMO we can implement it this way. Each platform can decide whether it can update or not based on a passed in set of options, and if it can't the config will be NACKd. IMO this is a good direction to go as well.

[ggreenway]

Is it terrible we just let the platform decide whether the option can be updated?

No I don't think this is terrible and IMO we can implement it this way. Each platform can decide whether it can update or not based on a passed in set of options, and if it can't the config will be NACKd. IMO this is a good direction to go as well.

I'm in favor of this idea, but there is a risk of a partially-applied config update, eg I had two socket options in the config that changed, the first one successfully updates in-place, the 2nd one fails. We've now modified the existing listener socket options, but can't fully rollback (at least not without some really careful work to revert the already-set socket options to previous values).

[mattklein123]

I'm in favor of this idea, but there is a risk of a partially-applied config update, eg I had two socket options in the config that changed, the first one successfully updates in-place, the 2nd one fails. We've now modified the existing listener socket options, but can't fully rollback (at least not without some really careful work to revert the already-set socket options to previous values).

yeah I think it would have to be 2-phase and the platform would have to have prior knowledge whether it can be updated or not. I agree there is still a risk of bug with this approach but it seems like the best we can do.

[soulxu]

@mattklein123 @ggreenway thanks! let me work out a PR

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.