filter-state: make ProxyProtocolFilterState mutable

[adisuissa]

Commit Message: filter-state: make ProxyProtocolFilterState mutable
Additional Description:
When inserting a key-value to the filter-state, one needs to define whether that key-value is ReadOnly or Mutable.
In conn_manager_impl:188 the insertion using a ReadOnly mode, whereas in #42824, the tcp_proxy insertion was modified from ReadOnly to Mutable.
Modifying the conn_manager_impl to also be mutable, as the current state triggers [ENVOY_BUG] (

envoy/source/common/stream_info/filter_state_impl.cc

Lines 123 to 124 in e194c53

	IS_ENVOY_BUG(fmt::format("FilterStateAccessViolation: FilterState accessed "
	"immutable data as mutable: {}.",

).

Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44308 was opened by adisuissa.

see: more, trace.

[adisuissa]

Assigning @paul-r-gall who looked at this internally, and @agrawroh and @ggreenway who reviewed #42824
/assign @paul-r-gall @agrawroh @ggreenway

[ggreenway]

Should we remove this setting from filter-state and just make everything mutable? Are we getting any value from the read-only setting? It feels like it only leads to bugs like this.

[adisuissa]

Should we remove this setting from filter-state and just make everything mutable? Are we getting any value from the read-only setting? It feels like it only leads to bugs like this.

That is an excellent question that I've asked myself, but I don't have sufficient context.
My guess was that this "complexity" was added for a reason, or otherwise just a mutable get/set would have been supported.

[ggreenway]

Should we remove this setting from filter-state and just make everything mutable? Are we getting any value from the read-only setting? It feels like it only leads to bugs like this.

That is an excellent question that I've asked myself, but I don't have sufficient context. My guess was that this "complexity" was added for a reason, or otherwise just a mutable get/set would have been supported.

I was there when this discussion was made. The mutable/read-only was added because someone feared that not adding it could lead to difficulties understanding where changes to the data were made. I never thought it was a strong argument. I don't recall who it was, but it was not one of the maintainers who insisted on it. I think I'll take a shot at removing it; we can discuss further in the comments on that PR when I post it.

[ggreenway]

@adisuissa I just posted #44343 which removes the checks.