Refactor build_requests #1082

howardjohn · 2024-05-21T22:16:31Z

This does a general cleanup of a lot of accumulated cruft. Lots of minor changes, but main ones:

Use Arc in more places
Rework Request and pstream type: rename fields to make it clear what are attributes of original workload/waypoint/network gatways. No more "gateway_address", especially not the mutable aspect
Collapse the fetch upstream, pick_workload_destination, compute various identity sets/SocketAddrs. Now one function does it all for us.
State has metrics now so we don't need to pass it in most functions

istio-testing · 2024-05-21T22:16:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

bleggett · 2024-05-22T15:08:25Z

src/proxy/outbound.rs

        let result_tracker = Box::new(ConnectionResult::new(
            source_addr,
-            req.gateway,
+            req.actual_destination,


bleggett

LGTM, mostly nits, will put a hold which you can self-remove.

bleggett · 2024-05-22T15:11:35Z

src/proxy/outbound.rs

@@ -329,6 +278,14 @@ impl OutboundConnection {
            .body(())
            .expect("builder with known status code should not fail");

+        let pool_key = Box::new(pool::WorkloadKey {
+            src_id: req.source.identity(),
+            // Clone here shouldn't be needed ideally, we could just take ownership of Request.


Yeah - why construct the poolkey here at all? We can just chuck the whole Box<request> + remote_addr at send_request_pooled?.

We have the same clone problem either way I think (actually, maybe not since we Box::pin the future).

If we pass owned Request here we can easy deconstruct the request and not clone upstream_sans. But then we end up paying for the stack cost of Request twice (and I think it somehow actually becomes 4x due to some async stuff?). If we have a reference we don't need to, but then we need to clone this. If we kick this down further into send_request_pooled we have the same problem, just possibly more stack usage I think?

I guess maybe an owned Box<Request> wouldn't have that problem...

I guess maybe an owned Box wouldn't have that problem...

That would be my intuition, but IDK for sure. Eh, it's a small thing either way.

bleggett · 2024-05-22T15:14:59Z

src/proxy/outbound.rs

+            {
+                let upstream_sans = waypoint.workload_and_services_san();
+                let actual_destination = waypoint.workload_socket_addr();
+                trace!("built request to service waypoint proxy");


Suggested change

trace!("built request to service waypoint proxy");

debug!("built request to service waypoint proxy");

Mega nit: slight pref for logs indicating request processing decisions to be debug! - trace is a lot of noise.

bleggett · 2024-05-22T15:15:19Z

src/proxy/outbound.rs

-            }
+            .await?
+        else {
+            trace!("built request as passthrough; no upstream found");


bleggett · 2024-05-22T15:15:52Z

src/proxy/outbound.rs

-    // The identity we will assert for the next hop; this may not be the same as destination_workload
+    // Source workload sending the request
+    source: Arc<Workload>,
+    // The actual destination workload we are targeting. When proxying through a waypoint, this is the waypoint,


👍 for the comments/naming here, extreme obviousness is good.

bleggett · 2024-05-22T15:19:19Z

src/state.rs

        &self,
        network: Strng,
        source_workload: &Workload,
        addr: SocketAddr,
-    ) -> Option<Upstream> {
+    ) -> Option<(Arc<Workload>, u16, Option<Arc<Service>>)> {


nit: maybe type this (FoundUpstream)?

This reverts commit a25984a.

This reverts commit 57e12c5.

istio-testing · 2024-05-22T19:07:39Z

In response to a cherrypick label: #1082 failed to apply on top of branch "release-1.22":

Applying: wip
Using index info to reconstruct a base tree...
M	src/proxy.rs
M	src/proxy/inbound.rs
M	src/proxy/inbound_passthrough.rs
M	src/proxy/outbound.rs
M	src/state.rs
M	src/state/workload.rs
Falling back to patching base and 3-way merge...
Auto-merging src/state/workload.rs
Auto-merging src/state.rs
CONFLICT (content): Merge conflict in src/state.rs
Auto-merging src/proxy/outbound.rs
CONFLICT (content): Merge conflict in src/proxy/outbound.rs
Auto-merging src/proxy/inbound_passthrough.rs
Auto-merging src/proxy/inbound.rs
Auto-merging src/proxy.rs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 wip
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2024-05-22T19:07:40Z

In response to a cherrypick label: new issue created for failed cherrypick: #1087

* wip * wip * explicit request enum * Revert "explicit request enum" This reverts commit a25984a. * Revert "wip" This reverts commit 57e12c5. * wip * wip * proper size * cleanup * lint * minor cleanup * change log (cherry picked from commit c84404d)

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label May 21, 2024

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2024

howardjohn changed the title ~~refactor/build request~~ Refactor build_requests May 21, 2024

howardjohn force-pushed the refactor/build_request branch from b7eaf4e to 7cc15e9 Compare May 21, 2024 23:20

howardjohn marked this pull request as ready for review May 22, 2024 00:20

howardjohn requested a review from a team as a code owner May 22, 2024 00:20

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 22, 2024

howardjohn force-pushed the refactor/build_request branch from 8efb13b to c59d4ae Compare May 22, 2024 00:27

howardjohn assigned bleggett May 22, 2024

howardjohn added the cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch label May 22, 2024

howardjohn force-pushed the refactor/build_request branch from 264555a to f49d153 Compare May 22, 2024 01:30

bleggett reviewed May 22, 2024

View reviewed changes

bleggett added the do-not-merge/hold Block automatic merging of a PR. label May 22, 2024

bleggett approved these changes May 22, 2024

View reviewed changes

howardjohn added 11 commits May 22, 2024 08:33

wip

351d182

wip

7963f66

explicit request enum

d8b3bbe

Revert "explicit request enum"

5d17ebe

This reverts commit a25984a.

Revert "wip"

c5afa17

This reverts commit 57e12c5.

wip

14a0ed2

wip

1b0589d

proper size

182e15a

cleanup

4ee4871

lint

6d2052e

minor cleanup

09a9abc

howardjohn force-pushed the refactor/build_request branch 2 times, most recently from 4858ef3 to 0351fbe Compare May 22, 2024 15:36

change log

a374db6

howardjohn force-pushed the refactor/build_request branch from 0351fbe to a374db6 Compare May 22, 2024 17:55

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label May 22, 2024

istio-testing merged commit c84404d into istio:master May 22, 2024
3 checks passed

istio-testing mentioned this pull request May 22, 2024

[release-1.22] Refactor build_requests #1087

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor build_requests #1082

Refactor build_requests #1082

howardjohn commented May 21, 2024 •

edited

Loading

istio-testing commented May 21, 2024

bleggett May 22, 2024

bleggett left a comment

bleggett May 22, 2024 •

edited

Loading

howardjohn May 22, 2024

bleggett May 22, 2024

bleggett May 22, 2024

bleggett May 22, 2024

bleggett May 22, 2024

bleggett May 22, 2024

istio-testing commented May 22, 2024

istio-testing commented May 22, 2024

	trace!("built request to service waypoint proxy");
	debug!("built request to service waypoint proxy");

Refactor build_requests #1082

Refactor build_requests #1082

Conversation

howardjohn commented May 21, 2024 • edited Loading

istio-testing commented May 21, 2024

bleggett May 22, 2024

Choose a reason for hiding this comment

bleggett left a comment

Choose a reason for hiding this comment

bleggett May 22, 2024 • edited Loading

Choose a reason for hiding this comment

howardjohn May 22, 2024

Choose a reason for hiding this comment

bleggett May 22, 2024

Choose a reason for hiding this comment

bleggett May 22, 2024

Choose a reason for hiding this comment

bleggett May 22, 2024

Choose a reason for hiding this comment

bleggett May 22, 2024

Choose a reason for hiding this comment

bleggett May 22, 2024

Choose a reason for hiding this comment

istio-testing commented May 22, 2024

istio-testing commented May 22, 2024

howardjohn commented May 21, 2024 •

edited

Loading

bleggett May 22, 2024 •

edited

Loading