Cleanup illegal_ports and ARC all of PI
#1040
Conversation
|
Skipping CI for Draft Pull Request. |
istio-testing
added
the
do-not-merge/work-in-progress
istio-testing
added
size/L
|
ah we do the test in |
We don't allow people to customize the listener ports (they're consts in the config which is where the listeners && illegal_ports both get them from) so if there are any tests that use dynamic ports, that's a bit suspect/spurious (but yeah, I don't think any do). |
|
@bleggett in |
Yeah I'm taking a look at those. |
istio-testing
added
the
needs-rebase
bleggett
force-pushed
the
bleggett/arc-pi
branch
2 times, most recently
from
3de0e8a
to
91e488a
Compare
istio-testing
removed
the
needs-rebase
What it boils down to is we break normal/prod behavior just for the purpose of some integration tests, and we have to add punchthrus to support providing a latebound HBONE port, when in every case outside of those few tests doing so would be a bug/a bad thing. Proper solution is to move everything in |
src/proxy.rs
Outdated
| // except in (some) contrived integ test cases (direct), where we bind to random non-well-known ports, | ||
| // and cannot rely on a well-known port - which is why this exists, so we can ignore | ||
| // the config object and sidechannel the non-fixed runtime-bound port. | ||
| #[cfg(any(test, feature = "testing"))] |
There was a problem hiding this comment.
Adding these conditional paths is risky - we effectively are now never testing the production codepath.
Why do we even need to split it? the test path will work for prod, no?
if we really want to verify we could just make an assertion conditional compiled in that hbone_port == 15008 or whatever want to check?
There was a problem hiding this comment.
We don't test the production codepath now - the production codepath can never use a dynamic port override, and we were making large chunks of (critical) state runtime-mutable just to enable this. Only tests can make use of it.
To be fair, I don't like the conditionals either though, and may just rewrite the tests to stop using dynamic port overrides.
Creating a runtime source of truth just to inject a duplicative static config value into it (but only for test flows) is both confusing to anyone that doesn't know why we are doing it, and a smell.
There was a problem hiding this comment.
We do test both today because we have code running dynamic and static ports (direct and namespaced). With this PR, the tests doesn't even compile the production codepath.
There was a problem hiding this comment.
With this PR, the tests doesn't even compile the production codepath.
Right, and those tests still pass, because they were never testing that particular production codepath to begin with.
The only tests I removed were ones that were using non-production punchthrus.
The only tests that have ever hit this particular codepath directly relied on doing stuff you can't/shouldn't do in production. It's a catch 22. I'll move the direct tests to namespaced and then we should be able to drop the test-only assumptions/mess.
There was a problem hiding this comment.
Namespacing every test in direct.rs is expensive, so I picked something that should be a decent compromise.
bleggett
added
the
do-not-merge/work-in-progress
istio-testing
added
the
needs-rebase
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
istio-testing
removed
the
needs-rebase
bleggett
removed
the
do-not-merge/work-in-progress
| socks5_addr: Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0)), | ||
| admin_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| readiness_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| stats_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| outbound_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| inbound_plaintext_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| dns_proxy_addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| illegal_ports: HashSet::new(), // for "direct" tests, since the ports are latebound, we can't test illegal ports |
There was a problem hiding this comment.
If we are not setting this, do we need to fix inbound_addr to a non-zero port?
The current approach will flake if you run multiple tests concurrently like cargo-nextest or others (I run it to detect flakes, etc)
There was a problem hiding this comment.
How bad is it to pass the hbone port still but keep the illegal_ports?
There was a problem hiding this comment.
Sorry I feel a bit like https://xkcd.com/1172/...
There was a problem hiding this comment.
The current approach will flake if you run multiple tests concurrently like cargo-nextest or others (I run it to detect flakes, etc)
Ah ok. We could probably work around this by using the namespaced ctor stuff (not isolated per-test, but isolated per-run)
There was a problem hiding this comment.
Still gross, but gross in one signposted spot. Should do for nextest and friends. Does for me.
bleggett
force-pushed
the
bleggett/arc-pi
branch
2 times, most recently
from
fc27069
to
beebe75
Compare
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
We were using the `cfg` in pool since istio#1040. `cfg` is not whether its used, but rather the intent of the user -- we may enable or disable at runtime. This renames the field to make it clear it should not be used like this, and fixes the issue. This went through CI due to istio#1084.
* Fix regression in source IP spoofing We were using the `cfg` in pool since #1040. `cfg` is not whether its used, but rather the intent of the user -- we may enable or disable at runtime. This renames the field to make it clear it should not be used like this, and fixes the issue. This went through CI due to #1084. * cleanup
This is mostly just pushing some minor local cleanups.
Arcall of ProxyInputs. We can probably de-arc some of the bits inside so they aren't double-arc'd eventually, but handlers shouldn't be able to get mut-ref to anything in PI - if they need to, it doesn't belong in PI.Put
illegal_portsin config - it is effectively config anyway - all the values are sourced from config, and everywhere we useillegal_portswe already have config.General goal is to make sure handlers keep a clean distinction between runtime-mutable state (their inner state/explicit mutable args) and state that should not be runtime-mutable (static config, proxyinputs)
We have tests that (ab)use this lack of immutability which this also twiddles a bit, mostly just enough to prevent us from writing more tests like that.