Refactor and break out top-level filter module. #286

XAMPPRocky · 2021-06-02T09:35:31Z

This is built on-top and replaces the changes in #265 and represents and attempt to split up the filter_registry.rs file into more digestible chunks, and easier to search.

Changes

filter_registry.rs's data structures have been split up into set.rs, factory.rs, filters.rs, config.rs.
filter_chain.rs renamed chain.rs.
Added the FilterSet type which acts as the data structure that ensures we have a unique read-only set of filter factories.
Added a new filters::prelude, which is a prelude that contains everything you need to implement Filter and FilterFactory.
extensions is now named filters and extensions::filters is now filters::extensions.
- I've deliberately not touched anything in filters::extensions besides fixing their imports, those improvements can come in a followup PR.

There is no public changes in this PR, other than the addition of the FilterSet type, and the prelude module, all other changes are internal.

iffyio

LGTM! Please let's replace the current wildcard prelude::* imports with the actual items used in each file

iffyio · 2021-06-02T15:14:47Z

src/filters/chain.rs

-    CreateFilterArgs, Filter, FilterRegistry, ReadContext, ReadResponse, WriteContext,
-    WriteResponse,
-};
+use crate::filters::{prelude::*, FilterRegistry};


Let's replace the wildcard import with the items themselves?

Hmm, I'm not sure. I think this is a case where a wildcard is beneficial. It's convention that you wildcard import prelude modules. The prelude also has a pretty tight scope, it's FilterFactory, and every type in its type signature or used to implement the traits, and you do have to import a lot of types to implement these traits, and we already have a lot of filters, and the number will only grow. For something so common reducing the boilerplate to one-line here is worth it I think.

use quilkin::filters::prelude::*; struct A; struct AFactory; impl FilterFactory for AFactory { fn name(&self) -> String { // ... } fn create_filter(&self, args: CreateFilterArgs) -> Result<Box<dyn Filter>, Error> { // … } } impl Filter for A { fn read(&self, ctx: ReadContext) -> Option<ReadResponse> { // … } fn write(&self, ctx: WriteContext) -> Option<WriteResponse> { // … } }

Discussed on Slack.

Discussed on Slack.

Can you expand here please? Let's please avoid hidden/obfuscated communication channels where possible - or where they take place, take some notes and make sure they are attached to the appropriate place here in Github for an audit trail and historical decision making.

Can you expand here please? Let's please avoid hidden/obfuscated communication channels where possible - or where they take place, take some notes and make sure they are attached to the appropriate place here in Github for an audit trail and historical decision making.

Sure, and I understand that concern. There wasn't actually much of a discussion though more of just a thumbs up, I'll make that more clear next time.

Awesome. Thanks so much!

src/filters/factory.rs

markmandel

extensions is now named filters and extensions::filters is now filters::extensions

🤔 I expect we'll have other pluggable extension points in the future. Originally I would have seen them sit under the extensions module? I guess, now, not so much? We flip that around to being <thing>/extensions where <thing> has the core logic?

src/filters/extensions.rs

markmandel · 2021-06-02T18:58:00Z

src/filters/mod.rs

@@ -0,0 +1,75 @@
+/*
+ * Copyright 2020 Google LLC All Rights Reserved.


Should this be src/filters.rs ?

Yep, just kept it this way till the switch was merged.

iffyio · 2021-06-02T19:13:06Z

extensions is now named filters and extensions::filters is now filters::extensions

I expect we'll have other pluggable extension points in the future. Originally I would have seen them sit under the extensions module? I guess, now, not so much? We flip that around to being <thing>/extensions where <thing> has the core logic?

Ah right, the idea was that filters is an extension to quilkin rather, coming to think of it then 'filters/extensions' won't make as much sense - the current 'filters/extensions' contains the actual filters themselves which makes the extensions part redundant. maybe we can keep 'extensions/filters' if we think there'll be more kinds of extensions to quilkin in the future, or we can get rid of 'extensions' entirely and just say filters?

markmandel · 2021-06-02T19:24:54Z

extensions is now named filters and extensions::filters is now filters::extensions

I expect we'll have other pluggable extension points in the future. Originally I would have seen them sit under the extensions module? I guess, now, not so much? We flip that around to being <thing>/extensions where <thing> has the core logic?

Ah right, the idea was that filters is an extension to quilkin rather, coming to think of it then 'filters/extensions' won't make as much sense - the current 'filters/extensions' contains the actual filters themselves which makes the extensions part redundant. maybe we can keep 'extensions/filters' if we think there'll be more kinds of extensions to quilkin in the future, or we can get rid of 'extensions' entirely and just say filters?

Admittedly, this was also a copy of the Envoy package format:
https://github.com/envoyproxy/envoy/tree/main/source/extensions

XAMPPRocky · 2021-06-03T05:16:51Z

I expect we'll have other pluggable extension points in the future. Originally I would have seen them sit under the extensions module? I guess, now, not so much? We flip that around to being /extensions where has the core logic?

Well to be honest I'm not sure extensions makes sense as part of the module structure at all in Rust, not that we can't have other pluggable points in the future, I just don't feel like they need to prefixed with extensions. "Extension" implies to me that's it somehow opt-in and separate from the core logic, where as these are the defaults and you have to opt out of them. Especially the filter traits, these feel core to quilkin, as it's not like you can opt-out of the filter traits when using quilkin as a library.

Extensions in Rust are typically separate crates like if you think about for example serde is a set of traits and serde-json is an implementation of those traits. If someone wrote a quilkin-dtls or quilkin-load-balancer crate that implements the Filter traits, that would be a filter extension in my mind.

iffyio · 2021-06-03T07:25:32Z

Well to be honest I'm not sure extensions makes sense as part of the module structure at all in Rust, not that we can't have other pluggable points in the future, I just don't feel like they need to prefixed with extensions

Makes sense to me! I'd be happy to drop the extensions part entirely

quilkin-bot · 2021-06-14T13:34:45Z

Build Succeeded 🥳

Build Id: c7363b01-7f2c-4f19-94f7-18d46521a13e

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/286/head:pr_286 && git checkout pr_286
cargo build

markmandel · 2021-06-15T22:23:13Z

docs/extensions/filters/filters.md

@@ -45,7 +45,7 @@ There are a few things we note here:
 ### Configuration Examples ###

 ```rust
-# // Wrap this example within an async main function since the
+# // Wrap this example within an agync main function since the


Suggested change

# // Wrap this example within an agync main function since the

# // Wrap this example within an async main function since the

Nit: introduced a typo

markmandel · 2021-06-15T22:32:39Z

src/filters/factory.rs

+use crate::filters::{ConfigType, Error, Filter};
+
+/// An owned pointer to a dynamic [`FilterFactory`] instance.
+pub type DynFilterFactory = Box<dyn FilterFactory>;


Nice touch 👍🏻

markmandel · 2021-06-15T22:39:01Z

LGTM, just a small nit, and want to capture that decision that was come to offline.

That and now we have proc macros, we have a stack of conflicts to resolve 👍🏻

quilkin-bot · 2021-06-16T11:02:55Z

Build Succeeded 🥳

Build Id: c41970fe-b7dc-42fe-a582-4e3fdf47e032

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/286/head:pr_286 && git checkout pr_286
cargo build

quilkin-bot · 2021-06-16T17:58:11Z

Build Succeeded 🥳

Build Id: 01c5f0bb-7a83-413e-a7a5-550c8434d18e

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/286/head:pr_286 && git checkout pr_286
cargo build

markmandel · 2021-06-16T18:05:50Z

LGTM. @iffyio if you are also good to go, please feel free to merge 👍🏻

quilkin-bot · 2021-06-17T07:51:47Z

Build Succeeded 🥳

Build Id: c8e1e582-e64d-4cb5-ba6a-76f234108ac9

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/286/head:pr_286 && git checkout pr_286
cargo build

XAMPPRocky requested review from markmandel and iffyio June 2, 2021 09:35

google-cla bot added the cla: yes label Jun 2, 2021