Verified Commit 14940bbe authored by Elliot Forbes's avatar Elliot Forbes 2️⃣ Committed by GitLab
Browse files

Merge branch 'testing/docker-compose-redis-autostart' into 'master' 

test(rate_limit): replace FakeRedis with real Redis via docker compose

See merge request !289

Merged-by: Elliot Forbes's avatarElliot Forbes <eforbes@gitlab.com>
Approved-by: Elliot Forbes's avatarElliot Forbes <eforbes@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
parents 8ee8406f 714224cb

.gitignore

+1 −0
Original line number Diff line number Diff line
@@ -3,3 +3,4 @@ Gemfile.lock 
node_modules

.bundle

/.env.sh

.idea/

.gitlab-ci.yml

+10 −0
Original line number Diff line number Diff line
@@ -27,6 +27,16 @@ include: 


  - component: $CI_SERVER_FQDN/gitlab-com/gl-infra/common-ci-tasks/danger@v3.24



# Attach a redis service to the rspec job from common-ci-tasks/ruby-build.

# GitLab merges keys when a local job has the same name as an included one,

# so this augments the base job (and is inherited by anything that extends it).

rspec:

  services:

    - name: redis:7-alpine

      alias: redis

  variables:

    LABKIT_TEST_REDIS_URL: redis://redis



ruby-versions:

  extends: rspec

  image: ${CI_REGISTRY}/gitlab-com/gl-infra/common-ci-tasks-images/ruby:${RUBY_VERSION}

README.md

+10 −0
Original line number Diff line number Diff line
@@ -43,6 +43,16 @@ $ # Run tests, linters 
$ bundle exec rake verify

```



Some specs require a real Redis instance. When you run the suite locally,

it will automatically start one via `docker compose up -d redis` (see

`docker-compose.yml`) and tear it down again when the test process exits.

Redis is exposed on `localhost:6390` so it does not collide with a local

GDK/Caproni Redis on the default port.



To opt out of autostart (e.g. you've started Redis some other way), set

`LABKIT_TEST_REDIS_URL` to a reachable instance, or

`LABKIT_TEST_REDIS_NO_AUTOSTART=1` to fail loudly instead of spawning.



Please also review the [development section of the LabKit (go) README](https://gitlab.com/gitlab-org/labkit#developing-labkit) for details of the LabKit architectural philosophy.



To work on some of the scripts we use for releasing a new version,

docker-compose.yml

0 → 100644
+10 −0
Original line number Diff line number Diff line 
services:

  redis:

    image: redis:7-alpine

    ports:

      - "6390:6379"

    healthcheck:

      test: ["CMD", "redis-cli", "ping"]

      interval: 5s

      timeout: 2s

      retries: 5

spec/labkit/rate_limit_spec.rb

+42 −83
Original line number Diff line number Diff line
@@ -2,64 +2,20 @@ 


require "spec_helper"

require "redis"



# A minimal in-memory Redis fake for integration tests.

# Defined at top level to avoid RSpec/LeakyConstantDeclaration.

class FakeRedis

  def initialize

    @store = Hash.new(0)

    @ttls = {}

  end



  def incr(key)

    @store[key] += 1

  end



  def expire(key, ttl)

    @ttls[key] = ttl

    true

  end



  def get(key)

    @store[key]

  end



  def ttl(key)

    @ttls.fetch(key, -1)

  end



  def pipelined

    results = []

    fake = self

    pipe = Object.new

    pipe.define_singleton_method(:incr) do |key|

      r = fake.incr(key)

      results << r

      r

    end

    pipe.define_singleton_method(:ttl) do |key|

      r = fake.ttl(key)

      results << r

      r

    end

    pipe.define_singleton_method(:get) do |key|

      v = fake.get(key)

      r = v.zero? ? nil : v.to_s

      results << r

      r

    end

    yield pipe

    results

  end

end

require_relative "../support/test_redis"



RSpec.describe Labkit::RateLimit do

  include StubENV



  let(:fake_redis) { FakeRedis.new }

  let(:redis) { PooledRedis.new(fake_redis) }

  let(:raw_redis) { TestRedis.client }

  let(:redis) { PooledRedis.new(raw_redis) }

  let(:logger) { instance_double(Logger, info: nil, warn: nil, error: nil) }



  def get_count(key)

    val = raw_redis.get(key)

    val.nil? ? 0 : Integer(val)

  end



  def rule(name: "r", match: {}, limit: 100, period: 60, action: :block, characteristics: [:user])

    Labkit::RateLimit::Rule.new(

      name: name, match: match, limit: limit, period: period,
@@ -73,6 +29,7 @@ RSpec.describe Labkit::RateLimit do 


  before do

    stub_env("RAILS_ENV", "test")

    TestRedis.reset!

  end



  describe "Scenario B: compound multi-characteristic Redis key" do
@@ -80,9 +37,9 @@ RSpec.describe Labkit::RateLimit do 
      r = rule(name: "auth_api", characteristics: [:user, :endpoint])

      result = limiter(rules: [r]).check({ user: 42, endpoint: "/api/foo" })



      expect(fake_redis.get("labkit:rl:rack_request:auth_api:user:42:endpoint:/api/foo")).to eq(1)

      expect(fake_redis.get("labkit:rl:rack_request:auth_api:user:42")).to eq(0)

      expect(fake_redis.get("labkit:rl:rack_request:auth_api:endpoint:/api/foo")).to eq(0)

      expect(get_count("labkit:rl:rack_request:auth_api:user:42:endpoint:/api/foo")).to eq(1)

      expect(get_count("labkit:rl:rack_request:auth_api:user:42")).to eq(0)

      expect(get_count("labkit:rl:rack_request:auth_api:endpoint:/api/foo")).to eq(0)

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

    end

  end
@@ -92,7 +49,7 @@ RSpec.describe Labkit::RateLimit do 
      r = rule(name: "auth_api", characteristics: [:user])

      result = limiter(rules: [r]).check({ ip: "1.2.3.4" })



      expect(fake_redis.get("labkit:rl:rack_request:auth_api:user:_unknown_")).to eq(1)

      expect(get_count("labkit:rl:rack_request:auth_api:user:_unknown_")).to eq(1)

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

    end

  end
@@ -104,8 +61,8 @@ RSpec.describe Labkit::RateLimit do 


      limiter(rules: [r_a, r_b]).check({ user: 42, ip: "1.2.3.4" })



      expect(fake_redis.get("labkit:rl:rack_request:rule_a:user:42")).to eq(1)

      expect(fake_redis.get("labkit:rl:rack_request:rule_b:ip:1.2.3.4")).to eq(0)

      expect(get_count("labkit:rl:rack_request:rule_a:user:42")).to eq(1)

      expect(get_count("labkit:rl:rack_request:rule_b:ip:1.2.3.4")).to eq(0)

    end

  end
@@ -117,8 +74,8 @@ RSpec.describe Labkit::RateLimit do 
      id = { user: 42, ip: "1.2.3.4", endpoint: "/api/v4/projects" }

      limiter(rules: [specific, generic]).check(id)



      expect(fake_redis.get("labkit:rl:rack_request:specific:user:42")).to eq(1)

      expect(fake_redis.get("labkit:rl:rack_request:generic:ip:1.2.3.4")).to eq(0)

      expect(get_count("labkit:rl:rack_request:specific:user:42")).to eq(1)

      expect(get_count("labkit:rl:rack_request:generic:ip:1.2.3.4")).to eq(0)

    end

  end
@@ -141,7 +98,7 @@ RSpec.describe Labkit::RateLimit do 
      first  = lim.check({ user: 42 })

      second = lim.check({ user: 42 })



      expect(fake_redis.get("labkit:rl:rack_request:log_rule:user:42")).to eq(2)

      expect(get_count("labkit:rl:rack_request:log_rule:user:42")).to eq(2)



      [first, second].each do |result|

        expect(result.matched?).to be(false)
@@ -163,7 +120,7 @@ RSpec.describe Labkit::RateLimit do 
      r = rule(name: "unauthenticated_api", characteristics: [:ip])

      limiter(rules: [r]).check({ ip: "1.2.3.4" })



      expect(fake_redis.get("labkit:rl:rack_request:unauthenticated_api:ip:1.2.3.4")).to eq(1)

      expect(get_count("labkit:rl:rack_request:unauthenticated_api:ip:1.2.3.4")).to eq(1)

    end

  end
@@ -172,18 +129,20 @@ RSpec.describe Labkit::RateLimit do 
      r = rule(name: "ttl_test", period: 120)

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



      expect(fake_redis.ttl("labkit:rl:rack_request:ttl_test:user:42")).to eq(120)

      expect(raw_redis.ttl("labkit:rl:rack_request:ttl_test:user:42")).to be_within(2).of(120)

    end



    it "does not reset TTL on subsequent writes" do

      r = rule(name: "ttl_test", period: 120)

      lim = limiter(rules: [r])

      lim.check({ user: 42 })

      expect(fake_redis.ttl("labkit:rl:rack_request:ttl_test:user:42")).to eq(120)

      key = "labkit:rl:rack_request:ttl_test:user:42"

      # Manually shrink the TTL so we can detect a reset on the second check.

      raw_redis.expire(key, 60)

      lim.check({ user: 42 })

      # TTL was not reset; still only set once - verified by FakeRedis only storing last value,

      # but expire must not have been called again (count == 2 on second call)

      expect(fake_redis.get("labkit:rl:rack_request:ttl_test:user:42")).to eq(2)



      expect(raw_redis.ttl(key)).to be_within(2).of(60)

      expect(get_count(key)).to eq(2)

    end

  end
@@ -202,7 +161,7 @@ RSpec.describe Labkit::RateLimit do 
      limiter(rules: [r]).check({ user: long_val })



      expected_hash = OpenSSL::Digest::SHA256.hexdigest(long_val)

      expect(fake_redis.get("labkit:rl:rack_request:sha_rule:user:#{expected_hash}")).to eq(1)

      expect(get_count("labkit:rl:rack_request:sha_rule:user:#{expected_hash}")).to eq(1)

    end

  end
@@ -210,7 +169,7 @@ RSpec.describe Labkit::RateLimit do 
    it "uses the callable's return value as the TTL" do

      r = rule(name: "dyn", period: -> { 300 })

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

      expect(fake_redis.ttl("labkit:rl:rack_request:dyn:user:42")).to eq(300)

      expect(raw_redis.ttl("labkit:rl:rack_request:dyn:user:42")).to be_within(2).of(300)

    end

  end
@@ -231,15 +190,15 @@ RSpec.describe Labkit::RateLimit do 
      limiter(rules: [r_a, r_b]).check({ user: 42, ip: "1.2.3.4" })

      limiter(rules: [r_b, r_a]).check({ user: 42, ip: "1.2.3.4" })



      expect(fake_redis.get("labkit:rl:rack_request:rule_a:user:42")).to eq(1)

      expect(fake_redis.get("labkit:rl:rack_request:rule_b:ip:1.2.3.4")).to eq(1)

      expect(get_count("labkit:rl:rack_request:rule_a:user:42")).to eq(1)

      expect(get_count("labkit:rl:rack_request:rule_b:ip:1.2.3.4")).to eq(1)

    end



    it "never writes integer-index key segments" do

      r = rule(name: "named_rule", characteristics: [:user])

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



      expect(fake_redis.get("labkit:rl:rack_request:0:user:42")).to eq(0)

      expect(get_count("labkit:rl:rack_request:0:user:42")).to eq(0)

    end

  end
@@ -316,7 +275,7 @@ RSpec.describe Labkit::RateLimit do 
            allow(broken).to receive(:pipelined).and_raise(RuntimeError, "connection refused")

            PooledRedis.new(broken)

          else

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

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

            redis

          end
@@ -360,8 +319,8 @@ RSpec.describe Labkit::RateLimit do 


      results = Array.new(4) { lim.check({ user: 42 }) }



      expect(fake_redis.get("labkit:rl:rack_request:shadow_log:user:42")).to eq(4)

      expect(fake_redis.get("labkit:rl:rack_request:live_block:user:42")).to eq(4)

      expect(get_count("labkit:rl:rack_request:shadow_log:user:42")).to eq(4)

      expect(get_count("labkit:rl:rack_request:live_block:user:42")).to eq(4)



      expect(results[0..2].map(&:action)).to all(eq(:allow))

      expect(results[3].action).to eq(:block)
@@ -378,14 +337,14 @@ RSpec.describe Labkit::RateLimit do 
      bypass_results = Array.new(5) { lim.check({ user: 1, bypass: true }) }

      expect(bypass_results.map(&:action)).to all(eq(:allow))

      expect(bypass_results.map(&:rule)).to all(eq(bypass_r))

      expect(fake_redis.get("labkit:rl:rack_request:bypass:user:1")).to eq(5)

      expect(fake_redis.get("labkit:rl:rack_request:limited:user:1")).to eq(0)

      expect(get_count("labkit:rl:rack_request:bypass:user:1")).to eq(5)

      expect(get_count("labkit:rl:rack_request:limited:user:1")).to eq(0)



      first  = lim.check({ user: 2 })

      second = lim.check({ user: 2 })

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

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

      expect(fake_redis.get("labkit:rl:rack_request:limited:user:2")).to eq(2)

      expect(get_count("labkit:rl:rack_request:limited:user:2")).to eq(2)

    end

  end
@@ -395,15 +354,15 @@ RSpec.describe Labkit::RateLimit do 
      block_r = rule(name: "live_block_p", action: :block, limit: 5, characteristics: [:user])

      lim     = limiter(rules: [log_r, block_r])



      fake_redis.incr("labkit:rl:rack_request:live_block_p:user:42")

      fake_redis.incr("labkit:rl:rack_request:live_block_p:user:42")

      raw_redis.incr("labkit:rl:rack_request:live_block_p:user:42")

      raw_redis.incr("labkit:rl:rack_request:live_block_p:user:42")



      result = lim.peek({ user: 42 })



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

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

      expect(result.info.count).to eq(2)

      expect(fake_redis.get("labkit:rl:rack_request:shadow_log_p:user:42")).to eq(0)

      expect(get_count("labkit:rl:rack_request:shadow_log_p:user:42")).to eq(0)

    end

  end
@@ -420,8 +379,8 @@ RSpec.describe Labkit::RateLimit do 
      expect(result.rule).to eq(bypass_r)

      expect(result.info.count).to eq(0)

      expect(result.info.resolved_limit).to eq(5)

      expect(fake_redis.get("labkit:rl:rack_request:bypass_pl:user:7")).to eq(0)

      expect(fake_redis.get("labkit:rl:rack_request:limited_pl:user:7")).to eq(0)

      expect(get_count("labkit:rl:rack_request:bypass_pl:user:7")).to eq(0)

      expect(get_count("labkit:rl:rack_request:limited_pl:user:7")).to eq(0)

    end

  end

end