Skip to content
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

fix(dbless): handle unexpected types when flattening errors #10896

Merged
merged 4 commits into from
May 26, 2023

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented May 19, 2023

Summary

This fixes error-flattening for a case where the declarative validation is yielding really odd results (see the linked issue for more details). If two plugins are configured with the same consumer (as a string key), the error yielded is:

{
  code = 14,
  fields = {
    plugins = {
      {
        consumer = {
          id = "missing primary key"
        }
      }
    }
  },
  message = "declarative config is invalid: {plugins={{consumer={id=\"missing primary key\"}}}}",
  name = "invalid declarative configuration"
}

The input for consumer was a string, yet at some point the declarative schema seems to be getting confused (likely after failing to resolve it to a foreign reference) and treating it as an entity table, returning an error because string.id is obviously not a thing. Because an invalid lua type typically results in an @entity error and not a field error, this leads the error-flattening down a code path it was not expected to traverse, leading to an exception and 500 error.

A proper fix for the error-flattening code means not only guarding against the unexpected non-table input, but also updating some of the foreign relationship traversal/recursion logic to ensure that the error is still captured. The end result is that the error is reported as a field-level error of the plugin entity instead:

{
  entity_name = "rate-limiting",
  entity_tags = { --[[ snipped ]] },
  entity_type = "plugin",
  errors = {
    {
      field = "consumer.id",
      message = "missing primary key",
      type = "field"
    }
  },
  entity = {
    config = { --[[ snipped ]] },
    consumer = "774f8446-6427-43f9-9962-ce7ab8097fe4",
    enabled = true,
    name = "rate-limiting",
    protocols = { --[[ snipped ]] },
    tags = { --[[ snipped ]] },
  },
}

I suspect there's a bug deeper in the declarative schema/parser, because attempting to add two plugins of the same name to a consumer should yield a better error. That said, trying to unearth and fix a bug in that area of the code is going to require a lot more than just an afternoon for me, so I think we should first merge this change to ensure that we can get the fix for the 500 error onto the patch train as soon as possible. Also, fixing the problem at the declarative schema level is not mutually exclusive with the input-handling improvements made in this PR anyways.

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG
  • There is a user-facing docs PR N/A

Issue reference

Fix #10767

flrgh added 3 commits May 22, 2023 09:19
fixes #10767

This fixes error-flattening for a case where the declarative validation
yields really odd results.
@RobSerafini
Copy link
Contributor

@flrgh - I added a few more reviewers to try and get this moving.

@RobSerafini RobSerafini requested a review from kikito May 25, 2023 15:22
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on the new docs and refactor. For the future, I would suggest keeping the 'fix commit' as simple as possible, leaving any unrelated improvements / refactors to separate commits -- this IMHO makes the review much easier.

@gszr gszr merged commit 5df62c8 into master May 26, 2023
@gszr gszr deleted the fix/dbless-config-flatten-errors branch May 26, 2023 16:21
gszr pushed a commit that referenced this pull request May 26, 2023
@gszr
Copy link
Member

gszr commented May 26, 2023

I'll keep an eye on the backports. Michael - please cherry manually and send me the PRs (our automation doesn't handle multiple-commits / rebase-merge well)

@team-gateway-bot
Copy link
Collaborator

The backport to release/3.2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.2.x release/3.2.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.2.x
# Create a new branch
git switch --create backport-10896-to-release/3.2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5df62c8f8d7b6d1d36731305abe78f2d5e6f45e2
# Push it to GitHub
git push --set-upstream origin backport-10896-to-release/3.2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.2.x

Then, create a pull request where the base branch is release/3.2.x and the compare/head branch is backport-10896-to-release/3.2.x.

@team-gateway-bot
Copy link
Collaborator

The backport to release/3.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.3.x release/3.3.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.3.x
# Create a new branch
git switch --create backport-10896-to-release/3.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5df62c8f8d7b6d1d36731305abe78f2d5e6f45e2
# Push it to GitHub
git push --set-upstream origin backport-10896-to-release/3.3.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.3.x

Then, create a pull request where the base branch is release/3.3.x and the compare/head branch is backport-10896-to-release/3.3.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DB-less config apply relies on flatten_errors url param: attempt to index local 'entity'
4 participants