Support regex filter

[fredagsfys]

No description provided.

[XAMPPRocky]

Thank you for your PR! This mostly looks good to me, I've left a one comment. Once that is addressed this should be good to merge.

[XAMPPRocky]

The Regex type should be created in create_filter, stored, and re-used here. This implementation will compile the Regex for each packet.

[fredagsfys]

Hey, this PR is still under WIP and I’ll address your mentioned concern before making it ready for review ☺️ tis 31 aug. 2021 kl. 07:34 skrev XAMPPRocky ***@***.***>:

…

***@***.**** approved this pull request. Thank you for your PR! This mostly looks good to me, I've left a one comment. Once that is addressed this should be good to merge. ------------------------------ In src/filters/regex_bytes.rs <#387 (comment)> : > + +impl RegexBytes { + fn new(base: &Logger, config: Config, metrics: Metrics) -> Self { + RegexBytes { + log: base.new(o!("source" => "extensions::RegexBytes")), + //capture: config.strategy.as_capture(), + metrics, + regex_expression: Arc::new(config.regex_expression), + //metadata_key: Arc::new(config.metadata_key), + } + } +} + +impl Filter for RegexBytes { + fn read(&self, ctx: ReadContext) -> Option<ReadResponse> { + let re = Regex::new(self.regex_expression.as_str()).unwrap(); The Regex type should be created in create_filter, stored, and re-used here. This implementation will compile the Regex for each packet. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#387 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXAGUV2HT6DOPNOXEGSFMDT7RSURANCNFSM5DCXD2JQ> .

[iffyio]

Thanks for the PR! We have a capture bytes that does something pretty similar to this from what I can tell, could we enhance that filter with a new regex Strategy instead of implementing this as a new filter? https://github.com/googleforgames/quilkin/blob/main/src/filters/capture_bytes/capture.rs#L2-L7

[fredagsfys]

Thanks for the PR! We have a capture bytes that does something pretty similar to this from what I can tell, could we enhance that filter with a new regex Strategy instead of implementing this as a new filter? https://github.com/googleforgames/quilkin/blob/main/src/filters/capture_bytes/capture.rs#L2-L7

I was looking at this as well first, but realised pretty soon that the description/idea of how this regex should work differs from what CaptureBytes does today. It will also introduce complexity to configuration, e.g different supporting properties, errors/unsupported combinations.

Example

config:
 strategy: PREFIX
 metadataKey: myapp.com/myownkey
 size: 3
 remove: false

vs

config:
 strategy: REGEX
 metadataKey: myapp.com/myownkey
 expression: "(?P<cstr>[^\x00]+)\x00"
 on_success: "capture | pass"
 on_error: "drop" 

We probably could try to align and merge logic of these two filters, for example PREFIX/SUFFIX drops today as default if size req is not met, but for the rest it is different. I see this more as regex filter utilises capture as a partial feature but also has support for pass.

An idea on how we could align these two filters to one is to support a configuration like this

config:
 strategy: PREFIX | SUFFIX | REGEX
 metadataKey: myapp.com/myownkey
 on_success: "capture | pass"
 on_error: "drop" 
 
 expression: "(?P<cstr>[^\x00]+)\x00" <-- Req strategy REGEX
 size: 3  <-- Req strategy PREFIX | SUFFIX
 remove: false <-- Req strategy PREFIX | SUFFIX

Required properties throws validation errors on startup if missing to set strategy, wdyt?

[iffyio]

Ah I see, yeah the only difference I can seems to be the pass feature while all other things seem common to capture filter. in which case we can merge it into the capture filter and make the 'pass' part configurable.
Config-wise, we can ensure that only one of them is provided and that strategy arguments aren't mixed with each other using a one-of schema type (we currently do something similar for the static/dynamic fields of a config).
So a config would look like:

# prefix
config:
  metadataKey: myapp.com/myownkey
  prefix:  # deserialization ensures that exactly one of prefix,suffix,regex is provided
    size: 3
    remove: false

or

# regex
config:
  metadataKey: myapp.com/myownkey
  regex:
    expression: ".*"
    remove: false
    capture: true # or maybe e.g `passthrough: true` since capturing the bytes makes more sense as a default given the filter name

For the on_error: drop, I'd say we ignore that field config-wise since that's already what all filters do when things go wrong (we can introduce options for all affected filters when we do in fact have options in such scenarios)

[markmandel]

Ah I see, yeah the only difference I can seems to be the pass feature while all other things seem common to capture filter. in which case we can merge it into the capture filter and make the 'pass' part configurable.
Config-wise, we can ensure that only one of them is provided and that strategy arguments aren't mixed with each other using a one-of schema type (we currently do something similar for the static/dynamic fields of a config).

I just came here to say that I like this idea 👍🏻

[markmandel]

Just checking in to see if you need / want a hand in any way?

[quilkin-bot]

Build Failed 😭

Build Id: c7c2d223-6237-4246-9901-0c93d2a8a59a

Status: FAILURE

• Cloud Build view
• Cloud Build log download

To get permission to view the Cloud Build view, join the quilkin-discuss Google Group.

[fredagsfys]

Ah I see, yeah the only difference I can seems to be the pass feature while all other things seem common to capture filter. in which case we can merge it into the capture filter and make the 'pass' part configurable.
Config-wise, we can ensure that only one of them is provided and that strategy arguments aren't mixed with each other using a one-of schema type (we currently do something similar for the static/dynamic fields of a config).

I just came here to say that I like this idea 👍🏻

Thanks @markmandel, tbh I've been swamped at work and private latley. However, I'm now in a state where I'm fighting the language more than anything.

Two initial things I'd like your help with

• As how I've refactored the configuration, we now support a dynamic strategy property which turns the enum to a struct with properties over a enum variant. This breaks the map_proto_enum in config.rs. Is there a nice alternative to parse this without compromising the nice configuration structure @iffyio proposed?

• I'm trying to use composition to handle both something now called SizeCapture and RegexCapture joined under Capture trait. How can I get these to work in harmony in as_capture?

Last but not least, feel free to contribute if you think it is taking too much time... It is totally understandable. Rust is a new language for me and this turned out to be bigger than I anticipated in the first place! 😅

[iffyio]

Hi @devharis!

I'm trying to use composition to handle both something now called SizeCapture and RegexCapture joined under Capture trait. How can I get these to work in harmony in as_capture?

I think the simpler route would be have to all three strategies implement the Capture trait rather than introduce a new trait. The CaptureBytes filter would no longer need to know about things like remove or size etc, rather those belong to the implementation now that they differ - so something like

struct Suffix {
  remove: bool,
  size: usize,
}
struct Regex {
  regex: Regex,
  remove: bool,
}
impl Capture for Suffix {}
impl Capture for Regex {}

struct CaptureBytes {
  log: Logger,
  capture: Box<dyn Capture>,
  metadata_key: Arc<String>,
}
impl Filter for CaptureBytes {
  fn read(&self) {
    // I think this part works largely the same
    ...
    // Except no extra parameters needed in the `capture()` call
    // since the `self.capture` implementation already has all the
    // needed configuration.
    let token = self.capture.capture(ctx.contents)
    ...
  }
}

This breaks the map_proto_enum in config.rs

the map_proto_enum implementation only converts proto enums to plain rust enums that don't contain any data so likely we won't be able to use it anymore for this filter. We can instead do the mapping ourselves with e.g:

let strategy: Strategy = match protoconfig.strategy {
  ProtoStrategy::Suffix => {
     Strategy::Suffix {
        size: protoconfig.size,
        ...
     }
  },
}

(also sorry about the delayed feedback)

[XAMPPRocky]

Since this has bitrotted with little activity, and I wanted to add support for regex to test something, I've continued this work in #458 and am going to close this, I'll be sure to add you as a coauthor to it. Thank you again for your PR!