diff --git a/lib/Config.js b/lib/Config.js index f4fdc96044..aae85250a2 100644 --- a/lib/Config.js +++ b/lib/Config.js @@ -1860,23 +1860,15 @@ class Config extends EventEmitter { this.enableVeeamRoute = config.enableVeeamRoute; } - this.rateLimiting = { - enabled: false, - bucket: { - configCacheTTL: constants.rateLimitDefaultConfigCacheTTL, - }, - }; + // Parse and validate all rate limiting configuration + this.rateLimiting = parseRateLimitConfig(config.rateLimiting); - if (config.rateLimiting?.enabled) { - // rate limiting uses the same localCache config defined for S3 to avoid - // config duplication. + // Rate limiting uses the same localCache config defined for S3 to avoid config duplication. + // If rate limiting is enabled check to make sure it is also configured. + if (this.rateLimiting.enabled) { assert(this.localCache, 'localCache must be defined when rate limiting is enabled'); - - // Parse and validate all rate limiting configuration - this.rateLimiting = parseRateLimitConfig(config.rateLimiting); } - if (config.capabilities) { if (config.capabilities.locationTypes) { this.supportedLocationTypes = new Set(config.capabilities.locationTypes); diff --git a/lib/api/apiUtils/rateLimit/cache.js b/lib/api/apiUtils/rateLimit/cache.js index a73147b580..2d9a3b2b67 100644 --- a/lib/api/apiUtils/rateLimit/cache.js +++ b/lib/api/apiUtils/rateLimit/cache.js @@ -1,7 +1,9 @@ const configCache = new Map(); +const bucketOwnerCache = new Map(); const namespace = { bucket: 'bkt', + account: 'acc', }; function cacheSet(cache, key, value, ttl) { @@ -54,13 +56,23 @@ const setCachedConfig = formatKeyDecorator(cacheSet.bind(null, configCache)); const deleteCachedConfig = formatKeyDecorator(cacheDelete.bind(null, configCache)); const expireCachedConfigs = cacheExpire.bind(null, configCache); +const getCachedBucketOwner = cacheGet.bind(null, bucketOwnerCache); +const setCachedBucketOwner = cacheSet.bind(null, bucketOwnerCache); +const deleteCachedBucketOwner = cacheDelete.bind(null, bucketOwnerCache); +const expireCachedBucketOwners = cacheExpire.bind(null, bucketOwnerCache); + module.exports = { namespace, setCachedConfig, getCachedConfig, expireCachedConfigs, deleteCachedConfig, + setCachedBucketOwner, + getCachedBucketOwner, + deleteCachedBucketOwner, + expireCachedBucketOwners, // Do not access directly // Used only for tests configCache, + bucketOwnerCache, }; diff --git a/lib/api/apiUtils/rateLimit/cleanup.js b/lib/api/apiUtils/rateLimit/cleanup.js index 40628cc97c..3a54fbcfaa 100644 --- a/lib/api/apiUtils/rateLimit/cleanup.js +++ b/lib/api/apiUtils/rateLimit/cleanup.js @@ -1,7 +1,21 @@ -const { expireCachedConfigs } = require('./cache'); +const { expireCachedConfigs, expireCachedBucketOwners } = require('./cache'); const { rateLimitCleanupInterval } = require('../../../../constants'); -let cleanupInterval = null; +let cleanupTimer = null; + +function cleanupJob(log, options = {}) { + const expiredConfigs = expireCachedConfigs(); + const expiredBucketOwners = expireCachedBucketOwners(); + if (expiredConfigs > 0 || expiredBucketOwners > 0) { + log.debug('Rate limit cleanup completed', { expiredConfigs, expiredBucketOwners }); + } + + cleanupTimer = setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval); + if (!options.skipUnref) { + cleanupTimer.unref(); + } +} + /** * Start periodic cleanup of expired rate limit counters and cached configs @@ -9,11 +23,12 @@ let cleanupInterval = null; * Runs every 10 seconds to: * - Remove expired GCRA counters (where emptyAt <= now) * - Remove expired cached rate limit configs (where TTL expired) + * - Remove expired cached bucket owners (where TTL expired) * * This prevents memory leaks from accumulating expired entries. */ function startCleanupJob(log, options = {}) { - if (cleanupInterval) { + if (cleanupTimer) { log.warn('Rate limit cleanup job already running'); return; } @@ -22,18 +37,12 @@ function startCleanupJob(log, options = {}) { interval: rateLimitCleanupInterval, }); - cleanupInterval = setInterval(() => { - const now = Date.now(); - const expiredConfigs = expireCachedConfigs(now); - if (expiredConfigs > 0) { - log.debug('Rate limit cleanup completed', { expiredConfigs }); - } - }, rateLimitCleanupInterval); + cleanupTimer = setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval); // Prevent cleanup job from keeping process alive // Skip unref() in test environment to work with sinon fake timers if (!options.skipUnref) { - cleanupInterval.unref(); + cleanupTimer.unref(); } } @@ -42,9 +51,9 @@ function startCleanupJob(log, options = {}) { * Used for graceful shutdown or testing */ function stopCleanupJob(log) { - if (cleanupInterval) { - clearInterval(cleanupInterval); - cleanupInterval = null; + if (cleanupTimer !== null) { + clearTimeout(cleanupTimer); + cleanupTimer = null; if (log) { log.info('Stopped rate limit cleanup job'); } diff --git a/lib/api/apiUtils/rateLimit/config.js b/lib/api/apiUtils/rateLimit/config.js index 1b6928b94f..84c4e49a35 100644 --- a/lib/api/apiUtils/rateLimit/config.js +++ b/lib/api/apiUtils/rateLimit/config.js @@ -2,7 +2,6 @@ const Joi = require('@hapi/joi'); const { errors, ArsenalError } = require('arsenal'); const { rateLimitDefaultConfigCacheTTL, rateLimitDefaultBurstCapacity } = require('../../../../constants'); -const { calculateInterval } = require('./gcra'); /** * Full Rate Limiting Configuration Example @@ -191,6 +190,24 @@ const rateLimitConfigSchema = Joi.object({ code: errors.SlowDown.message, message: errors.SlowDown.description, }), +}).default({ + enabled: false, + nodes: 1, + tokenBucketBufferSize: 50, + tokenBucketRefillThreshold: 20, + bucket: { + configCacheTTL: rateLimitDefaultConfigCacheTTL, + defaultBurstCapacity: rateLimitDefaultBurstCapacity, + }, + account: { + configCacheTTL: rateLimitDefaultConfigCacheTTL, + defaultBurstCapacity: rateLimitDefaultBurstCapacity, + }, + error: { + statusCode: 503, + code: errors.SlowDown.message, + message: errors.SlowDown.description, + }, }); /** @@ -203,10 +220,10 @@ const rateLimitConfigSchema = Joi.object({ * @returns {RateLimitClassConfig} Transformed rate limit config */ function transformClassConfig(resourceClass, validatedCfg, nodes) { - const transformed = { - defaultConfig: undefined, - configCacheTTL: validatedCfg.configCacheTTL, - defaultBurstCapacity: validatedCfg.defaultBurstCapacity, + const defaultConfig = { + RequestsPerSecond: { + BurstCapacity: validatedCfg.defaultBurstCapacity, + }, }; if (validatedCfg.defaultConfig?.requestsPerSecond) { @@ -223,23 +240,18 @@ function transformClassConfig(resourceClass, validatedCfg, nodes) { ); } - // Use provided burstCapacity or fall back to default - const effectiveBurstCapacity = burstCapacity || transformed.defaultBurstCapacity; - - // Calculate per-node interval using distributed architecture - const interval = calculateInterval(limit, nodes); - // Store both the original limit and the calculated values - transformed.defaultConfig = { - limit, - requestsPerSecond: { - interval, - bucketSize: effectiveBurstCapacity * 1000, - }, + defaultConfig.RequestsPerSecond = { + Limit: limit, + BurstCapacity: burstCapacity || validatedCfg.defaultBurstCapacity, }; } - return transformed; + return { + defaultConfig, + configCacheTTL: validatedCfg.configCacheTTL, + defaultBurstCapacity: validatedCfg.defaultBurstCapacity, + }; } /** diff --git a/lib/api/apiUtils/rateLimit/helpers.js b/lib/api/apiUtils/rateLimit/helpers.js index 8bc8d7b83c..07811b021c 100644 --- a/lib/api/apiUtils/rateLimit/helpers.js +++ b/lib/api/apiUtils/rateLimit/helpers.js @@ -5,102 +5,159 @@ const { policies: { actionMaps: { actionMapBucketRateLimit } } } = require('arse const rateLimitApiActions = Object.keys(actionMapBucketRateLimit); +function requestNeedsRateCheck(request) { + // Is the feature enabled? + if (!config.rateLimiting.enabled) { + return false; + } + + // Is the request an internal or rate limit admin operation? + if (rateLimitApiActions.includes(request.apiMethod) || request.isInternalServiceRequest) { + return false; + } + + // Have we already checked both bucket and account configs? + return !(request.rateLimitAccountAlreadyChecked && request.rateLimitBucketAlreadyChecked); +} + /** - * Extract rate limit configuration from bucket metadata or global rate limit configuration. + * Extract bucket rate limit configuration from bucket metadata or global rate limit configuration. * * Resolves in priority order: * 1. Per-bucket configuration (from bucket metadata) * 2. Global default configuration - * 3. No rate limiting (null) * * @param {object} bucket - Bucket metadata object - * @param {string} bucketName - Bucket name * @param {object} log - Logger instance - * @returns {object|null} Rate limit config or null if no limit + * @returns {object} Rate limit config */ -function extractBucketRateLimitConfig(bucket, bucketName, log) { +function extractBucketRateLimitConfig(bucketMD, log) { // Try per-bucket config first - const bucketConfig = bucket.getRateLimitConfiguration(); + const bucketConfig = bucketMD.getRateLimitConfiguration(); if (bucketConfig) { const data = bucketConfig.getData(); - const limitConfig = { - limit: data.RequestsPerSecond.Limit, - burstCapacity: config.rateLimiting.bucket.defaultBurstCapacity * 1000, - source: 'bucket', + + const merged = { + RequestsPerSecond: { + ...config.rateLimiting.bucket.defaultConfig.RequestsPerSecond, + ...(data.RequestsPerSecond || {}), + source: data.RequestsPerSecond !== undefined ? 'resource' : 'global', + }, }; log.debug('Extracted per-bucket rate limit config', { - bucketName, - limit: limitConfig.limit, - burstCapacity: config.rateLimiting.bucket.defaultBurstCapacity * 1000, + bucketName: bucketMD.getName(), + cfg: merged, }); - return limitConfig; + return merged; } - // Fall back to global default config - const globalLimit = config.rateLimiting.bucket?.defaultConfig?.limit; - if (globalLimit !== undefined && globalLimit > 0) { - const limitConfig = { - limit: globalLimit, - burstCapacity: config.rateLimiting.bucket.defaultBurstCapacity * 1000, + log.debug('Using global default rate limit config', { + bucketName: bucketMD.getName(), + cfg: { + ...config.rateLimiting.bucket.defaultConfig, + source: 'global', + } + }); + + return { + RequestsPerSecond: { + ...config.rateLimiting.bucket.defaultConfig.RequestsPerSecond, source: 'global', + }, + }; +} + +/** + * Extract account rate limit configuration from request or global rate limit configuration. + * + * Resolves in priority order: + * 1. Per-account configuration (from request) + * 2. Global default configuration + * + * @param {object} authInfo - Instance of AuthInfo class with requester's info + * @param {object} request - request object given by router + * @param {object} log - Logger instance + * @returns {object} Rate limit config + */ +function extractAccountRateLimitConfig(authInfo, request, log) { + // Try per-account config first + if (request.accountLimits) { + const merged = { + RequestsPerSecond: { + ...config.rateLimiting.account.defaultConfig.RequestsPerSecond, + ...(request.accountLimits.RequestsPerSecond || {}), + source: request.accountLimits.RequestsPerSecond !== undefined ? 'resource' : 'global', + }, }; - log.debug('Using global default rate limit config', { - bucketName, - limit: limitConfig.limit, + log.debug('Extracted per-account rate limit config', { + accountId: authInfo.getCanonicalID(), + cfg: merged, }); - return limitConfig; + return merged; } - // No rate limiting configured - cache null to avoid repeated lookups - log.trace('No rate limit configured for bucket', { bucketName }); - return null; -} + const cfg = { + RequestsPerSecond: { + ...config.rateLimiting.account.defaultConfig.RequestsPerSecond, + source: 'global', + }, + }; -function extractRateLimitConfigFromRequest(request) { - const applyRateLimit = config.rateLimiting?.enabled - && !rateLimitApiActions.includes(request.apiMethod) // Don't limit any rate limit admin actions - && !request.isInternalServiceRequest; // Don't limit any calls from internal services + log.debug('Using global default rate limit config', { + accountId: authInfo.getCanonicalID(), + cfg, + }); - if (!applyRateLimit) { - return { needsCheck: false }; - } + return cfg; +} - const limitConfigs = {}; - let needsCheck = false; +function extractRateLimitConfigFromRequest(request, authInfo, bucketMD, log) { + const limitConfig = { + bucket: extractBucketRateLimitConfig(bucketMD, log), + account: extractAccountRateLimitConfig(authInfo, request, log), + }; + return limitConfig; +} - if (request.accountLimits) { - limitConfigs.account = { - ...request.accountLimits, - source: 'account', - }; - needsCheck = true; +function getCachedRateLimitConfig(request) { + const cachedConfig = {}; + const cachedBucketConfig = cache.getCachedConfig(cache.namespace.bucket, request.bucketName); + if (cachedBucketConfig !== undefined) { + cachedConfig.bucket = cachedBucketConfig; } - if (request.bucketName) { - const cachedConfig = cache.getCachedConfig(cache.namespace.bucket, request.bucketName); - if (cachedConfig) { - limitConfigs.bucket = cachedConfig; - needsCheck = true; + const cachedOwner = cache.getCachedBucketOwner(request.bucketName); + if (cachedOwner !== undefined) { + const cachedAccountConfig = cache.getCachedConfig(cache.namespace.account, cachedOwner); + if (cachedAccountConfig !== undefined) { + cachedConfig.account = cachedAccountConfig; + cachedConfig.bucketOwner = cachedOwner; } + } - if (!request.accountLimits) { - const cachedOwner = cache.getCachedBucketOwner(request.bucketName); - if (cachedOwner !== undefined) { - const cachedConfig = cache.getCachedConfig(cache.namespace.account, cachedOwner); - if (cachedConfig) { - limitConfigs.account = cachedConfig; - limitConfigs.bucketOwner = cachedOwner; - needsCheck = true; - } - } - } + return cachedConfig; +} + +function buildRateChecksFromConfig(resourceClass, resourceId, limitConfig) { + const checks = []; + if (limitConfig?.RequestsPerSecond?.Limit > 0) { + checks.push({ + resourceClass, + resourceId, + measure: 'rps', + source: limitConfig.RequestsPerSecond.source, + config: { + limit: limitConfig.RequestsPerSecond.Limit, + burstCapacity: limitConfig.RequestsPerSecond.BurstCapacity * 1000, + }, + }); } - return { needsCheck, limitConfigs }; + return checks; } function checkRateLimitsForRequest(checks, log) { @@ -109,11 +166,11 @@ function checkRateLimitsForRequest(checks, log) { const bucket = getTokenBucket(check.resourceClass, check.resourceId, check.measure, check.config, log); if (!bucket.hasCapacity()) { log.debug('Rate limit check: denied (no tokens available)', { - resourceClass: bucket.resourceClass, - resourceId: bucket.resourceId, - measure: bucket.measure, - limit: bucket.limitConfig.limit, - source: bucket.limitConfig.source, + resourceClass: check.resourceClass, + resourceId: check.resourceId, + measure: check.measure, + limit: check.config.limit, + source: check.source, }); return { allowed: false, rateLimitSource: `${check.resourceClass}:${check.source}`}; @@ -122,13 +179,15 @@ function checkRateLimitsForRequest(checks, log) { buckets.push(bucket); log.trace('Rate limit check: allowed (token consumed)', { - resourceClass: bucket.resourceClass, - resourceId: bucket.resourceId, - measure: bucket.measure, - source: bucket.limitConfig.source, + resourceClass: check.resourceClass, + resourceId: check.resourceId, + measure: check.measure, + source: check.source, }); } + // buckets have already been checked for available capacity + // no need to check return values buckets.forEach(bucket => bucket.tryConsume()); return { allowed: true }; } @@ -137,5 +196,8 @@ module.exports = { rateLimitApiActions, extractBucketRateLimitConfig, extractRateLimitConfigFromRequest, + buildRateChecksFromConfig, checkRateLimitsForRequest, + getCachedRateLimitConfig, + requestNeedsRateCheck, }; diff --git a/lib/api/apiUtils/rateLimit/tokenBucket.js b/lib/api/apiUtils/rateLimit/tokenBucket.js index f20f284607..e372efd4eb 100644 --- a/lib/api/apiUtils/rateLimit/tokenBucket.js +++ b/lib/api/apiUtils/rateLimit/tokenBucket.js @@ -200,7 +200,7 @@ function getTokenBucket(resourceClass, resourceId, measure, limitConfig, log) { } else { const { updated, oldConfig } = bucket.updateLimit(limitConfig); if (updated) { - log.info('Updated token bucket limit config', { + log.debug('Updated token bucket limit config', { cacheKey, old: oldConfig, new: limitConfig, diff --git a/tests/unit/api/apiUtils/rateLimit/cleanup.js b/tests/unit/api/apiUtils/rateLimit/cleanup.js index fccf6cc048..eab3e5901b 100644 --- a/tests/unit/api/apiUtils/rateLimit/cleanup.js +++ b/tests/unit/api/apiUtils/rateLimit/cleanup.js @@ -9,8 +9,8 @@ const constants = require('../../../../../constants'); describe('Rate limit cleanup job', () => { let mockLog; - let setIntervalSpy; - let clearIntervalSpy; + let setTimeoutSpy; + let clearTimeoutSpy; beforeEach(() => { mockLog = { @@ -18,14 +18,14 @@ describe('Rate limit cleanup job', () => { warn: sinon.stub(), debug: sinon.stub(), }; - setIntervalSpy = sinon.spy(global, 'setInterval'); - clearIntervalSpy = sinon.spy(global, 'clearInterval'); + setTimeoutSpy = sinon.spy(global, 'setTimeout'); + clearTimeoutSpy = sinon.spy(global, 'clearTimeout'); }); afterEach(() => { stopCleanupJob(); - setIntervalSpy.restore(); - clearIntervalSpy.restore(); + setTimeoutSpy.restore(); + clearTimeoutSpy.restore(); }); it('should start cleanup job successfully', () => { @@ -35,22 +35,22 @@ describe('Rate limit cleanup job', () => { assert(mockLog.info.calledWith('Starting rate limit cleanup job', { interval: constants.rateLimitCleanupInterval, })); - assert(setIntervalSpy.calledOnce); - assert.strictEqual(setIntervalSpy.firstCall.args[1], constants.rateLimitCleanupInterval); + assert(setTimeoutSpy.calledOnce); + assert.strictEqual(setTimeoutSpy.firstCall.args[1], constants.rateLimitCleanupInterval); }); it('should not start cleanup job if already running', () => { startCleanupJob(mockLog, { skipUnref: true }); mockLog.info.resetHistory(); mockLog.warn.resetHistory(); - setIntervalSpy.resetHistory(); + setTimeoutSpy.resetHistory(); startCleanupJob(mockLog, { skipUnref: true }); assert(mockLog.warn.calledOnce); assert(mockLog.warn.calledWith('Rate limit cleanup job already running')); assert(mockLog.info.notCalled); - assert(setIntervalSpy.notCalled); + assert(setTimeoutSpy.notCalled); }); it('should stop cleanup job successfully', () => { @@ -61,36 +61,36 @@ describe('Rate limit cleanup job', () => { assert(mockLog.info.calledOnce); assert(mockLog.info.calledWith('Stopped rate limit cleanup job')); - assert(clearIntervalSpy.calledOnce); + assert(clearTimeoutSpy.calledOnce); }); it('should not error when stopping cleanup job that is not running', () => { assert.doesNotThrow(() => { stopCleanupJob(mockLog); }); - assert(clearIntervalSpy.notCalled); + assert(clearTimeoutSpy.notCalled); }); it('should allow restarting cleanup job after stopping', () => { startCleanupJob(mockLog, { skipUnref: true }); - assert(setIntervalSpy.calledOnce); + assert(setTimeoutSpy.calledOnce); stopCleanupJob(mockLog); - assert(clearIntervalSpy.calledOnce); + assert(clearTimeoutSpy.calledOnce); - setIntervalSpy.resetHistory(); - clearIntervalSpy.resetHistory(); + setTimeoutSpy.resetHistory(); + clearTimeoutSpy.resetHistory(); mockLog.info.resetHistory(); startCleanupJob(mockLog, { skipUnref: true }); - assert(setIntervalSpy.calledOnce); + assert(setTimeoutSpy.calledOnce); assert(mockLog.info.calledOnce); }); it('should call unref() on interval by default', () => { const mockUnref = sinon.stub(); - setIntervalSpy.restore(); - setIntervalSpy = sinon.stub(global, 'setInterval').returns({ + setTimeoutSpy.restore(); + setTimeoutSpy = sinon.stub(global, 'setTimeout').returns({ unref: mockUnref, }); @@ -99,13 +99,13 @@ describe('Rate limit cleanup job', () => { assert(mockUnref.calledOnce); stopCleanupJob(mockLog); - setIntervalSpy.restore(); + setTimeoutSpy.restore(); }); it('should not call unref() when skipUnref is true', () => { const mockUnref = sinon.stub(); - setIntervalSpy.restore(); - setIntervalSpy = sinon.stub(global, 'setInterval').returns({ + setTimeoutSpy.restore(); + setTimeoutSpy = sinon.stub(global, 'setTimeout').returns({ unref: mockUnref, }); @@ -114,6 +114,6 @@ describe('Rate limit cleanup job', () => { assert(mockUnref.notCalled); stopCleanupJob(mockLog); - setIntervalSpy.restore(); + setTimeoutSpy.restore(); }); }); diff --git a/tests/unit/api/apiUtils/rateLimit/config.js b/tests/unit/api/apiUtils/rateLimit/config.js index 118032603a..e6fbd35e74 100644 --- a/tests/unit/api/apiUtils/rateLimit/config.js +++ b/tests/unit/api/apiUtils/rateLimit/config.js @@ -36,7 +36,7 @@ describe('parseRateLimitConfig', () => { assert.strictEqual(result.error.message, 'ServiceUnavailable'); assert.strictEqual(result.error.description, 'Service Unavailable'); assert(result.bucket.defaultConfig); - assert(result.bucket.defaultConfig.requestsPerSecond); + assert(result.bucket.defaultConfig.RequestsPerSecond); }); it('should use default values when optional fields are omitted', () => { @@ -53,7 +53,9 @@ describe('parseRateLimitConfig', () => { assert.strictEqual(result.tokenBucketRefillThreshold, 20); // Default // Bucket config is always initialized for per-bucket rate limiting via API assert(result.bucket); - assert.strictEqual(result.bucket.defaultConfig, undefined); // No global default + assert.deepStrictEqual(result.bucket.defaultConfig, { + RequestsPerSecond: { BurstCapacity: constants.rateLimitDefaultBurstCapacity }, + }); assert.strictEqual(result.bucket.configCacheTTL, constants.rateLimitDefaultConfigCacheTTL); // Default assert.strictEqual(result.bucket.defaultBurstCapacity, constants.rateLimitDefaultBurstCapacity); // Default assert.strictEqual(result.error.code, errors.SlowDown.code); // Default @@ -71,7 +73,9 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); assert.strictEqual(result.bucket.configCacheTTL, 600); - assert.strictEqual(result.bucket.defaultConfig, undefined); + assert.deepStrictEqual(result.bucket.defaultConfig, { + RequestsPerSecond: { BurstCapacity: constants.rateLimitDefaultBurstCapacity }, + }); }); it('should use default configCacheTTL when not specified', () => { @@ -374,7 +378,7 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); assert(result.bucket.defaultConfig); - assert(result.bucket.defaultConfig.requestsPerSecond); + assert(result.bucket.defaultConfig.RequestsPerSecond); }); it('should throw if defaultConfig is not an object', () => { @@ -421,7 +425,7 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); // limit = 0 means unlimited, should be accepted - assert(result.bucket.defaultConfig.requestsPerSecond); + assert(result.bucket.defaultConfig.RequestsPerSecond); }); it('should propagate validation errors for negative limit', () => { @@ -476,9 +480,10 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.bucket.defaultConfig.requestsPerSecond.bucketSize; - // bucketSize = burstCapacity * 1000 - assert.strictEqual(bucketSize, constants.rateLimitDefaultBurstCapacity * 1000); + assert.strictEqual( + result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, + constants.rateLimitDefaultBurstCapacity + ); }); it('should use custom burstCapacity when provided', () => { @@ -495,8 +500,9 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.bucket.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, 20 * 1000); + assert.strictEqual( + result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 20 + ); }); it('should throw if burstCapacity is negative', () => { @@ -573,8 +579,9 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.bucket.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, 1.5 * 1000); + assert.strictEqual( + result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5 + ); }); }); @@ -877,7 +884,7 @@ describe('parseRateLimitConfig', () => { assert(result.account); assert(result.account.defaultConfig); - assert.strictEqual(result.account.defaultConfig.limit, 500); + assert.strictEqual(result.account.defaultConfig.RequestsPerSecond.Limit, 500); assert.strictEqual(result.account.configCacheTTL, 60000); assert.strictEqual(result.account.defaultBurstCapacity, 2); }); @@ -904,9 +911,9 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); assert(result.bucket.defaultConfig); - assert.strictEqual(result.bucket.defaultConfig.limit, 1000); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.Limit, 1000); assert(result.account.defaultConfig); - assert.strictEqual(result.account.defaultConfig.limit, 500); + assert.strictEqual(result.account.defaultConfig.RequestsPerSecond.Limit, 500); }); it('should validate account limit against nodes', () => { @@ -973,8 +980,10 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.account.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, constants.rateLimitDefaultBurstCapacity * 1000); + assert.strictEqual( + result.account.defaultConfig.RequestsPerSecond.BurstCapacity, + constants.rateLimitDefaultBurstCapacity + ); }); it('should use custom burstCapacity when provided', () => { @@ -991,8 +1000,9 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.account.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, 20 * 1000); + assert.strictEqual( + result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 20 + ); }); it('should accept float burstCapacity', () => { @@ -1009,8 +1019,9 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - const bucketSize = result.account.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, 1.5 * 1000); + assert.strictEqual( + result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5 + ); }); it('should throw if burstCapacity is negative', () => { @@ -1142,14 +1153,14 @@ describe('parseRateLimitConfig', () => { }); describe('calculation verification', () => { - it('should calculate correct interval for distributed setup', () => { + it('should store Limit in defaultConfig for distributed setup', () => { const config = { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', nodes: 2, bucket: { defaultConfig: { requestsPerSecond: { - limit: 100, // 100 req/s global + limit: 100, }, }, }, @@ -1157,13 +1168,14 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); - // Per-NODE rate = 100 / 2 nodes = 50 req/s - // Interval = 1000ms / 50 = 20ms - const interval = result.bucket.defaultConfig.requestsPerSecond.interval; - assert.strictEqual(interval, 20); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.Limit, 100); + assert.strictEqual( + result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, + constants.rateLimitDefaultBurstCapacity + ); }); - it('should calculate correct bucketSize from burstCapacity', () => { + it('should store BurstCapacity from burstCapacity', () => { const config = { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', bucket: { @@ -1178,19 +1190,17 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); - // bucketSize = burstCapacity * 1000 - const bucketSize = result.bucket.defaultConfig.requestsPerSecond.bucketSize; - assert.strictEqual(bucketSize, 15 * 1000); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 15); }); - it('should handle single node and single worker', () => { + it('should handle single node setup', () => { const config = { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', nodes: 1, bucket: { defaultConfig: { requestsPerSecond: { - limit: 50, // 50 req/s + limit: 50, }, }, }, @@ -1198,10 +1208,7 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); - // Per-node rate = 50 / 1 = 50 req/s - // Interval = 1000ms / 50 = 20ms - const interval = result.bucket.defaultConfig.requestsPerSecond.interval; - assert.strictEqual(interval, 20); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.Limit, 50); }); it('should handle high-scale distributed setup', () => { @@ -1211,7 +1218,8 @@ describe('parseRateLimitConfig', () => { bucket: { defaultConfig: { requestsPerSecond: { - limit: 10000, // 10,000 req/s global + limit: 10000, + burstCapacity: 5, }, }, }, @@ -1219,10 +1227,8 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); - // Per-NODE rate = 10000 / 10 nodes = 1000 req/s - // Interval = 1000ms / 1000 = 1ms - const interval = result.bucket.defaultConfig.requestsPerSecond.interval; - assert.strictEqual(interval, 1); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.Limit, 10000); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 5); }); }); }); diff --git a/tests/unit/api/apiUtils/rateLimit/helpers.js b/tests/unit/api/apiUtils/rateLimit/helpers.js index 39c49060b3..953f335ce0 100644 --- a/tests/unit/api/apiUtils/rateLimit/helpers.js +++ b/tests/unit/api/apiUtils/rateLimit/helpers.js @@ -3,7 +3,6 @@ const sinon = require('sinon'); const { config } = require('../../../../../lib/Config'); const cache = require('../../../../../lib/api/apiUtils/rateLimit/cache'); const helpers = require('../../../../../lib/api/apiUtils/rateLimit/helpers'); -const constants = require('../../../../../constants'); const tokenBucket = require('../../../../../lib/api/apiUtils/rateLimit/tokenBucket'); describe('Rate limit helpers', () => { @@ -20,6 +19,7 @@ describe('Rate limit helpers', () => { }; // Clear cache before each test cache.configCache.clear(); + cache.bucketOwnerCache.clear(); // Clear token buckets tokenBucket.getAllTokenBuckets().clear(); }); @@ -28,22 +28,111 @@ describe('Rate limit helpers', () => { sandbox.restore(); }); - describe('extractBucketRateLimitConfig', () => { - let configStub; + describe('requestNeedsRateCheck', () => { + it('should return false when rate limiting is disabled', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: false, + }); + + const request = { apiMethod: 'objectGet' }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), false); + }); + + it('should return false for rate limit admin API actions', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + for (const action of helpers.rateLimitApiActions) { + const request = { apiMethod: action }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), false, + `Expected false for rate limit action: ${action}`); + } + }); + it('should return false for internal service requests', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + const request = { + apiMethod: 'objectGet', + isInternalServiceRequest: true, + }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), false); + }); + + it('should return false when both bucket and account are already checked', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + const request = { + apiMethod: 'objectGet', + rateLimitAccountAlreadyChecked: true, + rateLimitBucketAlreadyChecked: true, + }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), false); + }); + + it('should return true when only bucket is already checked', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + const request = { + apiMethod: 'objectGet', + rateLimitAccountAlreadyChecked: false, + rateLimitBucketAlreadyChecked: true, + }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), true); + }); + + it('should return true when only account is already checked', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + const request = { + apiMethod: 'objectGet', + rateLimitAccountAlreadyChecked: true, + rateLimitBucketAlreadyChecked: false, + }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), true); + }); + + it('should return true for a normal request needing rate check', () => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + }); + + const request = { + apiMethod: 'objectGet', + isInternalServiceRequest: false, + rateLimitAccountAlreadyChecked: false, + rateLimitBucketAlreadyChecked: false, + }; + assert.strictEqual(helpers.requestNeedsRateCheck(request), true); + }); + }); + + describe('extractBucketRateLimitConfig', () => { beforeEach(() => { - configStub = sandbox.stub(config, 'rateLimiting').value({ + sandbox.stub(config, 'rateLimiting').value({ enabled: true, + serviceUserArn: 'foo', bucket: { configCacheTTL: 30000, - defaultBurstCapacity: 1, + defaultConfig: { + RequestsPerSecond: { BurstCapacity: 1 }, + }, }, }); }); it('should extract per-bucket config', () => { - const bucketName = 'test-bucket'; const mockBucket = { + getName: () => 'test-bucket', getRateLimitConfiguration: () => ({ getData: () => ({ RequestsPerSecond: { Limit: 200 }, @@ -51,92 +140,81 @@ describe('Rate limit helpers', () => { }), }; - const result = helpers.extractBucketRateLimitConfig(mockBucket, bucketName, mockLog); + const result = helpers.extractBucketRateLimitConfig(mockBucket, mockLog); - assert.deepStrictEqual(result, { limit: 200, burstCapacity: 1000, source: 'bucket' }); + assert.deepStrictEqual(result, { + RequestsPerSecond: { BurstCapacity: 1, Limit: 200, source: 'resource' }, + }); }); - it('should fall back to global default config when no bucket config', () => { - const bucketName = 'test-bucket'; + it('should use global defaults when bucket has no per-resource config', () => { const mockBucket = { - getRateLimitConfiguration: () => null, + getName: () => 'test-bucket', + getRateLimitConfiguration: () => ({ + getData: () => ({ + RequestsPerSecond: undefined, + }), + }), }; - configStub.value({ - enabled: true, - bucket: { - defaultConfig: { limit: 100 }, - defaultBurstCapacity: 1, - configCacheTTL: 30000, - }, - }); - - const result = helpers.extractBucketRateLimitConfig(mockBucket, bucketName, mockLog); + const result = helpers.extractBucketRateLimitConfig(mockBucket, mockLog); - assert.deepStrictEqual(result, { limit: 100, burstCapacity: 1000, source: 'global' }); + assert.deepStrictEqual(result, { + RequestsPerSecond: { BurstCapacity: 1, source: 'global' }, + }); }); - it('should return null when no config exists', () => { - const bucketName = 'test-bucket'; + it('should fall back to global default config when no bucket config', () => { const mockBucket = { + getName: () => 'test-bucket', getRateLimitConfiguration: () => null, }; - configStub.value({ + sandbox.stub(config, 'rateLimiting').value({ enabled: true, bucket: { - defaultBurstCapacity: 1, + defaultConfig: { + RequestsPerSecond: { Limit: 100, BurstCapacity: 1 }, + }, configCacheTTL: 30000, }, }); - const result = helpers.extractBucketRateLimitConfig(mockBucket, bucketName, mockLog); + const result = helpers.extractBucketRateLimitConfig(mockBucket, mockLog); - assert.strictEqual(result, null); + assert.deepStrictEqual(result, { + RequestsPerSecond: { Limit: 100, BurstCapacity: 1, source: 'global' }, + }); }); - it('should return null when global default limit is 0', () => { - const bucketName = 'test-bucket'; + it('should return global defaults with no Limit when defaultConfig has no Limit', () => { const mockBucket = { + getName: () => 'test-bucket', getRateLimitConfiguration: () => null, }; - configStub.value({ - enabled: true, - bucket: { - defaultConfig: { limit: 0 }, - defaultBurstCapacity: 1, - configCacheTTL: 30000, - }, - }); - - const result = helpers.extractBucketRateLimitConfig(mockBucket, bucketName, mockLog); + const result = helpers.extractBucketRateLimitConfig(mockBucket, mockLog); - assert.strictEqual(result, null); + assert.deepStrictEqual(result, { + RequestsPerSecond: { BurstCapacity: 1, source: 'global' }, + }); }); - it('should use default TTL when configCacheTTL is not set', () => { - const bucketName = 'test-bucket'; + it('should merge per-bucket config over global defaults', () => { const mockBucket = { + getName: () => 'test-bucket', getRateLimitConfiguration: () => ({ getData: () => ({ - RequestsPerSecond: { Limit: 200 }, + RequestsPerSecond: { Limit: 500, BurstCapacity: 10 }, }), }), }; - configStub.value({ - enabled: true, - bucket: { - defaultBurstCapacity: 1, - }, - }); - - sandbox.stub(constants, 'rateLimitDefaultConfigCacheTTL').value(60000); + const result = helpers.extractBucketRateLimitConfig(mockBucket, mockLog); - const result = helpers.extractBucketRateLimitConfig(mockBucket, bucketName, mockLog); - - assert.deepStrictEqual(result, { limit: 200, burstCapacity: 1000, source: 'bucket' }); + assert.deepStrictEqual(result, { + RequestsPerSecond: { BurstCapacity: 10, Limit: 500, source: 'resource' }, + }); }); }); @@ -317,4 +395,277 @@ describe('Rate limit helpers', () => { assert.strictEqual(bucket.tokens, 48); }); }); + + describe('extractRateLimitConfigFromRequest', () => { + beforeEach(() => { + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + serviceUserArn: 'foo', + bucket: { + configCacheTTL: 30000, + defaultConfig: { + RequestsPerSecond: { BurstCapacity: 1 }, + }, + }, + account: { + configCacheTTL: 30000, + defaultConfig: { + RequestsPerSecond: { BurstCapacity: 2 }, + }, + }, + }); + }); + + it('should return both bucket and account configs', () => { + const mockBucket = { + getName: () => 'test-bucket', + getRateLimitConfiguration: () => ({ + getData: () => ({ + RequestsPerSecond: { Limit: 200 }, + }), + }), + }; + const mockAuthInfo = { + getCanonicalID: () => 'account-123', + }; + const request = { + accountLimits: { + RequestsPerSecond: { Limit: 500 }, + }, + }; + + const result = helpers.extractRateLimitConfigFromRequest( + request, mockAuthInfo, mockBucket, mockLog); + + assert.deepStrictEqual(result.bucket, { + RequestsPerSecond: { BurstCapacity: 1, Limit: 200, source: 'resource' }, + }); + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, Limit: 500, source: 'resource' }, + }); + }); + + it('should use global defaults when no per-resource configs exist', () => { + const mockBucket = { + getName: () => 'test-bucket', + getRateLimitConfiguration: () => null, + }; + const mockAuthInfo = { + getCanonicalID: () => 'account-123', + }; + const request = {}; + + const result = helpers.extractRateLimitConfigFromRequest( + request, mockAuthInfo, mockBucket, mockLog); + + assert.deepStrictEqual(result.bucket, { + RequestsPerSecond: { BurstCapacity: 1, source: 'global' }, + }); + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, + }); + }); + + it('should extract per-account config with resource source', () => { + const mockBucket = { + getName: () => 'test-bucket', + getRateLimitConfiguration: () => null, + }; + const mockAuthInfo = { + getCanonicalID: () => 'account-123', + }; + const request = { + accountLimits: { + RequestsPerSecond: { Limit: 300, BurstCapacity: 5 }, + }, + }; + + const result = helpers.extractRateLimitConfigFromRequest( + request, mockAuthInfo, mockBucket, mockLog); + + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 5, Limit: 300, source: 'resource' }, + }); + }); + + it('should use global source when accountLimits has no RequestsPerSecond', () => { + const mockBucket = { + getName: () => 'test-bucket', + getRateLimitConfiguration: () => null, + }; + const mockAuthInfo = { + getCanonicalID: () => 'account-123', + }; + const request = { + accountLimits: { + RequestsPerSecond: undefined, + }, + }; + + const result = helpers.extractRateLimitConfigFromRequest( + request, mockAuthInfo, mockBucket, mockLog); + + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, + }); + }); + }); + + describe('buildRateChecksFromConfig', () => { + it('should build a check when Limit is set and positive', () => { + const limitConfig = { + RequestsPerSecond: { Limit: 100, BurstCapacity: 2, source: 'resource' }, + }; + + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', limitConfig); + + assert.strictEqual(checks.length, 1); + assert.deepStrictEqual(checks[0], { + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + source: 'resource', + config: { + limit: 100, + burstCapacity: 2000, + }, + }); + }); + + it('should return empty array when Limit is 0', () => { + const limitConfig = { + RequestsPerSecond: { Limit: 0, BurstCapacity: 2, source: 'global' }, + }; + + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', limitConfig); + + assert.strictEqual(checks.length, 0); + }); + + it('should return empty array when Limit is undefined', () => { + const limitConfig = { + RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, + }; + + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', limitConfig); + + assert.strictEqual(checks.length, 0); + }); + + it('should return empty array when limitConfig is null', () => { + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', null); + + assert.strictEqual(checks.length, 0); + }); + + it('should return empty array when limitConfig is undefined', () => { + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', undefined); + + assert.strictEqual(checks.length, 0); + }); + + it('should return empty array when RequestsPerSecond is missing', () => { + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', {}); + + assert.strictEqual(checks.length, 0); + }); + + it('should multiply BurstCapacity by 1000', () => { + const limitConfig = { + RequestsPerSecond: { Limit: 50, BurstCapacity: 5, source: 'global' }, + }; + + const checks = helpers.buildRateChecksFromConfig('acc', 'account-1', limitConfig); + + assert.strictEqual(checks[0].config.burstCapacity, 5000); + }); + + it('should return empty array when Limit is negative', () => { + const limitConfig = { + RequestsPerSecond: { Limit: -1, BurstCapacity: 2, source: 'global' }, + }; + + const checks = helpers.buildRateChecksFromConfig('bkt', 'test-bucket', limitConfig); + + assert.strictEqual(checks.length, 0); + }); + }); + + describe('getCachedRateLimitConfig', () => { + it('should return empty object when nothing is cached', () => { + const request = { bucketName: 'test-bucket' }; + + const result = helpers.getCachedRateLimitConfig(request); + + assert.deepStrictEqual(result, {}); + }); + + it('should return cached bucket config when available', () => { + const bucketConfig = { + RequestsPerSecond: { Limit: 100, source: 'resource' }, + }; + cache.setCachedConfig(cache.namespace.bucket, 'test-bucket', bucketConfig, 30000); + + const request = { bucketName: 'test-bucket' }; + const result = helpers.getCachedRateLimitConfig(request); + + assert.deepStrictEqual(result.bucket, bucketConfig); + assert.strictEqual(result.account, undefined); + }); + + it('should return cached account config when bucket owner and account config are cached', () => { + const accountConfig = { + RequestsPerSecond: { Limit: 500, source: 'global' }, + }; + cache.setCachedBucketOwner('test-bucket', 'owner-123', 30000); + cache.setCachedConfig(cache.namespace.account, 'owner-123', accountConfig, 30000); + + const request = { bucketName: 'test-bucket' }; + const result = helpers.getCachedRateLimitConfig(request); + + assert.deepStrictEqual(result.account, accountConfig); + assert.strictEqual(result.bucketOwner, 'owner-123'); + }); + + it('should return both bucket and account configs when all are cached', () => { + const bucketConfig = { + RequestsPerSecond: { Limit: 100, source: 'resource' }, + }; + const accountConfig = { + RequestsPerSecond: { Limit: 500, source: 'global' }, + }; + cache.setCachedConfig(cache.namespace.bucket, 'test-bucket', bucketConfig, 30000); + cache.setCachedBucketOwner('test-bucket', 'owner-123', 30000); + cache.setCachedConfig(cache.namespace.account, 'owner-123', accountConfig, 30000); + + const request = { bucketName: 'test-bucket' }; + const result = helpers.getCachedRateLimitConfig(request); + + assert.deepStrictEqual(result.bucket, bucketConfig); + assert.deepStrictEqual(result.account, accountConfig); + assert.strictEqual(result.bucketOwner, 'owner-123'); + }); + + it('should not return account config when bucket owner is cached but account config is not', () => { + cache.setCachedBucketOwner('test-bucket', 'owner-123', 30000); + + const request = { bucketName: 'test-bucket' }; + const result = helpers.getCachedRateLimitConfig(request); + + assert.strictEqual(result.account, undefined); + assert.strictEqual(result.bucketOwner, undefined); + }); + + it('should not return account config when bucket owner is not cached', () => { + const accountConfig = { + RequestsPerSecond: { Limit: 500, source: 'global' }, + }; + cache.setCachedConfig(cache.namespace.account, 'owner-123', accountConfig, 30000); + + const request = { bucketName: 'test-bucket' }; + const result = helpers.getCachedRateLimitConfig(request); + + assert.strictEqual(result.account, undefined); + }); + }); }); diff --git a/tests/unit/api/apiUtils/rateLimit/refillJob.js b/tests/unit/api/apiUtils/rateLimit/refillJob.js index b6e9d90595..0e870f33a9 100644 --- a/tests/unit/api/apiUtils/rateLimit/refillJob.js +++ b/tests/unit/api/apiUtils/rateLimit/refillJob.js @@ -1,15 +1,30 @@ const assert = require('assert'); const sinon = require('sinon'); +const { config } = require('../../../../../lib/Config'); const refillJob = require('../../../../../lib/api/apiUtils/rateLimit/refillJob'); const tokenBucket = require('../../../../../lib/api/apiUtils/rateLimit/tokenBucket'); const logger = require('../../../../../lib/utilities/logger'); describe('Token refill job', () => { let sandbox; + let mockLog; beforeEach(() => { sandbox = sinon.createSandbox(); + sandbox.stub(config, 'rateLimiting').value({ + enabled: true, + nodes: 1, + tokenBucketBufferSize: 50, + tokenBucketRefillThreshold: 20, + }); + mockLog = { + trace: sinon.stub(), + debug: sinon.stub(), + info: sinon.stub(), + warn: sinon.stub(), + error: sinon.stub(), + }; // Clear token buckets tokenBucket.getAllTokenBuckets().clear(); @@ -34,16 +49,10 @@ describe('Token refill job', () => { }); it('should check all active token buckets', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - // Create 3 buckets - const bucket1 = tokenBucket.getTokenBucket('bucket-1', { limit: 100 }, mockLog); - const bucket2 = tokenBucket.getTokenBucket('bucket-2', { limit: 200 }, mockLog); - const bucket3 = tokenBucket.getTokenBucket('bucket-3', { limit: 300 }, mockLog); + const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', { limit: 100 }, mockLog); + const bucket2 = tokenBucket.getTokenBucket('bkt', 'bucket-2', 'rps', { limit: 200 }, mockLog); + const bucket3 = tokenBucket.getTokenBucket('bkt', 'bucket-3', 'rps', { limit: 300 }, mockLog); // Stub refillIfNeeded to prevent actual refill sandbox.stub(bucket1, 'refillIfNeeded').resolves(); @@ -59,13 +68,7 @@ describe('Token refill job', () => { }); it('should call refillIfNeeded on each bucket', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket = tokenBucket.getTokenBucket('test-bucket', { limit: 100 }, mockLog); + const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', { limit: 100 }, mockLog); const refillSpy = sandbox.stub(bucket, 'refillIfNeeded').resolves(); await refillJob.refillTokenBuckets(logger); @@ -74,13 +77,7 @@ describe('Token refill job', () => { }); it('should handle refill errors gracefully', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket = tokenBucket.getTokenBucket('test-bucket', { limit: 100 }, mockLog); + const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', { limit: 100 }, mockLog); sandbox.stub(bucket, 'refillIfNeeded').rejects(new Error('Refill failed')); // Should not throw @@ -90,14 +87,8 @@ describe('Token refill job', () => { }); it('should process multiple buckets in parallel', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket1 = tokenBucket.getTokenBucket('bucket-1', { limit: 100 }, mockLog); - const bucket2 = tokenBucket.getTokenBucket('bucket-2', { limit: 200 }, mockLog); + const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', { limit: 100 }, mockLog); + const bucket2 = tokenBucket.getTokenBucket('bkt', 'bucket-2', 'rps', { limit: 200 }, mockLog); let refill1Called = false; let refill2Called = false; @@ -117,14 +108,8 @@ describe('Token refill job', () => { }); it('should wait for all refills to complete', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket1 = tokenBucket.getTokenBucket('bucket-1', { limit: 100 }, mockLog); - const bucket2 = tokenBucket.getTokenBucket('bucket-2', { limit: 200 }, mockLog); + const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', { limit: 100 }, mockLog); + const bucket2 = tokenBucket.getTokenBucket('bkt', 'bucket-2', 'rps', { limit: 200 }, mockLog); let call1 = false; let call2 = false; @@ -197,13 +182,7 @@ describe('Token refill job', () => { describe('Integration scenarios', () => { it('should call refillIfNeeded on buckets below threshold', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket = tokenBucket.getTokenBucket('test-bucket', { limit: 100 }, mockLog); + const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', { limit: 100 }, mockLog); bucket.tokens = 10; // Below threshold (20) // Stub refillIfNeeded @@ -215,33 +194,25 @@ describe('Token refill job', () => { }); it('should skip refill for buckets above threshold', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket = tokenBucket.getTokenBucket('test-bucket', { limit: 100 }, mockLog); + const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', { limit: 100 }, mockLog); bucket.tokens = 40; // Above threshold (20) - const refillSpy = sandbox.spy(bucket, 'refill'); + // refillIfNeeded is always called by the job, but it checks + // the threshold internally and returns early without refilling + const refillSpy = sandbox.spy(bucket, 'refillIfNeeded'); await refillJob.refillTokenBuckets(logger); - // Refill should not have been called - assert.strictEqual(refillSpy.called, false); + // refillIfNeeded was called, but tokens should be unchanged + // (no actual refill happened since above threshold) + assert.strictEqual(refillSpy.calledOnce, true); + assert.strictEqual(bucket.tokens, 40); }); it('should handle concurrent refills for multiple buckets', async () => { - const mockLog = { - trace: sinon.stub(), - debug: sinon.stub(), - error: sinon.stub(), - }; - - const bucket1 = tokenBucket.getTokenBucket('bucket-1', { limit: 100 }, mockLog); - const bucket2 = tokenBucket.getTokenBucket('bucket-2', { limit: 200 }, mockLog); - const bucket3 = tokenBucket.getTokenBucket('bucket-3', { limit: 300 }, mockLog); + const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', { limit: 100 }, mockLog); + const bucket2 = tokenBucket.getTokenBucket('bkt', 'bucket-2', 'rps', { limit: 200 }, mockLog); + const bucket3 = tokenBucket.getTokenBucket('bkt', 'bucket-3', 'rps', { limit: 300 }, mockLog); // All below threshold bucket1.tokens = 5; diff --git a/tests/unit/api/apiUtils/rateLimit/tokenBucket.js b/tests/unit/api/apiUtils/rateLimit/tokenBucket.js index fdd490b294..7959c01adc 100644 --- a/tests/unit/api/apiUtils/rateLimit/tokenBucket.js +++ b/tests/unit/api/apiUtils/rateLimit/tokenBucket.js @@ -299,8 +299,6 @@ describe('Token bucket management functions', () => { assert.strictEqual(bucket1, bucket2); assert.strictEqual(bucket2.limitConfig.limit, 200); - assert(mockLog.info.calledOnce); - assert(mockLog.info.firstCall.args[0].includes('Updated token bucket limit config')); }); it('should not log update when limit is unchanged', () => {