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

Exception Handling #56

Open
washcroft opened this issue Oct 7, 2022 · 1 comment
Open

Exception Handling #56

washcroft opened this issue Oct 7, 2022 · 1 comment

Comments

@washcroft
Copy link

washcroft commented Oct 7, 2022

Hello,
What is the intended behaviour of a plugin raising an exception?

e.g.

  "plugins": {
    "ext-plugin-pre-req": {
      "allow_degradation": false,
      "conf": [
        {
          "name": "netsuite-authenticator",
          "value": "this is invalid json config"
        }
       ]
      }
    }
    def config(self, conf: Any) -> Any:
        """
        Parse plugin configuration
        :param conf:
        :return:
        """
        try:
            conf_json = json.loads(conf)
        except ValueError as e:
            raise Exception("Configuration is invalid json")

        return conf_json
2022-10-07 07:11:42,257 - INFO - request type:2, len:1792
2022-10-07 07:11:42,260 - ERROR - execute plugin `netsuite-authenticator` AnyError, ('Configuration is invalid json',)
2022-10-07 07:11:42,260 - INFO - response type:2, len:16

However apisix ignores the plugin and continues with the request to the upstream. This same occurs even if the exception is thrown in the filter() method.

I would prefer the ability to control this behaviour and I was expecting allow_degredation in apisix config to control the behaviour, but it seems this only apply to the plugin runner process itself, not errors from the plugin.

In some cases I also see the following errors immediately after the starting the plugin in dev mode due to the config token caching mechanism.

2022-10-07 07:37:13,104 - INFO - request type:2, len:1792
2022-10-07 07:37:13,106 - ERROR - token `1` cache acquisition failed
2022-10-07 07:37:13,106 - INFO - response type:2, len:16

Again, apisix ignores the plugin and continues with the request to the upstream, but I would rather it didn't.

I believe this is happening because the plugin runner is always returning a successful RPC response, but the "action" in the response isn't set to either stop or rewrite, so apisix just assumes the plugin didn't want to do anything.

@SkyeYoung
Copy link
Member

SkyeYoung commented Oct 8, 2022

Hi, I checked the logic of the code. Let me try to answer these questions for you.


apisix ignores the plugin and continues with the request to the upstream

In python-plugin-runner.
  1. https://github.com/apache/apisix-python-plugin-runner/blob/master/apisix/runner/plugin/core.py#L92-L94

    image
  2. https://github.com/apache/apisix-python-plugin-runner/blob/master/apisix/runner/server/handle.py#L83-L87

    image
In apisix/plugins/ext-plugin.
  1. https://github.com/apache/apisix/blob/master/apisix/plugins/ext-plugin/init.lua#L566

    image

Since python-plugin-runner does not modify ty's original type RPC_HTTP_REQ_CALL, apisix ignores the error, which seems to be an implementation problem with python-plugin-runner. 🤔

Also, errors throwed by config() are only logged in the execute() of python-plugin-runner, but are not passed to the function that calls execute(), and so are not passed to apisix.


allow_degredation

image

If the ty type problem described above can be fixed, it should be possible to get some errors here so that the allow_degradation should behave appropriately. 🤔

I'm not familiar enough with the code. 😅 @bzp2010 Pls help me check this.

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

No branches or pull requests

2 participants