Verified Commit fa04d0a3 authored by Max Woolf's avatar Max Woolf Committed by GitLab
Browse files

Merge branch 'remove-allow-rule-action' into 'master'

feat!: remove the :allow rule action

See merge request !332

Merged-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Approved-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
parents ee23909d 24eeac82
Loading
Loading
Loading
Loading
Loading
+3 −8
Original line number Diff line number Diff line
@@ -146,7 +146,7 @@ A `Rule` is a `Data.define` value object with the following fields:
| `match`           | Hash of identifier key/value predicates that must **all** be satisfied for the rule to apply. Empty hash matches anything. See [Matchers](#matchers).                |
| `limit`           | Integer request threshold per `period`. May be a callable resolved on every check.                                                                                   |
| `period`          | Window length in seconds. May be a callable resolved on every check.                                                                                                 |
| `action`          | What the result reports when the limit is exceeded. One of `:block`, `:log`, `:allow`, `:skip`. Default `:block`. See [Actions](#actions).                           |
| `action`          | What the result reports when the limit is exceeded. One of `:block`, `:log`, `:skip`. Default `:block`. See [Actions](#actions).                                     |
| `characteristics` | Array of identifier keys whose values are folded into the Redis counter key. Each unique combination gets its own counter.                                           |

Making `limit` or `period` callable is the supported pattern for
@@ -198,7 +198,7 @@ flowchart TD
    Build --> Emit[Emit calls_total + limit/period gauges]
    Emit --> Act{rule.action}
    Act -->|":log<br/>(non-terminating)"| Iter
    Act -->|:block or :allow| Return([Return Result])
    Act -->|:block| Return([Return Result])
    Iter -->|no more rules| Unmatched[Emit calls_total<br/>rule=unmatched, action=allow]
    Unmatched --> ReturnUnmatched([Return matched=false<br/>action=:allow])
    Eval -. StandardError .-> Error[Emit errors_total<br/>log warn]
@@ -219,17 +219,12 @@ which never touch Redis:
  together and the `:log` rule cannot disable the `:block` rule. Note that a
  pure `:log`-only check still emits one `rule="unmatched"` metric entry
  because no terminating rule fired.
- `:allow` — when exceeded, `Result#action` is `:allow` (rather than
  `:block`). Useful for "always allow this caller even if they're over the
  limit" cases while still observing them via metrics. Evaluation terminates
  on the first match.
- `:skip` — bypass. A matching rule terminates evaluation with
  `Result#action` `:allow` **without any Redis operation**: nothing is
  counted, so `limit`, `period`, `characteristics`, and `count_distinct` are
  inert and the result carries no `info` (`to_response_headers` is `{}`).
  The match is still observable via `calls_total{action="skip"}`. Use this
  for bypasses that don't need a counter; use `:allow` only when you want
  the bypassed traffic counted.
  for bypasses.

### Redis keys

+3 −3
Original line number Diff line number Diff line
@@ -8,9 +8,9 @@ module Labkit
    # action    - the outcome: what the caller should do
    #             :block = rule matched, exceeded, rule configured to block
    #             :log   = rule matched, exceeded, rule configured to log only
    #             :allow = rule matched but count within limit, rule configured to allow,
    #                      rule configured to skip (bypass, nothing counted),
    #                      no rule matched, or error (fail-open)
    #             :allow = rule matched but count within limit, rule configured
    #                      to skip (bypass, nothing counted), no rule matched,
    #                      or error (fail-open)
    #             The rule's configured action is available via rule.action.
    # rule      - the matched Rule object (nil when matched? is false)
    # error?    - true if Redis was unavailable; result fails open (exceeded? is false)
+2 −4
Original line number Diff line number Diff line
@@ -2,7 +2,7 @@

module Labkit
  module RateLimit
    KNOWN_ACTIONS = %i[block log allow skip].freeze
    KNOWN_ACTIONS = %i[block log skip].freeze
    RULE_NAME_PATTERN = /\A[a-z0-9_]+\z/
    RULE_NAME_MAX_LENGTH = 64

@@ -13,9 +13,7 @@ module Labkit
    # limit           - request threshold; may be a callable (resolved per check)
    # period          - window in seconds; may be a callable (resolved per check)
    # action          - :block (enforce), :log (count and log only, do not block,
    #                   evaluation continues to subsequent rules), :allow
    #                   (count but always permit; terminates evaluation on match
    #                   regardless of whether the limit was exceeded), or :skip
    #                   evaluation continues to subsequent rules), or :skip
    #                   (bypass: permit and terminate evaluation on match without
    #                   counting; performs no Redis operation, so limit, period,
    #                   characteristics, and count_distinct are inert)
+5 −61
Original line number Diff line number Diff line
@@ -795,7 +795,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do
    end
  end

  describe "Multi-rule evaluation with :log/:block/:allow/:skip", :with_metrics_config do
  describe "Multi-rule evaluation with :log/:block/:skip", :with_metrics_config do
    let(:metrics) { Labkit::RateLimit::Metrics }

    it "with a single :log rule that exceeds, increments the counter and returns the fall-through Result" do
@@ -848,36 +848,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do
      expect(raw_redis.exists?("labkit:rl:rack_request:log_rule_d:user:42")).to be(false)
    end

    it "with an :allow rule whose match: is satisfied, evaluates against Redis and returns :allow" do
      allow_r = make_rule(name: "allow_rule_e", action: :allow, match: { bypass: true }, limit: 5, period: 60)
      bypass_id = Labkit::RateLimit::Identifier.new(bypass: true)

      result = evaluator(rules: [allow_r]).check(bypass_id)

      expect(result.matched?).to be(true)
      expect(result.action).to eq(:allow)
      expect(result.rule).to eq(allow_r)
      expect(result.exceeded?).to be(false)
      expect(result.info.count).to eq(1.0)
      expect(result.info.resolved_limit).to eq(5)
      expect(result.to_response_headers).to include(
        "RateLimit-Limit" => "5",
        "RateLimit-Remaining" => "4"
      )
    end

    it "with an :allow rule preceding a :block rule, identifier matches :allow, never evaluates :block" do
      allow_r = make_rule(name: "allow_rule_f", action: :allow, match: { bypass: true }, limit: 1, period: 60)
      block_r = make_rule(name: "block_rule_f", action: :block, limit: 1, characteristics: [:user])
      bypass_id = Labkit::RateLimit::Identifier.new(bypass: true, user: 42)

      result = evaluator(rules: [allow_r, block_r]).check(bypass_id)

      expect(result.action).to eq(:allow)
      expect(result.rule).to eq(allow_r)
      expect(raw_redis.exists?("labkit:rl:rack_request:block_rule_f:user:42")).to be(false)
    end

    it "with no matching rule, increments the unmatched metric and returns the fall-through" do
      rule = make_rule(name: "unmatched_g", action: :block, limit: 1, match: { user: 999 })

@@ -899,27 +869,16 @@ RSpec.describe Labkit::RateLimit::Evaluator do
      expect(stored_count("labkit:rl:rack_request:log_rule_h_b:user:42")).to eq(1.0)
    end

    it "with an :allow rule whose match: is NOT satisfied, skips :allow and evaluates the :block" do
      allow_r = make_rule(name: "allow_rule_m", action: :allow, match: { bypass: true }, limit: 1, period: 60)
      block_r = make_rule(name: "block_rule_m", action: :block, limit: 1, characteristics: [:user])
      non_bypass_id = Labkit::RateLimit::Identifier.new(user: 1)

      result = evaluator(rules: [allow_r, block_r]).check(non_bypass_id, cost: 2)

      expect(result.action).to eq(:block)
      expect(result.rule).to eq(block_r)
    end

    it "with an :allow rule using match: {} as a universal bypass, never falls through" do
      allow_r = make_rule(name: "allow_rule_n", action: :allow, match: {}, limit: 1, period: 60)
    it "with a :skip rule using match: {} as a universal bypass, never falls through" do
      skip_r = make_rule(name: "skip_rule_n", action: :skip, match: {}, limit: 1, period: 60)
      block_r = make_rule(name: "block_rule_n", action: :block, limit: 1, characteristics: [:user])
      ev = evaluator(rules: [allow_r, block_r])
      ev = evaluator(rules: [skip_r, block_r])

      results = Array.new(10) do |i|
        ev.check(Labkit::RateLimit::Identifier.new(user: i))
      end

      expect(results).to all(have_attributes(matched?: true, action: :allow, rule: allow_r))
      expect(results).to all(have_attributes(matched?: true, action: :allow, rule: skip_r))
      expect(raw_redis.keys("labkit:rl:rack_request:block_rule_n:*")).to be_empty
    end

@@ -934,21 +893,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do
      expect(raw_redis.keys("labkit:rl:*")).to be_empty
    end

    it "peek with an :allow rule whose match: is satisfied, reads through Redis and never evaluates :block" do
      allow_r = make_rule(name: "allow_rule_l", action: :allow, match: { bypass: true }, limit: 5, period: 60)
      block_r = make_rule(name: "block_rule_l", action: :block, limit: 1, characteristics: [:user])
      bypass_id = Labkit::RateLimit::Identifier.new(bypass: true, user: 42)

      result = evaluator(rules: [allow_r, block_r]).peek(bypass_id)

      expect(result.matched?).to be(true)
      expect(result.action).to eq(:allow)
      expect(result.rule).to eq(allow_r)
      expect(result.info.count).to eq(0.0)
      expect(result.info.resolved_limit).to eq(5)
      expect(raw_redis.keys("labkit:rl:rack_request:block_rule_l:*")).to be_empty
    end

    it "with a :skip rule whose match: is satisfied, terminates without counting and never evaluates :block" do
      skip_r = make_rule(name: "skip_rule_o", action: :skip, match: { bypass: true }, limit: 5, period: 60)
      block_r = make_rule(name: "block_rule_o", action: :block, limit: 1, characteristics: [:user])
+6 −2
Original line number Diff line number Diff line
@@ -85,10 +85,9 @@ RSpec.describe Labkit::RateLimit::Rule do
    {
      :block => :block,
      :log => :log,
      :allow => :allow,
      :skip => :skip,
      "block" => :block,
      "allow" => :allow,
      "log" => :log,
      "skip" => :skip
    }.each do |input, expected|
      it "accepts #{input.inspect} and exposes it as #{expected.inspect}" do
@@ -100,6 +99,11 @@ RSpec.describe Labkit::RateLimit::Rule do
      expect { valid_rule(action: :deny) }
        .to raise_error(ArgumentError, /Invalid action/)
    end

    it "raises on :allow, which is not a rule action" do
      expect { valid_rule(action: :allow) }
        .to raise_error(ArgumentError, /Invalid action/)
    end
  end

  describe "match and characteristics normalization" do
Loading