-
Notifications
You must be signed in to change notification settings - Fork 77
feat: update method for merging ISC with TOML config #5712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bae53b9
3bddedd
7ce5ee5
923aba7
e6d1243
c95fc75
965e24e
5e9471f
ef47e15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,32 +3,35 @@ import { basename, extname, dirname, join } from 'path' | |
|
||
import isPathInside from 'is-path-inside' | ||
import mergeOptions from 'merge-options' | ||
import { z } from 'zod' | ||
|
||
import { FunctionSource } from './function.js' | ||
import type { NodeBundlerName } from './runtimes/node/bundlers/types.js' | ||
import type { ModuleFormat } from './runtimes/node/utils/module_format.js' | ||
import { nodeBundler } from './runtimes/node/bundlers/types.js' | ||
import { moduleFormat } from './runtimes/node/utils/module_format.js' | ||
import { minimatch } from './utils/matching.js' | ||
|
||
interface FunctionConfig { | ||
externalNodeModules?: string[] | ||
includedFiles?: string[] | ||
includedFilesBasePath?: string | ||
ignoredNodeModules?: string[] | ||
nodeBundler?: NodeBundlerName | ||
nodeSourcemap?: boolean | ||
nodeVersion?: string | ||
rustTargetDirectory?: string | ||
schedule?: string | ||
zipGo?: boolean | ||
name?: string | ||
generator?: string | ||
timeout?: number | ||
export const functionConfig = z.object({ | ||
externalNodeModules: z.array(z.string()).optional().catch([]), | ||
generator: z.string().optional().catch(undefined), | ||
includedFiles: z.array(z.string()).optional().catch([]), | ||
includedFilesBasePath: z.string().optional().catch(undefined), | ||
ignoredNodeModules: z.array(z.string()).optional().catch([]), | ||
name: z.string().optional().catch(undefined), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's a lot of duplication in this. does mit make sense to extract There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean, but at the same time I don't know what would be the criteria for extracting something into a constant. We could have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any strings where we don't have a Maybe it'd already help to add a few empty lines in between the different parameters. That'd make it less of a uniform wall of text. |
||
nodeBundler: nodeBundler.optional().catch(undefined), | ||
nodeSourcemap: z.boolean().optional().catch(undefined), | ||
nodeVersion: z.string().optional().catch(undefined), | ||
rustTargetDirectory: z.string().optional().catch(undefined), | ||
schedule: z.string().optional().catch(undefined), | ||
timeout: z.number().optional().catch(undefined), | ||
zipGo: z.boolean().optional().catch(undefined), | ||
|
||
// Temporary configuration property, only meant to be used by the deploy | ||
// configuration API. Once we start emitting ESM files for all ESM functions, | ||
// we can remove this. | ||
nodeModuleFormat?: ModuleFormat | ||
} | ||
nodeModuleFormat: moduleFormat.optional().catch(undefined), | ||
}) | ||
|
||
type FunctionConfig = z.infer<typeof functionConfig> | ||
|
||
interface FunctionConfigFile { | ||
config: FunctionConfig | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,30 +1,69 @@ | ||||||||
export enum RateLimitAlgorithm { | ||||||||
SlidingWindow = 'sliding_window', | ||||||||
} | ||||||||
import { z } from 'zod' | ||||||||
|
||||||||
export enum RateLimitAggregator { | ||||||||
Domain = 'domain', | ||||||||
IP = 'ip', | ||||||||
export interface TrafficRules { | ||||||||
action: { | ||||||||
type: string | ||||||||
config: { | ||||||||
rateLimitConfig: { | ||||||||
algorithm: string | ||||||||
windowSize: number | ||||||||
windowLimit: number | ||||||||
} | ||||||||
aggregate: { | ||||||||
keys: { | ||||||||
type: string | ||||||||
}[] | ||||||||
} | ||||||||
to?: string | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
export enum RateLimitAction { | ||||||||
Limit = 'rate_limit', | ||||||||
Rewrite = 'rewrite', | ||||||||
} | ||||||||
const rateLimitAction = z.enum(['rate_limit', 'rewrite']) | ||||||||
const rateLimitAlgorithm = z.enum(['sliding_window']) | ||||||||
const rateLimitAggregator = z.enum(['domain', 'ip']) | ||||||||
const slidingWindow = z.object({ | ||||||||
windowLimit: z.number(), | ||||||||
windowSize: z.number(), | ||||||||
}) | ||||||||
const rewriteActionConfig = z.object({ | ||||||||
to: z.string(), | ||||||||
}) | ||||||||
|
||||||||
interface SlidingWindow { | ||||||||
windowLimit: number | ||||||||
windowSize: number | ||||||||
} | ||||||||
export const rateLimit = z | ||||||||
.object({ | ||||||||
action: rateLimitAction.optional(), | ||||||||
aggregateBy: rateLimitAggregator.or(z.array(rateLimitAggregator)).optional(), | ||||||||
algorithm: rateLimitAlgorithm.optional(), | ||||||||
}) | ||||||||
.merge(slidingWindow) | ||||||||
Skn0tt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
.merge(rewriteActionConfig.partial()) | ||||||||
|
||||||||
export type RewriteActionConfig = SlidingWindow & { | ||||||||
to: string | ||||||||
} | ||||||||
type RateLimit = z.infer<typeof rateLimit> | ||||||||
|
||||||||
interface RateLimitConfig { | ||||||||
action?: RateLimitAction | ||||||||
aggregateBy?: RateLimitAggregator | RateLimitAggregator[] | ||||||||
algorithm?: RateLimitAlgorithm | ||||||||
} | ||||||||
/** | ||||||||
* Takes a rate limiting configuration object and returns a traffic rules | ||||||||
* object that is added to the manifest. | ||||||||
*/ | ||||||||
export const getTrafficRulesConfig = (input: RateLimit): TrafficRules | undefined => { | ||||||||
const { windowSize, windowLimit, algorithm, aggregateBy, action, to } = input | ||||||||
const rateLimitAgg = Array.isArray(aggregateBy) ? aggregateBy : [rateLimitAggregator.Enum.domain] | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this incorrectly turns
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going by the current implementation:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, then let's keep the fixing of that to a different PR. @paulo I think you wrote this (and I reviewed ...), what are your thoughts on it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thought process at the time was to not fail the build and just use the default since it's not an obligatory field, but I'm now regretting it. If I could do it all over again I think I'd just fail the build if aggregateBy is not an array (if defined, not an obligatory field), but leaving the current logic is also ok |
||||||||
const rewriteConfig = to ? { to: input.to } : undefined | ||||||||
|
||||||||
export type RateLimit = RateLimitConfig & (SlidingWindow | RewriteActionConfig) | ||||||||
return { | ||||||||
action: { | ||||||||
type: action || rateLimitAction.Enum.rate_limit, | ||||||||
config: { | ||||||||
...rewriteConfig, | ||||||||
rateLimitConfig: { | ||||||||
windowLimit, | ||||||||
windowSize, | ||||||||
algorithm: algorithm || rateLimitAlgorithm.Enum.sliding_window, | ||||||||
}, | ||||||||
aggregate: { | ||||||||
keys: rateLimitAgg.map((agg) => ({ type: agg })), | ||||||||
}, | ||||||||
}, | ||||||||
}, | ||||||||
} | ||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure why, but using
undefined
as the fallback for an optional value makes more sense to me.