Add Regexp Targeting

[ttstarck]

Add Regexp matching to Targeting

The goal of these changes is to allow being able to dynamically target similar services that are only unique by a single context value. For example, if I had the following frontend services with names:

frontend-primary-1
frontend-primary-2
frontend-secondary-1
frontend-secondary-2

If I wanted to only target services with names that only match frontend-primary, that would not be possible with current targeting features. Of course, one might argue there should be some other context like region or deployGroup to designate these but there are cases where this might not be possible, extremely difficult to fully setup, or not worth the time for a setting that may only be temporary.

Summary of Changes

• Target.target_key_matches? will now check if the target_value is a Regexp object and run .matches on the context hash.

Example targeting:

target:
  service_name: !ruby/regexp /frontend-primary/

This target should only match frontend-primary-1 and frontend-primary-2 and not match frontend-secondary-1 and frontend-secondary-2 from the above example.

[ColinDKelley]

Great passion project!

Check out the power of let in my test suggestions. It's the most amazing part of rspec. It's so frustrating to switch over to the web repo and not have access to let in minitest!

[ColinDKelley]

⛏️ That's actually a "slash". \ is a backslash.

[ColinDKelley]

And I was glad to see in the tests and implementation that slashes are required at both the front and back of the string, so a path prefix like /tmp won't trigger it. At first I missed it in your documentation here, because it says "a backslash" (singular). How about:

By adding `/` at the front and end of a value, the pattern given between the slashes will be treated as a regular expression to match with the context.

^ Note it's more than just a substring--it's a regexp! Can you change the example to use some regexp special values, like maybe /frontend-\d{3}/ for 3 digits after the hyphen?

[ColinDKelley]

I'd use slashes here in the example since that's how Ruby would do it after =~:

that has `service_name =~ /frontend/

[ColinDKelley]

Oh, you need to anchor that regexp! As you have it written, a string like "network/advertiser/affiliate" would match! Can you add a test that fails on that?

Also, sadly, =~ is slightly non-preferred because it's slow. .match? is preferred when you don't need the MatchData back.

if target_value.match?(/\A\/.+\/\z/)

An alternative would be:

if target_value.start_with?('/') && target_value.end_with?('/') && target_value.size > 2

But...there's also this cool Regexp extraction notation (parens go around the group(s) you want to extract and then , 1 means "get me the first match group"). Note that I used /x in the Regexp so that whitespace can be added for readability.

if (pattern = target_value[/\A \/ (.+) \/ \z/x, 1])
  context_hash.match?(pattern)

[ColinDKelley]

subject and is_expected.to can make this sort of test read great. It's a bit more verbose, so feel free to skip it. But I really like how it reads. (I learned this from @jebentier BTW. It's now pretty common to see in Hydra tests.)

context 'when target value has a leading slash only' do
  let(:target_hash) { { 'service' => '/telecom' } }
  let(:process_target) { described_class.new(target_hash) }
  subject { process_target.target_key_matches?(context_hash) }

  describe 'when context_hash has the string but not the slash' do
    let(:context_hash) {
      {
          'service' => 'telecom',
          'region'  => 'east',
          'cdr'     => { 'caller' => '+18056807000' }
      }
    }
    it { is_expected.to be_falsey }
  end

  describe 'when context_hash is empty' do
    let(:context_hash) { {} }
    it { is_expected.to be_falsey }
  end

  describe 'when context_hash has an exact string match' do
    let(:context_hash) { { 'service' => '/telecom' } }
    it { is_expected.to be_truthy }
  end
end

[ColinDKelley]

And the failing test I mentioned above:

context 'when target value has embedded slashes (not at the front and back)' do
  let(:service) { 'network/advertiser|affiliate/path' } # | won't match if this is treated as a Regexp
  let(:target_hash) { { 'service' => service } 
  it { is_expected.to be_truthy }
end

[ColinDKelley]

You can DRY up the tests below by putting some common lets up here:

let(:context_hash) do
  {
    'service' => service,
    'region' => 'east',
    'cdr' => { 'caller' => '+18056807000' }
  }
end

The individual contexts just need to do let(:service) { ... }.

[ColinDKelley]

The singular is still a bit confusing. And the substring part isn't right--it'll be a regexp match!
How about this wording:

To provide a regular expression for matching, use a leading and trailing slash `/` around your expression. For example:

And then I'd make sure that the example is more than just a substring. That's why I suggested

  service_name: /frontend-\d/

and then maybe you can give an example that matches because of a digit and also one that fails because it's a non-digit there.

[ColinDKelley]

I changed the PR title to "Add Regexp Targeting". Can you change it here in the CHANGELOG and anywhere else in the PR?

[ttstarck]

Fixed c2e5de2

[jebentier]

I have a general question about the implementation. I found out today through a little research that Ruby's YAML parser has built in support for declaring a Regexp. How would we feel about allowing the use of this built in so that we appropraitely load a Regex into the target_value and don't need to do these string comparisons and extractions? The YAML syntax isn't the prettiest, but it's declarative

key: !ruby/regexp /frontend-\d/

This would reduce the necessary changes in the gem as well as speed up the comparison by not having to check every string value of the target every time.

[jebentier]

Can we put Unreleased for the release date until it's released?

[ColinDKelley]

Nice twist there to use actual regexps.

[ColinDKelley]

Whoa! This is really cool to not preclude slashes around strings. In the past we've been nervous about using this YAML ruby class support because it has a much wider surface area for security vulnerabilities (since this Ruby class will be invoked). That doesn't seem like a big deal here, but we might want to get an opinion from someone with a security background.

[ttstarck]

Here is the source for how Pysch parses it this value:
https://github.com/ruby/psych/blob/master/lib/psych/visitors/to_ruby.rb#L96-L110

[ttstarck]

@ColinDKelley @jebentier It looks like we technically are already open to this potential vulnerability! I can switch how we are parsing yaml to use Pysch.safe_load_file and specify allowing only Regexp as a permitted class besides the default permitted classes

[ttstarck]

See

process_settings/lib/process_settings/targeted_settings.rb

Line 76 in 6ff07b4

json_doc = Psych.load_file(file_path)

[ColinDKelley]

Good point--we are already vulnerable! And because the process_settings are set by developers, I think we'll be fine? Worth mentioning that to in the request for a security sign-off. (It's the cases where raw YAML is parsed from the outside world that have caused serious security problems. And that's why safe_load_file was invented.)

And if it doesn't add much scope, I really like your suggestion of switching over to Pysch.safe_load_file here with only Regexp permitted. But if that takes more than a couple minutes, feel free to create a ticket for Octothorpe instead.

[ttstarck]

Added: c6d10fe

[jebentier]

Changes in general look good to me, but Colin brings up a good point about pulling in someone from Security to make sure we're not exposing ourselves to a regex vulnerability.

[ColinDKelley]

Awesome work!

[ColinDKelley]

@ttstarck @jbuehring was hoping to use this feature. Anything blocking it from getting merged?