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

Merge branch 'bvl/fix-ratelimit-result' into 'master' 

fix: make `Result#action` return the outcome

See merge request !275

Merged-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Approved-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Co-authored-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
parents 96e48148 a9f10824

lib/labkit/rate_limit/evaluator.rb

+4 −3
Original line number Diff line number Diff line
@@ -25,7 +25,7 @@ module Labkit 
        # Intentionally broad: fail-open applies to any unexpected error (network,

        # timeout, OOM) not only Redis protocol errors.

        log_error(e, identifier)

        Result.new(matched: false, error: true)

        Result.new(matched: false, error: true, action: :allow)

      end



      private
@@ -37,7 +37,7 @@ module Labkit 
          return evaluate_rule(rule, identifier)

        end



        Result.new(matched: false)

        Result.new(matched: false, action: :allow)

      end



      def rule_matches?(rule, identifier)
@@ -51,8 +51,9 @@ module Labkit 


        count = incr_with_ttl(redis_key, resolved_period)

        exceeded = count > resolved_limit

        action = exceeded ? rule.action : :allow



        Result.new(matched: true, exceeded: exceeded, action: rule.action, rule: rule)

        Result.new(matched: true, exceeded: exceeded, action: action, rule: rule)

      end



      def build_redis_key(rule, identifier)

lib/labkit/rate_limit/result.rb

+7 −2
Original line number Diff line number Diff line
@@ -5,11 +5,16 @@ module Labkit 
    # Result is the return value of Limiter#check.

    # matched?  - true if a rule's match conditions were satisfied

    # exceeded? - true if the matched rule's counter exceeded its limit

    # action    - :block or :log (nil when matched? is false)

    # 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, or

    #                      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)

    Result = Data.define(:matched, :exceeded, :action, :rule, :error) do

      def initialize(matched:, exceeded: false, action: nil, rule: nil, error: false)

      def initialize(matched:, action:, exceeded: false, rule: nil, error: false)

        super

      end

spec/labkit/rate_limit/result_spec.rb

+6 −5
Original line number Diff line number Diff line
@@ -4,11 +4,11 @@ require "spec_helper" 


RSpec.describe Labkit::RateLimit::Result do

  describe "no-match result" do

    subject(:result) { described_class.new(matched: false) }

    subject(:result) { described_class.new(matched: false, action: :allow) }



    it { expect(result.matched?).to be(false) }

    it { expect(result.exceeded?).to be(false) }

    it { expect(result.action).to be_nil }

    it { expect(result.action).to eq(:allow) }

    it { expect(result.rule).to be_nil }

    it { expect(result.error?).to be(false) }

  end
@@ -16,11 +16,11 @@ RSpec.describe Labkit::RateLimit::Result do 
  describe "matched, not exceeded" do

    let(:rule) { instance_double(Labkit::RateLimit::Rule) }



    subject(:result) { described_class.new(matched: true, exceeded: false, action: :block, rule: rule) }

    subject(:result) { described_class.new(matched: true, exceeded: false, action: :allow, rule: rule) }



    it { expect(result.matched?).to be(true) }

    it { expect(result.exceeded?).to be(false) }

    it { expect(result.action).to eq(:block) }

    it { expect(result.action).to eq(:allow) }

    it { expect(result.rule).to be(rule) }

    it { expect(result.error?).to be(false) }

  end
@@ -45,10 +45,11 @@ RSpec.describe Labkit::RateLimit::Result do 
  end



  describe "error result (Redis unavailable)" do

    subject(:result) { described_class.new(matched: false, error: true) }

    subject(:result) { described_class.new(matched: false, error: true, action: :allow) }



    it { expect(result.error?).to be(true) }

    it { expect(result.exceeded?).to be(false) }

    it { expect(result.matched?).to be(false) }

    it { expect(result.action).to eq(:allow) }

  end

end

spec/labkit/rate_limit_spec.rb

+80 −61
Original line number Diff line number Diff line
@@ -97,53 +97,17 @@ RSpec.describe Labkit::RateLimit do 
    end

  end



  describe "Scenario G: no rules match - result indicates no match" do

    it "returns a no-match Result with no Redis writes" do

      r = rule(name: "no_match", match: { user: 999 })

      result = limiter(rules: [r]).check({ user: 42 })



      expect(result.matched?).to be(false)

      expect(result.exceeded?).to be(false)

      expect(result.action).to be_nil

      expect(result.rule).to be_nil

    end

  end



  describe "Scenario H: empty rules array" do

    it "returns a no-match Result without writing to Redis and without a warning" do

      result = limiter(rules: []).check({ user: 42 })



      expect(result.matched?).to be(false)

      expect(result.exceeded?).to be(false)

      expect(result.action).to be(:allow)

      expect(logger).not_to have_received(:warn)

    end

  end



  describe "Scenario I: result object - within limit" do

    it "returns matched? true, exceeded? false, correct action and rule" do

      r = rule(name: "within", action: :block, limit: 10)

      result = limiter(rules: [r]).check({ user: 42 })



      expect(result.matched?).to be(true)

      expect(result.exceeded?).to be(false)

      expect(result.action).to eq(:block)

      expect(result.rule).to eq(r)

      expect(result.error?).to be(false)

    end

  end



  describe "Scenario J: result object - :block rule exceeded" do

    it "returns exceeded? true with action :block" do

      10.times { redis.incr("labkit:rl:rack_request:block_rule:user:42") }

      r = rule(name: "block_rule", action: :block, limit: 10)

      result = limiter(rules: [r]).check({ user: 42 })



      expect(result.matched?).to be(true)

      expect(result.exceeded?).to be(true)

      expect(result.action).to eq(:block)

    end

  end



  describe "Scenario K: result object - :log rule exceeded" do

    it "returns exceeded? true with action :log; caller decides what to do" do

      r = rule(name: "log_rule", action: :log, limit: 1)
@@ -153,7 +117,7 @@ RSpec.describe Labkit::RateLimit do 
      second = lim.check({ user: 42 })



      expect(first.exceeded?).to be(false)

      expect(first.action).to eq(:log)

      expect(first.action).to eq(:allow)

      expect(second.exceeded?).to be(true)

      expect(second.action).to eq(:log)

    end
@@ -196,20 +160,6 @@ RSpec.describe Labkit::RateLimit do 
    end

  end



  describe "Scenario S: Redis unavailable - fail open" do

    it "returns error Result and does not exceed" do

      broken = instance_double(Redis)

      allow(broken).to receive(:incr).and_raise(RuntimeError, "Cannot connect")



      r = rule(name: "any")

      result = Labkit::RateLimit::Limiter.new(name: "rack_request", rules: [r], redis: broken, logger: logger)

        .check({ user: 42 })



      expect(result.error?).to be(true)

      expect(result.exceeded?).to be(false)

    end

  end



  describe "Identifier round-trip" do

    it "round-trips through serialize/deserialize" do

      original = Labkit::RateLimit::Identifier.new(user: 42, ip: "1.2.3.4", endpoint: "/api/v4/projects")
@@ -229,15 +179,6 @@ RSpec.describe Labkit::RateLimit do 
    end

  end



  describe "limit 0 blocks on first call" do

    it "returns exceeded? true when limit is 0" do

      r = rule(name: "zero_limit", limit: 0, action: :block)

      result = limiter(rules: [r]).check({ user: 42 })

      expect(result.exceeded?).to be(true)

      expect(result.action).to eq(:block)

    end

  end



  describe "callable period" do

    it "uses the callable's return value as the TTL" do

      r = rule(name: "dyn", period: -> { 300 })
@@ -274,4 +215,82 @@ RSpec.describe Labkit::RateLimit do 
      expect(redis.get("labkit:rl:rack_request:0:user:42")).to eq(0)

    end

  end



  describe "Result action outcome matrix" do

    {

      "block rule, within limit" => {

        rule_args: { action: :block, limit: 10 },

        pre_increment: 0,

        expected: { matched: true, action: :allow, exceeded: false, error: false }

      },

      "block rule, at exact limit" => {

        rule_args: { action: :block, limit: 1 },

        pre_increment: 0,

        expected: { matched: true, action: :allow, exceeded: false, error: false }

      },

      "block rule, exceeded" => {

        rule_args: { action: :block, limit: 1 },

        pre_increment: 1,

        expected: { matched: true, action: :block, exceeded: true, error: false }

      },

      "log rule, within limit" => {

        rule_args: { action: :log, limit: 10 },

        pre_increment: 0,

        expected: { matched: true, action: :allow, exceeded: false, error: false }

      },

      "log rule, exceeded" => {

        rule_args: { action: :log, limit: 1 },

        pre_increment: 1,

        expected: { matched: true, action: :log, exceeded: true, error: false }

      },

      "block rule, limit 0 (exceeded on first call)" => {

        rule_args: { action: :block, limit: 0 },

        pre_increment: 0,

        expected: { matched: true, action: :block, exceeded: true, error: false }

      },

      "no rule matches" => {

        rule_args: { action: :block, limit: 10, match: { user: 999 } },

        pre_increment: 0,

        expected: { matched: false, action: :allow, exceeded: false, error: false }

      },

      "empty rules array" => {

        rules: [],

        pre_increment: 0,

        expected: { matched: false, action: :allow, exceeded: false, error: false }

      },

      "redis error" => {

        rule_args: { action: :block, limit: 10 },

        pre_increment: 0,

        broken_redis: true,

        expected: { matched: false, action: :allow, exceeded: false, error: true }

      }

    }.each do |scenario, config|

      it scenario do

        test_rules =

          if config.key?(:rules)

            config[:rules]

          else

            [rule(name: "test_rule", **config[:rule_args])]

          end



        test_redis =

          if config[:broken_redis]

            broken = instance_double(Redis)

            allow(broken).to receive(:incr).and_raise(RuntimeError, "connection refused")

            broken

          else

            config[:pre_increment].times { redis.incr("labkit:rl:rack_request:test_rule:user:42") }

            redis

          end



        result = Labkit::RateLimit::Limiter.new(

          name: "rack_request", rules: test_rules, redis: test_redis, logger: logger

        ).check({ user: 42 })



        expected_rule = config[:expected][:matched] ? test_rules.first : nil

        expected = Labkit::RateLimit::Result.new(**config[:expected], rule: expected_rule)

        expect(result).to eq(expected)

      end

    end

  end

end