Verified Commit fd51ce57 authored by Bob Van Landuyt's avatar Bob Van Landuyt 💬 Committed by GitLab
Browse files

Merge branch 'spec-10-regex-matcher' into 'master'

feat(rate_limit): regex matchers in Rule#match (Spec 10)

See merge request !283

Merged-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Approved-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Reviewed-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Reviewed-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: default avatarSam Wiskow <swiskow@gitlab.com>
parents ae9140c5 9538c34f
Loading
Loading
Loading
Loading
Loading
+1 −0
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ module Labkit
  module RateLimit
    autoload :Configuration, "labkit/rate_limit/configuration"
    autoload :Identifier, "labkit/rate_limit/identifier"
    autoload :Matcher, "labkit/rate_limit/matcher"
    autoload :Result, "labkit/rate_limit/result"
    autoload :Rule, "labkit/rate_limit/rule"
    autoload :Evaluator, "labkit/rate_limit/evaluator"
+1 −1
Original line number Diff line number Diff line
@@ -66,7 +66,7 @@ module Labkit
      end

      def rule_matches?(rule, identifier)
        rule.match.all? { |key, value| identifier[key] == value }
        rule.match.all? { |key, matcher| matcher.match?(identifier[key]) }
      end

      def evaluate_rule(rule, identifier)
+97 −0
Original line number Diff line number Diff line
# frozen_string_literal: true

module Labkit
  module RateLimit
    # Matcher is the internal representation of a single key/value predicate in
    # a Rule#match hash. Rule.new normalizes every match value through
    # Matcher.build; the Evaluator calls Matcher#match? per identifier value.
    #
    # Accepted input shapes (everything else raises ArgumentError):
    #   - any plain value (String, Symbol, Integer, ...) -> :eq matcher
    #   - a Regexp instance                              -> :re matcher (Ruby convenience)
    #   - { eq: <value> }                                -> :eq matcher (canonical, YAML-compatible)
    #   - { re: <String|Regexp> }                        -> :re matcher (canonical, YAML-compatible)
    #
    # Hash-key naming follows the metrics-catalog selector pattern. Glob,
    # prefix, and other matcher kinds are intentionally out of scope here; see
    # gitlab-com/gl-infra/production-engineering#28853 for that follow-up.
    #
    # An :re matcher coerces the identifier value via #to_s before applying
    # the regex, so callers can match non-String identifier values such as
    # Integer status codes (e.g. {status: { re: "^5" }} against status: 503).
    class Matcher < Data.define(:type, :value)
      KNOWN_HASH_KEYS = %i[eq re].freeze
      MAX_REGEX_SOURCE_LENGTH = 200
      ERROR_INSPECT_LIMIT = 80

      def self.build(input)
        case input
        when Regexp
          new(type: :re, value: input)
        when Hash
          from_hash(input)
        when Array
          raise ArgumentError,
            "rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
        else
          new(type: :eq, value: input)
        end
      end

      def self.from_hash(input)
        if input.size != 1
          raise ArgumentError,
            "rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
        end

        type, source = input.first
        type_sym = type.to_sym

        unless KNOWN_HASH_KEYS.include?(type_sym)
          raise ArgumentError,
            "rate-limit match value has unknown type key #{truncate_for_error(type)}; accepted: #{KNOWN_HASH_KEYS.inspect}"
        end

        compile(type_sym, source)
      end
      private_class_method :from_hash

      def self.compile(type_sym, source)
        case type_sym
        when :eq
          new(type: :eq, value: source)
        when :re
          if source.to_s.length > MAX_REGEX_SOURCE_LENGTH
            raise ArgumentError,
              "rate-limit match value {re: ...} source exceeds #{MAX_REGEX_SOURCE_LENGTH} characters"
          end

          begin
            new(type: :re, value: Regexp.new(source))
          rescue RegexpError, TypeError => e
            raise ArgumentError,
              "rate-limit match value {re: #{truncate_for_error(source)}} failed to compile: #{e.message}"
          end
        end
      end
      private_class_method :compile

      def self.truncate_for_error(value)
        s = value.inspect
        s.length > ERROR_INSPECT_LIMIT ? "#{s[0, ERROR_INSPECT_LIMIT]}...(truncated)" : s
      end
      private_class_method :truncate_for_error

      def match?(identifier_value)
        case type
        when :eq
          value == identifier_value
        when :re
          value.match?(identifier_value.to_s)
        else
          raise ArgumentError, "unknown matcher type: #{type.inspect}"
        end
      end
    end
  end
end
+1 −1
Original line number Diff line number Diff line
@@ -35,7 +35,7 @@ module Labkit

        super(
          name: name_str.freeze,
          match: match.transform_keys(&:to_sym).freeze,
          match: match.transform_keys(&:to_sym).transform_values { |v| Matcher.build(v) }.freeze,
          limit: limit,
          period: period,
          action: action_sym,
+160 −0
Original line number Diff line number Diff line
@@ -570,4 +570,164 @@ RSpec.describe Labkit::RateLimit::Evaluator do
      ev.check(identifier)
    end
  end

  describe "with an :eq matcher" do
    it "matches and increments the counter when the identifier value equals the rule value" do
      rule = make_rule(name: "api_endpoint", match: { endpoint: "GET /api/v4/projects" }, characteristics: [:user])
      id = Labkit::RateLimit::Identifier.new(user: 42, endpoint: "GET /api/v4/projects")

      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      result = evaluator(rules: [rule]).check(id)
      expect(result.matched?).to be(true)
      expect(result.rule).to eq(rule)
    end

    it "matches via { eq: ... }" do
      rule = make_rule(name: "api_endpoint",
        match: { endpoint: { eq: "GET /api/v4/projects" } },
        characteristics: [:user])
      id = Labkit::RateLimit::Identifier.new(user: 42, endpoint: "GET /api/v4/projects")

      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
    end
  end

  describe "with a :re matcher" do
    let(:id) { Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects/42") }

    it "matches via { re: \"...\" }" do
      rule = make_rule(name: "projects_api",
        match: { path: { re: "^/api/v\\d+/projects" } },
        characteristics: [:user])
      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
    end

    it "matches via a bare Regexp" do
      rule = make_rule(name: "projects_api",
        match: { path: %r{^/api/v\d+/projects} },
        characteristics: [:user])
      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
    end

    it "matches via { re: <Regexp> }" do
      rule = make_rule(name: "projects_api",
        match: { path: { re: %r{^/api/v\d+/projects} } },
        characteristics: [:user])
      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
    end

    it "AND-matches across mixed { eq: ... } and { re: ... } shapes" do
      rule = make_rule(name: "projects_api",
        match: { request_type: { eq: "api" }, path: { re: "^/api/v\\d+/projects" } },
        characteristics: [:user])
      mixed_id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/api/v4/projects/42")
      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(mixed_id).matched?).to be(true)
    end

    it "uses the identifier value (not the regex source) in the compound counter key" do
      rule = make_rule(name: "projects_api",
        match: { path: { re: "^/api/v\\d+/projects" } },
        characteristics: [:user, :path])
      counter_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")

      expect(pipe).to receive(:incr)
        .with("labkit:rl:rack_request:projects_api:user:42:path:/api/v4/projects")
        .and_return(nil)
      expect(raw_redis).to receive(:expire)

      evaluator(rules: [rule]).check(counter_id)
    end

    context "when the identifier value does not match" do
      it "does not call Redis and the rule does not match" do
        rule = make_rule(match: { path: { re: "^/admin" } })
        no_match_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")
        expect(raw_redis).not_to receive(:pipelined)

        expect(evaluator(rules: [rule]).check(no_match_id).matched?).to be(false)
      end
    end

    context "when the identifier value is not a String" do
      it "casts the identifier value via to_s and matches when the cast value matches the regex" do
        rule = make_rule(name: "status_5xx",
          match: { status: { re: "^5\\d\\d$" } },
          characteristics: [:user])
        id_503 = Labkit::RateLimit::Identifier.new(user: 42, status: 503)
        expect(pipe).to receive(:incr).and_return(nil)
        expect(raw_redis).to receive(:expire)

        expect(evaluator(rules: [rule]).check(id_503).matched?).to be(true)
      end

      it "does not match (and does not raise) when the cast value fails the regex" do
        rule = make_rule(match: { user: { re: "^9\\d+$" } })
        int_id = Labkit::RateLimit::Identifier.new(user: 42)
        expect(raw_redis).not_to receive(:pipelined)

        expect { evaluator(rules: [rule]).check(int_id) }.not_to raise_error
        expect(evaluator(rules: [rule]).check(int_id).matched?).to be(false)
      end

      it "falls through to a subsequent rule when the cast value does not match the first" do
        regex_rule = make_rule(name: "regex_first", match: { user: { re: "^9\\d+$" } }, action: :block)
        eq_rule    = make_rule(name: "eq_second",   match: { user: 42 }, action: :log)
        int_id = Labkit::RateLimit::Identifier.new(user: 42)

        expect(pipe).to receive(:incr).and_return(nil)
        expect(raw_redis).to receive(:expire)

        result = evaluator(rules: [regex_rule, eq_rule]).check(int_id)
        expect(result.matched?).to be(true)
        expect(result.rule).to eq(eq_rule)
      end
    end
  end

  describe "when the rule has multiple match keys" do
    let(:rule) do
      make_rule(name: "mixed",
        match: { request_type: "api", path: { re: "^/api/v\\d+/projects" } },
        characteristics: [:user])
    end

    it "matches when every predicate passes" do
      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/api/v4/projects")
      expect(pipe).to receive(:incr).and_return(nil)
      expect(raw_redis).to receive(:expire)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
    end

    it "does not match when one predicate fails (equality passes, regex fails)" do
      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/admin")
      expect(raw_redis).not_to receive(:pipelined)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)
    end

    it "does not match when one predicate fails (regex passes, equality fails)" do
      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "web", path: "/api/v4/projects")
      expect(raw_redis).not_to receive(:pipelined)

      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)
    end
  end
end
Loading