Skip to content

Conversation

@membphis
Copy link
Member

@membphis membphis commented Sep 3, 2020

What this PR does / why we need it:

If the administrator user creates and deletes the same route multiple times, the new route will not be correctly synchronized to the APISIX gateway node, resulting in some request route matching failures and a 404 response.

bugfix: clear all cached data when get delete action, and removing stale boolean data in table self.values with a safer way

it is not easy to add test case. will add later

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@membphis
Copy link
Member Author

membphis commented Sep 4, 2020

we'll get the wrong route list in APISIX by this case:

ETCDCTL_API=2 etcdctl rm -r /apisix && make init

make run

curl -i http://127.0.0.1:9080/apisix/admin/routes/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "uri": "/route",
    "plugins": {
        "serverless-pre-function": {
            "phase": "rewrite",
            "functions" : ["return function() local router = require(\"apisix.http.router.radixtree_host_uri\")\nlocal routes, routes_ver = router.routes()\n\nlocal core = require(\"apisix.core\")\nngx.say(\"worker_pid: \", ngx.worker.pid())\nngx.say(\"route_ver: \", routes_ver)\nngx.say(\"routes_type: \", type(routes))\n\nlocal keys = {}\nfor key in pairs(routes) do\n    table.insert(keys, key)\nend\n\ntable.sort(keys)\n\nfor _, key in ipairs(keys) do\n    ngx.say(\"--> key: \", key)\n    ngx.say(\"--> val: \", core.json.encode(routes[key], true))\nend\n end"]
        }
    }
}'

curl http://127.0.0.1:9080/route -i

# 200 routes
bash -x create && bash -x delete && bash -x create

curl http://127.0.0.1:9080/route -i

make stop

@membphis membphis changed the title bugfix: it is unsafe to get the length of array if it contains any `n… bugfix: clear all cached data when get delete action, and removing stale boolean data in table self.values with a safer way Sep 4, 2020
@membphis
Copy link
Member Author

membphis commented Sep 4, 2020

@apache/apisix-committers please take a look at this PR, I updated the title and reason right now

@marcoabreu
Copy link

Please close this PR and lock the conversation. You have just pinged hundreds of Apache people...

@kamaci
Copy link
Member

kamaci commented Sep 4, 2020

Hi @membphis,

No need to mention apache committers since it notifies all the committers of all ASF projects.

@rmetzger
Copy link

rmetzger commented Sep 4, 2020

to be precise, 3301 people.

@membphis
Copy link
Member Author

membphis commented Sep 4, 2020

Hi @membphis,

No need to mention apache committers since it notifies all the committers of all ASF projects.

sorry for this, I chose wong group. will take care later

@mxm
Copy link

mxm commented Sep 4, 2020

Not really @membphis fault. I think this is a configuration issue. Notifying all committers should only be allowed by the admins of the Apache GitHub organization.

@moonming moonming closed this Sep 4, 2020
@apache apache locked as off-topic and limited conversation to collaborators Sep 4, 2020
@gstein
Copy link
Member

gstein commented Sep 4, 2020

@mxm that is not an available configuration option, unfortunately 😥 (believe me: we'd do that in a heartbeat)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants