Commit c22497af authored by Sam Wiskow's avatar Sam Wiskow
Browse files

fix: action reprazent human review feedback on MR !272 

BLOCKER: Add Labkit.dev_or_test? using RAILS_ENV (consistent with the
rest of labkit). Remove private dev_or_test? from Limiter.

BLOCKER: Remove all logging from Evaluator. Per-request log messages
double log volume for the RackAttack limiter. Metrics deferred to #28798
(Prometheus) and logging integration deferred to #28799 (per-request
log fields after #28785). Evaluator no longer takes a logger: param.

Style: Consolidate validate_name! - nil/empty check moved from
initialize into validate_name! so all validation lives in one place.

Style: Remove # Scenario X: section comments from specs.

Style: Remove Scenario A test (Evaluator reuse is a caller concern).

Minor: Sanitize : in Rule name with tr(":", "_") since : is the Redis
key separator.

Co-Authored-By: default avatarClaude Sonnet 4.6 <noreply@anthropic.com>
parent 2da3a1e5

lib/gitlab-labkit.rb

+6 −0
Original line number Diff line number Diff line
@@ -5,6 +5,12 @@ 
# infrastructural concerns, partcularly related to

# observability.

module Labkit

  class << self

    def dev_or_test?

      %w[development test].include?(ENV.fetch("RAILS_ENV", nil))

    end

  end



  autoload :System, "labkit/system"



  autoload :Context, "labkit/context"

lib/labkit/rate_limit/evaluator.rb

+4 −46
Original line number Diff line number Diff line 
# frozen_string_literal: true



require "openssl"

require "labkit/logging/json_logger"



module Labkit

  module RateLimit

    # Evaluator holds the static parts of a rate limit check (name, rules, Redis,

    # logger) and exposes a per-request #check(identifier) method.

    # Evaluator holds the static parts of a rate limit check (name, rules, Redis)

    # and exposes a per-request #check(identifier) method.

    # @api private

    class Evaluator

      REDIS_KEY_PREFIX = "labkit:rl"

      CHAR_VALUE_MAX_LENGTH = 200

      MISSING_VALUE_SENTINEL = "_unknown_"



      def initialize(name:, rules:, redis:, logger:)

      def initialize(name:, rules:, redis:)

        @name = name

        @rules = rules

        @redis = redis

        @logger = logger

      end



      def check(identifier)

        check_rules(identifier)

      rescue StandardError => e

      rescue StandardError

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

      end
@@ -38,7 +35,6 @@ module Labkit 
          return evaluate_rule(rule, identifier)

        end



        log_no_match(identifier)

        Result.new(matched: false)

      end
@@ -54,8 +50,6 @@ module Labkit 
        count = incr_with_ttl(redis_key, resolved_period)

        exceeded = count > resolved_limit



        log_match(rule, identifier, count, redis_key, resolved_limit, resolved_period, exceeded)



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

      end
@@ -93,42 +87,6 @@ module Labkit 
        @redis.expire(redis_key, period) if count == 1

        count

      end



      def log_match(rule, identifier, count, redis_key, limit, period, exceeded)

        @logger.info(

          message: "rate_limit_check",

          name: @name,

          matched: true,

          rule_name: rule.name,

          characteristics: rule.characteristics,

          counter_key: redis_key,

          current_count: count,

          limit: limit,

          period: period,

          action: rule.action.to_s,

          exceeded: exceeded,

          identifier: identifier.to_h,

          remaining: [limit - count, 0].max

        )

      end



      def log_no_match(identifier)

        @logger.info(

          message: "rate_limit_check",

          name: @name,

          matched: false,

          identifier: identifier.to_h

        )

      end



      def log_error(error, identifier)

        @logger.warn(

          message: "rate_limit_error",

          name: @name,

          error: error.class.to_s,

          identifier: identifier&.to_h

        )

      end

    end

  end

end

lib/labkit/rate_limit/limiter.rb

+3 −10
Original line number Diff line number Diff line
@@ -20,16 +20,13 @@ module Labkit 
      NAME_PATTERN = /\A[a-z0-9_]+\z/



      def initialize(name:, rules:, redis: nil, logger: nil)

        raise ArgumentError, "name must be a non-empty String" unless name.is_a?(String) && !name.empty?



        resolved_logger = logger || RateLimit.config.logger || Labkit::Logging::JsonLogger.new($stdout)

        validated_name = validate_name!(name, resolved_logger)



        @evaluator = Evaluator.new(

          name: validated_name,

          rules: rules,

          redis: redis || RateLimit.config.redis,

          logger: resolved_logger

          redis: redis || RateLimit.config.redis

        )

      end
@@ -43,19 +40,15 @@ module Labkit 
      private



      def validate_name!(name, logger)

        raise ArgumentError, "name must be a non-empty String" unless name.is_a?(String) && !name.empty?

        return name if NAME_PATTERN.match?(name)



        raise ArgumentError, "Invalid name: #{name.inspect}. Must match /\\A[a-z0-9_]+\\z/" if dev_or_test?

        raise ArgumentError, "Invalid name: #{name.inspect}. Must match /\\A[a-z0-9_]+\\z/" if Labkit.dev_or_test?



        sanitized = name.gsub(/[^a-z0-9_]/, "_")

        logger.warn(message: "rate_limit_invalid_name", name: name, sanitized: sanitized)

        sanitized

      end



      def dev_or_test?

        env = ENV.fetch("LABKIT_ENV", nil)

        env == "test" || env == "development"

      end

    end

  end

end

lib/labkit/rate_limit/rule.rb

+1 −1
Original line number Diff line number Diff line
@@ -13,7 +13,7 @@ module Labkit 
    Rule = Data.define(:name, :match, :limit, :period, :action, :characteristics) do

      def initialize(name:, limit:, period:, characteristics:, match: {}, action: :block)

        super(

          name: name.to_s,

          name: name.to_s.tr(":", "_"),

          match: match.transform_keys(&:to_sym).freeze,

          limit: limit,

          period: period,

spec/labkit/rate_limit/evaluator_spec.rb

+3 −49
Original line number Diff line number Diff line
@@ -7,7 +7,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
  include StubENV



  let(:redis) { instance_double(Redis) }

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

  let(:identifier) { Labkit::RateLimit::Identifier.new(user: 42, ip: "1.2.3.4") }



  def make_rule(name: "default", match: {}, limit: 100, period: 60, action: :block, characteristics: [:user])
@@ -18,16 +17,13 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
  end



  def evaluator(name: "rack_request", rules: [])

    described_class.new(name: name, rules: rules, redis: redis, logger: logger)

    described_class.new(name: name, rules: rules, redis: redis)

  end



  before do

    stub_env("LABKIT_ENV", "test")

    allow(logger).to receive(:info)

    allow(logger).to receive(:warn)

    stub_env("RAILS_ENV", "test")

  end



  # Scenario Q: Rule name appears in Redis key (not positional index)

  describe "Scenario Q: Redis key format uses rule name" do

    it "builds a compound key with the rule name" do

      rule = make_rule(name: "unauthenticated_api", characteristics: [:ip])
@@ -41,7 +37,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
      evaluator(rules: [rule]).check(id)

    end



    # Scenario B: compound multi-characteristic key

    it "joins multiple characteristics into a single compound key (Scenario B)" do

      rule = make_rule(name: "auth_api", characteristics: [:user, :ip])

      id = Labkit::RateLimit::Identifier.new(user: 42, ip: "1.2.3.4")
@@ -75,7 +70,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  # Scenario R: TTL is set to rule period on first write only

  describe "Scenario R: counter TTL matches rule period" do

    it "sets expire on first write (count == 1)" do

      rule = make_rule(name: "ttl_rule", period: 120)
@@ -94,7 +88,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  # Scenario C: _unknown_ sentinel for missing characteristic

  describe "Scenario C: _unknown_ sentinel for missing characteristic" do

    it "uses _unknown_ sentinel when characteristic value is nil" do

      id = Labkit::RateLimit::Identifier.new(ip: "1.2.3.4")
@@ -133,7 +126,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  # Scenario O: arbitrary characteristic accepted (no KNOWN_CHARACTERISTICS gate)

  describe "Scenario O: arbitrary characteristics accepted" do

    it "does not raise for an unknown characteristic in test env" do

      rule = make_rule(name: "custom", characteristics: [:custom_field])
@@ -148,11 +140,9 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  # Scenario S: Redis unavailable - fail open with error Result

  describe "Scenario S: Redis unavailable" do

    it "returns error Result and logs when Redis is unavailable" do

    it "returns error Result when Redis is unavailable" do

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

      expect(logger).to receive(:warn).with(hash_including(message: "rate_limit_error"))



      rule = make_rule(name: "err_rule")

      result = evaluator(rules: [rule]).check(identifier)
@@ -163,42 +153,6 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  # Scenario T: structured logging fields

  describe "Scenario T: structured logging" do

    it "logs required fields for a matched rule" do

      rule = make_rule(name: "logged_rule", limit: 100, period: 60, characteristics: [:user])

      allow(redis).to receive(:incr).and_return(3)

      allow(redis).to receive(:expire)



      expect(logger).to receive(:info) do |msg|

        expect(msg[:message]).to eq("rate_limit_check")

        expect(msg[:name]).to eq("rack_request")

        expect(msg[:matched]).to be(true)

        expect(msg[:rule_name]).to eq("logged_rule")

        expect(msg[:characteristics]).to eq([:user])

        expect(msg[:counter_key]).to include("logged_rule")

        expect(msg[:current_count]).to eq(3)

        expect(msg[:limit]).to eq(100)

        expect(msg[:period]).to eq(60)

        expect(msg[:action]).to eq("block")

        expect(msg).to have_key(:exceeded)

        expect(msg[:identifier]).to be_a(Hash)

        expect(msg).to have_key(:remaining)

      end



      evaluator(rules: [rule]).check(identifier)

    end



    it "logs matched: false when no rule matches" do

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



      expect(logger).to receive(:info).with(hash_including(matched: false, name: "rack_request"))

      expect(redis).not_to receive(:incr)



      evaluator(rules: [rule]).check(identifier)

    end

  end



  describe "non-matching rules" do

    it "does not write to Redis and returns no-match Result" do

      rule = make_rule(match: { user: 999 })