refactor(policy): generalize route types in outbound index #12664

the-wondersmith · 2024-05-30T23:26:52Z

Subject

Prepare to expand the subset of Gateway API route types linkerd supports driving outbound traffic with in linkerd-policy-controller-k8s-index.

Problem

Currently, the policy controller's index component is written with HTTPRoute support (effectively) exclusively, both in its structure/organization as well as its naming (e.g. HttpRoute as a primary type name, convert_http_route as a method name, etc...).

In order to expand the subset of route types defined by the Gateway API that linkerd supports for driving outbound traffic policy, the policy controller's index component needs to be made more generic in both respects.

Solution

NOTE: PR is branched from the policy-refactor-core-grpc-generalize-route-types branch, so the diff will be appear larger than it actually is until after #12662 has been merged.

PR introduces structural and naming changes making the codebase generic with respect to the type of route being handled (e.g. HTTPRoute -> Route). Changes are largely cosmetic, with no behavioral changes introduced by any functional refactoring.

Validation

maintainer review
tests pass

Fixes Lays Groundwork For Addressing

policy: Support Gateway API GRPCRoute resources #12404

Signed-off-by: Mark S <the@wondersmith.dev>

…ute-types

olix0r

It looks like Cargo.lock is updating many dependencies. Is that required for this change? Otherwise, we should update them with our normal dependency update process.

the-wondersmith · 2024-06-03T19:06:37Z

It looks like Cargo.lock is updating many dependencies. Is that required for this change? ...

The itertools crate offers some conveniences that let me simplify a couple of spots. Luckily, itertools is already a transient dependency from prost-derive [ref] so it's not a new addition to the lock file 🙂.

The changes in the lockfile should be correct now though:

adding itertools to linkerd-policy-controller-grpc
removing k8s-gateway-api from linkerd-policy-controller-k8s-index

Signed-off-by: Mark S <the@wondersmith.dev>

adleong · 2024-06-03T22:37:18Z

policy-controller/grpc/src/outbound.rs

-                (None, Some(_)) => return std::cmp::Ordering::Greater,
-            };
-            by_ts.then_with(|| a_name.name.cmp(&b_name.name))
+        let accrual = outbound.accrual.map(|accrual| outbound::FailureAccrual {


is there a reason this was moved up? can we keep it where it was to minimize noise in the diff and the git history?

It was moved up because the value is used in the match outbound.routes arms below it, so it needs to be bound before that match is evaluated.

policy-controller/grpc/src/outbound.rs

adleong · 2024-06-03T22:49:04Z

policy-controller/core/src/outbound.rs

+
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum OutboundRouteCollection {
+    Http(HashMap<GroupKindNamespaceName, OutboundRoute<HttpRouteMatch>>),


What do you think of adding an Empty variant to this enum and then making routes an OutboundRouteCollection instead of an Option<OutboundRouteCollection>. (we would use OutboundRouteCollection::Empty instead of None)

The advantage of this is it lets this type internalize more of the state logic. E.g. calling insert with an HttpRoute while in the Empty state transitions to the OutboundRouteCollection::Http state. Calling insert with a GrpcRoute while in the Http state returns an error. Calling remove which causes the HashMap to become empty transitions it to the Empty state. etc. etc.

Internalizing this state logic lets us get rid of functions like default_collection_for.

I agree with all of that. I went the Option route to reduce the diff, the amount of additional/future code maintenance introduced, etc...

If there's buy in for it, I think your suggestion would for sure be a more polished way to go

@adleong implemented and pushed the changes to make this happen. Looking forward to your thoughts

adleong · 2024-06-03T22:56:42Z

policy-controller/k8s/index/src/outbound/index.rs

+                    None => {}
+                    Some(OutboundRouteCollection::Http(routes)) => {
+                        service_routes
+                            .insert_producer_and_consumer_routes(routes, self.namespace.as_str());


what's the motivation for moving this into a function?

Code duplication reduction. It doesn't happen here (yet) because there's only one variant of OutboundRouteCollection, but once the Grpc variant is introduced all the code that's now inside insert_producer_and_consumer_routes would be duplicated otherwise.

policy-controller/k8s/index/src/outbound/index.rs

…efactor-index-outbound-generalize-route-types Signed-off-by: Mark S <the@wondersmith.dev> # Conflicts: # policy-controller/core/src/outbound.rs # policy-controller/grpc/src/outbound.rs # policy-controller/k8s/index/src/http_route.rs # policy-controller/k8s/index/src/inbound/http_route.rs # policy-controller/k8s/index/src/inbound/tests/authorization_policy.rs # policy-controller/k8s/index/src/outbound/index.rs # policy-controller/k8s/index/src/outbound/tests/routes/http.rs # policy-controller/k8s/status/src/index.rs # policy-controller/k8s/status/src/resource_id.rs # policy-controller/k8s/status/src/tests/http_routes.rs

Signed-off-by: Mark S <the@wondersmith.dev>

…ute-types

adleong · 2024-06-04T22:50:07Z

What do you think about something like this? https://github.com/linkerd/linkerd2/compare/alex/std-mem-i-dont-know-her?expand=1

I replaced std::mem::replace with simple assignment. I also made it an invariant that we cannot be in a non-Empty state with an empty map so that we don't have two different states which represent the same thing. And combined a couple of match statements to tighten things up. LMK what you think.

Signed-off-by: Mark S <the@wondersmith.dev>

policy-controller/core/src/outbound.rs

policy-controller/grpc/src/outbound.rs

policy-controller/k8s/index/src/outbound/index.rs

the-wondersmith · 2024-06-04T23:08:32Z

What do you think about something like this? https://github.com/linkerd/linkerd2/compare/alex/std-mem-i-dont-know-her?expand=1

I replaced std::mem::replace with simple assignment. I also made it an invariant that we cannot be in a non-Empty state with an empty map so that we don't have two different states which represent the same thing. And combined a couple of match statements to tighten things up. LMK what you think.

Love it, love everything about it (especially the branch name). Irritatingly, when I originally tried to implement the internal transition to and from Empty as *self = Self::Empty the compiler just straight up refused to cooperate with anything other than replace (no idea why). Your version is way better 😅.

I pulled the changes over and pushed them up 😁

Signed-off-by: Mark S <the@wondersmith.dev>

…ute-types

Signed-off-by: Mark S <the@wondersmith.dev>

the-wondersmith added 4 commits May 30, 2024 14:38

refactor: preliminary name changes to make route types generic

2c08f77

Signed-off-by: Mark S <the@wondersmith.dev>

style: cargo fmt

b6dbafc

Signed-off-by: Mark S <the@wondersmith.dev>

revert: revert lock changes

c053346

Signed-off-by: Mark S <the@wondersmith.dev>

refactor: generalize outbound route types

b397fe8

Signed-off-by: Mark S <the@wondersmith.dev>

the-wondersmith requested a review from a team as a code owner May 30, 2024 23:26

the-wondersmith changed the title ~~Policy refactor index outbound generalize route types~~ refactor(policy): generalize route types in outbound index May 31, 2024

Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…

ce8a72e

…ute-types

olix0r reviewed Jun 3, 2024

View reviewed changes

the-wondersmith added 3 commits June 3, 2024 15:08

revert(deps): revert lockfile changes

be33011

Signed-off-by: Mark S <the@wondersmith.dev>

revert(deps): revert dep changes

acf6102

Signed-off-by: Mark S <the@wondersmith.dev>

chore(deps): fix bad lockfile update

229d892

Signed-off-by: Mark S <the@wondersmith.dev>

adleong reviewed Jun 3, 2024

View reviewed changes

the-wondersmith added 6 commits June 4, 2024 09:41

style: consolidate imports from same crate

584fd1b

Signed-off-by: Mark S <the@wondersmith.dev>

chore: remove resolved todo

c90c5fd

Signed-off-by: Mark S <the@wondersmith.dev>

style: revert for_each calls back to explicit for loops

fb040c9

Signed-off-by: Mark S <the@wondersmith.dev>

refactor: add Empty variant to OutboundRouteType

ac269f0

Signed-off-by: Mark S <the@wondersmith.dev>

Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…

61faf69

…ute-types

perf: improve implementation of insert and remove

153e350

Signed-off-by: Mark S <the@wondersmith.dev>

adleong reviewed Jun 4, 2024

View reviewed changes

policy-controller/core/src/outbound.rs Outdated Show resolved Hide resolved

policy-controller/grpc/src/outbound.rs Outdated Show resolved Hide resolved

policy-controller/k8s/index/src/outbound/index.rs Outdated Show resolved Hide resolved

the-wondersmith added 2 commits June 4, 2024 19:13

perf: remove unnecessary emptiness check

ad46e78

Signed-off-by: Mark S <the@wondersmith.dev>

style: revert change to filter vs if in ServiceRoutes::apply

817907c

Signed-off-by: Mark S <the@wondersmith.dev>

adleong approved these changes Jun 4, 2024

View reviewed changes

the-wondersmith added 2 commits June 5, 2024 11:08

Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…

92d4ccb

…ute-types

bump

259fc2c

Signed-off-by: Mark S <the@wondersmith.dev>

adleong merged commit d734df0 into linkerd:main Jun 5, 2024
27 checks passed

the-wondersmith deleted the policy-refactor-index-outbound-generalize-route-types branch June 5, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(policy): generalize route types in outbound index #12664

refactor(policy): generalize route types in outbound index #12664

the-wondersmith commented May 30, 2024 •

edited

Loading

olix0r left a comment

the-wondersmith commented Jun 3, 2024 •

edited

Loading

adleong Jun 3, 2024

the-wondersmith Jun 4, 2024

adleong Jun 3, 2024

the-wondersmith Jun 4, 2024

the-wondersmith Jun 4, 2024

adleong Jun 3, 2024

the-wondersmith Jun 4, 2024

adleong commented Jun 4, 2024

the-wondersmith commented Jun 4, 2024

refactor(policy): generalize route types in outbound index #12664

refactor(policy): generalize route types in outbound index #12664

Conversation

the-wondersmith commented May 30, 2024 • edited Loading

Subject

Problem

Solution

Validation

Fixes Lays Groundwork For Addressing

olix0r left a comment

Choose a reason for hiding this comment

the-wondersmith commented Jun 3, 2024 • edited Loading

adleong Jun 3, 2024

Choose a reason for hiding this comment

the-wondersmith Jun 4, 2024

Choose a reason for hiding this comment

adleong Jun 3, 2024

Choose a reason for hiding this comment

the-wondersmith Jun 4, 2024

Choose a reason for hiding this comment

the-wondersmith Jun 4, 2024

Choose a reason for hiding this comment

adleong Jun 3, 2024

Choose a reason for hiding this comment

the-wondersmith Jun 4, 2024

Choose a reason for hiding this comment

adleong commented Jun 4, 2024

the-wondersmith commented Jun 4, 2024

the-wondersmith commented May 30, 2024 •

edited

Loading

the-wondersmith commented Jun 3, 2024 •

edited

Loading