linkerd / linkerd2

Ultralight, security-first service mesh for Kubernetes. Main repo for Linkerd 2.x.
https://linkerd.io
Apache License 2.0
10.48k stars 1.26k forks source link

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

Closed the-wondersmith closed 1 month ago

the-wondersmith commented 1 month ago

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

Screenshot 2024-05-30 at 7 26 34 PM

Fixes Lays Groundwork For Addressing

the-wondersmith commented 1 month ago

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:

adleong commented 1 month ago

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.

the-wondersmith commented 1 month ago

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 😁