AlinsRan commented on code in PR #13840:
URL: https://github.com/apache/apisix/pull/13840#discussion_r3819305663
##########
apisix/plugins/graphql-limit-count.lua:
##########
@@ -25,30 +28,115 @@ local pairs = pairs
local ipairs = ipairs
local pcall = pcall
local max = math.max
+local ceil = math.ceil
local tonumber = tonumber
local GRAPHQL_DEFAULT_MAX_SIZE = 1048576
+local QUERY_COST_HEADER = "X-Graphql-Query-Cost"
local plugin_name = "graphql-limit-count"
+
+-- The plugin reuses the whole limit-count configuration surface and adds the
cost
+-- model on top of it. limit_count.schema is shared with limit-count,
+-- limit-count-advanced and ai-rate-limiting, so it must not be mutated in
place.
+local schema = core.table.deepcopy(limit_count.schema)
+
+schema.properties.cost_strategy = {
+ type = "string",
+ enum = {"depth", "complexity", "node_quantifier"},
+ -- "depth" is what this plugin has always done; keeping it as the default
means
+ -- an existing configuration keeps its current cost after the upgrade.
+ default = "depth",
+}
+schema.properties.max_cost = {
+ type = "number",
+ minimum = 0,
+ default = 0,
+ description = "reject with 403 above this cost, 0 disables the check",
+}
+schema.properties.score_factor = {
+ type = "number",
+ exclusiveMinimum = 0,
+ default = 1,
+ description = "scaling applied to the raw cost before the quota is
charged",
+}
+schema.properties.resolve_variables = {
+ type = "boolean",
+ -- On by default: with it off, moving `first: 10000` to `first: $n` makes
the
+ -- same request cost a fraction of what the literal costs, which is a
bypass
+ -- of max_cost and of the quota. Turn it off only to reproduce an engine
that
+ -- ignores variables.
+ default = true,
+ description = "resolve GraphQL variables and schema argument defaults when
" ..
+ "computing the cost, instead of treating them as absent",
+}
+schema.properties.introspection_endpoint = {
+ type = "string",
+ pattern = "^https?://",
+ description = "explicit schema introspection endpoint, derived from the "
..
+ "upstream when unset",
+}
+schema.properties.introspection_headers = {
+ type = "object",
+ patternProperties = {["^[^:]+$"] = {type = "string"}},
+ -- Deliberately not taken from the request. The schema is cached per
service,
+ -- so an introspection whose result depends on the caller would let
whichever
+ -- request warms a worker pick the schema every later request is costed
+ -- against, and would let one caller's bad credentials cache a failure that
+ -- rejects everyone else. Credentials for it belong to the operator.
+ description = "headers sent on the schema introspection request, for an "
..
+ "upstream whose introspection needs credentials",
+}
Review Comment:
Fixed in `d8804a5b`, and it was slightly worse than described: this plugin
declared no `encrypt_fields` of its own at all, only what came along with
`limit-count`'s schema deepcopy. `introspection_headers` is appended to that
list, and a case asserts both it and `redis_password` are in it.
##########
apisix/plugins/graphql-limit-count.lua:
##########
@@ -146,6 +245,63 @@ local function node_depth(node, fragments, visited, memo,
cycle)
end
+-- Returns the depth, or nil plus the log line and the client message when the
+-- document is not valid GraphQL to begin with.
+local function query_depth(operations, fragments)
+ local depth = 0
+ local memo = {}
+ local cycle = {found = false}
+ for _, op in ipairs(operations) do
+ depth = max(depth, node_depth(op, fragments, {}, memo, cycle))
+ end
+
+ if cycle.found then
+ return nil, "invalid graphql request: fragment spreads form a cycle",
+ "Invalid graphql request: fragment spreads must not form cycles"
+ end
+
+ depth = max(depth, 1)
+ core.log.info("graphql node depth: ", depth)
+ return depth
+end
+
+
+-- Returns the raw cost of the query, or nil plus an error message.
+local function raw_query_cost(conf, ctx, operations, fragments, variables)
+ if conf.cost_strategy == "depth" then
+ return query_depth(operations, fragments)
+ end
+
+ -- Decorations are owned by the service; a route that is not bound to one
has
+ -- no place to hang them, so the cost model simply does not apply there.
+ local service_decorations
+ if ctx.service_id then
+ service_decorations = decorations.get(ctx.service_id)
+ else
+ -- info, not warn: this is on the request path, and the effect is
already
+ -- visible on every response through X-Graphql-Query-Cost
+ core.log.info("the route is not bound to a service, so it has no
graphql ",
+ "cost decorations; the query cost degenerates to the
node count")
+ end
+
+ local schema_index
+ if service_decorations then
+ local err
+ schema_index, err = introspection.get(conf, ctx)
+ if not schema_index then
+ return nil, err
+ end
+ end
+
+ return gql_cost.query_cost(conf.cost_strategy, operations, fragments, {
+ decorations = service_decorations,
+ schema = schema_index,
+ variables = conf.resolve_variables and variables or nil,
+ use_defaults = conf.resolve_variables,
+ })
Review Comment:
The documentation was wrong here, not the behaviour -- fixed in `d8804a5b`.
`node_quantifier` charges only nodes that carry a quantifier argument, so with
no decorations there is nothing to charge and the floor gives 1. Falling back
to the complexity walk would make the strategy mean something different
depending on whether a decoration happens to exist, which is worse than the
current answer. The docs now state what each strategy does when there are no
decorations.
##########
apisix/plugins/graphql-limit-count.lua:
##########
@@ -203,23 +360,66 @@ function _M.access(conf, ctx)
return 400, {message = "Invalid graphql request: empty graphql query"}
end
- local depth = 0
- local memo = {}
- local cycle = {found = false}
- for _, op in ipairs(operations) do
- local d = node_depth(op, fragments, {}, memo, cycle)
- depth = max(depth, d)
+ -- A document with several operations only executes the one `operationName`
+ -- selects, so that is the one to charge for. Without it the request is
not a
+ -- valid multi operation request at all; the whole document is then costed
and
+ -- the most expensive operation charged, which cannot under charge
whichever
+ -- one the upstream ends up running.
+ if operation_name and #operations > 1 then
+ for _, op in ipairs(operations) do
+ if op.name and op.name.value == operation_name then
+ operations = {op}
+ break
+ end
+ end
end
- if cycle.found then
- core.log.error("invalid graphql request: fragment spreads form a
cycle")
- return 400, {message = "Invalid graphql request: fragment spreads must
not form cycles"}
+ local raw_cost, client_msg
+ raw_cost, err, client_msg = raw_query_cost(conf, ctx, operations,
fragments, variables)
+ if not raw_cost then
+ -- a malformed document reports its own message; anything else is the
+ -- introspection failing
+ core.log.error(client_msg and err
+ or "failed to compute the graphql query cost: " .. err)
+ return 400, {message = client_msg
+ or "Invalid graphql request: failed to
introspect the "
+ .. "upstream graphql schema"}
end
- depth = max(depth, 1)
- core.log.info("graphql query depth: ", depth)
+ -- The +0.01 floor makes a query whose nodes are all undecorated still
cost 1.
+ -- "depth" is never 0 and has always charged exactly the depth, so it is
left
+ -- alone: with the default score_factor of 1 the cost is unchanged.
+ if conf.cost_strategy ~= "depth" then
+ raw_cost = raw_cost + 0.01
+ end
+
+ -- ceil keeps the value an integer, which the Redis backend requires anyway
+ local cost = max(ceil(raw_cost * (conf.score_factor or 1)), 1)
Review Comment:
Correct on the arithmetic: `max(..., 1)` already provides the minimum
charge, and the epsilon rounds every integral cost up by one. It is deliberate
-- part of the cost model's definition rather than a floor -- and the published
cost of every documented example includes it, so dropping it means restating
them all. The comment claimed to be the thing that makes an undecorated query
cost 1, which was wrong; `d8804a5b` makes it say what it actually does. Happy
to remove it if the cleaner arithmetic is worth restating the numbers, but that
is a product call rather than a correctness one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
