feat(clustering): report config reload errors to Konnect

[flrgh]

summary

This adds new error-reporting for data planes running in Konnect.

When config update fails due to invalid configuration or more transient reasons (e.g. LMDB map size exceeded), the data plane will collect the error into a JSON payload and send this back to the control-plane as a binary WebSocket message.

I have refactored some of the code in kong.clustering.config_helper.update() to make error-handling more ergonomic and robust (swapping in xpcall to get better exception data).

On the Konnect mode limitation

In its initial form, this feature is being added for Konnect-only deployments. Konnect is only supported by Enterprise versions of Kong, but this feature requires changes to files in the kong/clustering/* tree that would be very frustrating to maintain if committed directly to the EE codebase. I think refactoring could help with this obstacle, but with the current state of kong.clustering it would need to be a pretty large refactor.

For these reasons, the code has been added to OSS. Because the konnect_mode flag is only present in EE, I am using a custom plugin to monkey-patch kong.clustering.init_dp_worker() such that error reporting is always enabled. Otherwise we would not be able to test this code in OSS, making maintenance even more of a headache.

For OSS and non-Konnect EE deployments, the side-effect is that there is that the data plane's code path for reconfigure payloads does some unnecessary work preparing an error payload that will ultimately be discarded, but this is only in the error-handling path and should hopefully not become a performance concern.

NOTE: Because OSS data planes are not yet supported by Konnect, there is no changelog entry for this feature. I will add one for the EE pull request.

Example Payloads

Invalid Declarative Config

{
  "type": "error",
  "error": {
    "name": "declarative configuration parse failure",
    "config_hash": "706f787e686e32a565e2a578aba1b347",
    "source": "kong.db.declarative.parse_table"
    "message": "declarative config is invalid: {extra_top_level_field=\"unknown field\"}",
    "code": 14,
    "fields": {
      "extra_top_level_field": "unknown field"
    },
    "flattened_errors": [
      {
        "entity_id": "2e1373fa-874a-426c-b639-b9e20284ebbc",
        "entity_name": "my-service",
        "entity_tags": [
          "tag-1",
          "tag-2"
        ],
        "entity_type": "service",
        "errors": [
          {
            "field": "host",
            "message": "required field missing",
            "type": "field"
          },
          {
            "field": "extra_field",
            "message": "unknown field",
            "type": "field"
          }
        ]
      }
    ]
  }
}

Exception Thrown During Config Reload

{
  "type": "error",
  "error": {
    "name": "configuration reload failed",
    "config_hash": "8cb8c00863336fee14e4364ab3e8819c",
    "exception": "...plugins/kong/plugins/cluster-error-reporting/handler.lua:19: oh no!",
    "message": "an exception was raised while updating the configuration",
    "source": "kong.clustering.config_helper.update",
    "traceback": "...plugins/kong/plugins/cluster-error-reporting/handler.lua:19: oh no!\nstack traceback:\n\t...michaelm/git/kong/kong/kong/clustering/config_helper.lua:278: in function <...michaelm/git/kong/kong/kong/clustering/config_helper.lua:272>\n\t[C]: in function 'error'\n\t...plugins/kong/plugins/cluster-error-reporting/handler.lua:19: in function 'load_into_cache_with_events'\n\t...michaelm/git/kong/kong/kong/clustering/config_helper.lua:346: in function <...michaelm/git/kong/kong/kong/clustering/config_helper.lua:297>\n\t[C]: in function 'xpcall'\n\t...michaelm/git/kong/kong/kong/clustering/config_helper.lua:374: in function 'update'\n\t/home/michaelm/git/kong/kong/kong/clustering/data_plane.lua:255: in function </home/michaelm/git/kong/kong/kong/clustering/data_plane.lua:224>"
  }
}

LMDB Map Full

{
  "type": "error",
  "error": {
    "name": "configuration reload failed",
    "config_hash": "d78ed6c82b8aedf5323f249b3f0a363d",
    "message": "map full",
    "source": "kong.db.declarative.load_into_cache_with_events"
  }
}

See also

KAG-3249

[hbagdi]

I suggest making this more descriptive, like "configuration_validation_error" (a better name and more condensed is good).
This keeps the protocol well-documented and makes it trivial to introduce other error types. You can also choose to have a subtype if you prefer. Assigning 'error' to mean configuration validation only errors is also possible in the future, so this is optional. Nevertheless, it's a good code of hygiene.

[flrgh]

To clarify, "error" here is just a top-level classifier, and the .error.name attribute holds one of several well-defined, descriptive error labels that I've defined in the kong.constants.CLUSTERING_DATA_PLANE_ERROR Lua table. Example:

{
  "type": "error",
  "error": {
    "name": "declarative configuration parse failure",
    "message": "invalid entity 'foo' blah blah blah blah..."
  }
}

This is just what "felt right" to me when writing the code, so happy to entertain changes.

[GGabriele]

@flrgh To help clarifying, are the following assertions correct?

• in case of multiple errors, we expect the DP to send multiple error messages
• at any time, a payload would generate at most one error per type (e.g. one invalid declarative configuration, one transient error and one exception)

[RobSerafini]

@GGabriele @flrgh - Do you guys need any help here? We are 1 week from feature freeze.

[flrgh]

Do you guys need any help here? We are 1 week from feature freeze.

@RobSerafini aside from needing gateway team code review (which can be sorted with tomorrows PR meeting), I need a product/manager decision on whether or not this is a pure OSS feature or a Konnect-only feature.

[flrgh]

* in case of multiple errors, we expect the DP to send multiple error messages

* at any time, a payload would generate at most one error per type (e.g. one `invalid declarative configuration`, one `transient error` and one `exception`)

@GGabriele I think I haven't done the best job of laying out specifications with examples, and "error" as a term is pretty generic, so allow me to expand a little and see if this answers your questions...

Let's call the final JSON-encoded, binary WebSocket message that is sent to the CP the error payload.

An error payload is a JSON object with a top-level classification of type: "error" and an object in the error attribute:

{
  "type": "error",
  "error": {
    "name":        "my error name",
    "config_hash": "588b0c0baa505a71be9369608e8b75f1",

    ...other attributes specific to error.name also live here
  }
}

...where error.name describes the type of error that was recorded and includes exactly one of (currently 3) different strings that are being added to kong/constants.lua:

+  CLUSTERING_DATA_PLANE_ERROR = {
+    CONFIG_PARSE     = "declarative configuration parse failure",
+    RELOAD           = "configuration reload failed",
+    GENERIC          = "generic or unknown error",
+  },

Additionally, error.config_hash represents the value of config_hash within the reconfigure message that was received from the CP.

Upon receiving a reconfigure payload from the CP, the DP enacts a serialized, atomic(ish) procedure for handling the payload: decode it, validate it, and then reload runtime state from it. If any one of those steps fails, the process is immediately aborted, and the DP sends an error payload for the CP--the contents of which will depend upon which step yielded an error. Because this is a direct consequence of the DP's receipt of a reconfigure payload from the CP, one message from the CP will result in at most one error payload being sent from the DP.

In the case of CONFIG_PARSE, there will be an array called error.flattened_errors* that contains individual error structures, because a single declarative config may contain more than one invalid entity. So by definition, yes, an error payload can contain more than one "error", but that is just an implementation detail of the declarative validation code and the error structure it returns. Each error contained within error.flattened_errors is to be considered part of the entire CONFIG_PARSE error payload as a unit. Here is complete example of a CONFIG_PARSE error payload

{
  "type": "error",
  "error": {
    "name": "declarative configuration parse failure",
    "config_hash": "ba5fa5346a9f48062895911e5ebcee03",
    "message": "declarative config is invalid: {}",
    "source": "kong.db.declarative.parse_table",
    "code": 14,
    "fields": {},
    "flattened_errors": [
      {
        "entity_id": "3923d481-b21c-4c0e-a91e-e4eb5d63a8f7",
        "entity_name": "my-other-service",
        "entity_tags": [
          "tag-1",
          "tag-2"
        ],
        "entity_type": "service",
        "errors": [
          {
            "field": "host",
            "message": "required field missing",
            "type": "field"
          },
          {
            "field": "extra_field",
            "message": "unknown field",
            "type": "field"
          }
        ]
      },
      {
        "entity_id": "908e55c2-0b83-4c47-b291-eb9c8776fef3",
        "entity_name": "my-service",
        "entity_tags": [
          "tag-1",
          "tag-2"
        ],
        "entity_type": "service",
        "errors": [
          {
            "field": "host",
            "message": "required field missing",
            "type": "field"
          }
        ]
      }
    ]
  }
}

* flattened_errors is not something introduced by this PR. See this older PR for more context on where it comes from

[GGabriele]

@flrgh thanks a lot for the clarification, this is super helpful!!

[flrgh]

Added an additional commit with better test coverage (assert more fields and cover the error() case). Please squash this on merge.

[locao]

Just leaving some comments, I want to take another look before approving.

The custom plugin was a smart idea, way better than a huge refactor.

[locao]

LGTM 🚀

My change suggestion is related to flakiness I saw in the past, I don't know if it is still relevant. Feel free to ignore.

[RobSerafini]

@GGabriele - I would like a final approval from you on behalf of Konnect.

[GGabriele]

[team-gateway-bot]

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-12282-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-12282-to-master-to-upstream
git checkout -b cherry-pick-12282-to-master-to-upstream
ancref=$(git merge-base ccfac55b965c9818955c3422d7cfc4e509dcf922 3ad0280b6e5368c4f4e25a1a014aabd3ea02c931)
git cherry-pick -x $ancref..3ad0280b6e5368c4f4e25a1a014aabd3ea02c931