fix: wrong namespace related endpoint in k8s#10917
Conversation
Signed-off-by: dongjiang1989 <[email protected]>
|
@dongjiang1989 please explain your bugfix, why is it required and also add test cases. Thanks. |
This is url path splicing bug. endpoints api url: /api/v1/ |
|
Could you add test cases to cover this? |
Thanks. I got it! |
|
|
||
| if namespace and namespace ~= "" then | ||
| path = path .. "/namespace/" .. namespace | ||
| path = path .. "/namespaces/" .. namespace |
There was a problem hiding this comment.
why have you made the same change in another PR? #10916
There was a problem hiding this comment.
Hmmm... Just split PR into bugfix and feature
There was a problem hiding this comment.
please remove this change if it is not needed.
There was a problem hiding this comment.
Okay my bad, it seems this is not needed in the newer PR.
| $block->set_value("main_config", $main_config); | ||
|
|
||
| my $config = $block->config // <<_EOC_; | ||
| location /informer { |
There was a problem hiding this comment.
this test case doesn't make sense. It is difficult to understand what problem this PR fixes (although from the link that you shared, it appears that the url is incorrect).. so in theory... namespace based queries should fail (given the path is incorrect)...
can you show in a test case that namespace related queries pass now?
There was a problem hiding this comment.
Thanks for your review. It's difficult to verify in e2e case. Is there an example of unittest case?
There was a problem hiding this comment.
I'm not familar with this plugin, could you explain why the url you changed has anything to do with the document you are referencing?
We can ignore the test cases If they are too difficult
There was a problem hiding this comment.
Just fix Informer_factory.path splicing problem
|
If possible, please also provide some reproduction steps to show how the current state leads to a bug/failure. Thanks. |
|
@zhixiongdu027 Could you help to review this pr? |
| === TEST 12: parse informer_factory information | ||
| --- yaml_config eval: $::yaml_config | ||
| --- request eval | ||
| [ | ||
|
|
||
| "GET /queries | ||
| [ | ||
| \"first/ns-a/ep:p1\",\"first/ns-a/ep:p2\", | ||
| \"second/ns-a/ep:p1\",\"second/ns-a/ep:p2\" | ||
| ]", | ||
|
|
||
| "GET /informer | ||
| []" | ||
|
|
||
| ] | ||
| --- response_body eval | ||
| [ | ||
| "{ 0 0 0 0 }\n", | ||
| "{ 0 }\n", | ||
| ] |
There was a problem hiding this comment.
The test case seems meanless?
There was a problem hiding this comment.
exactly, I struggle to make any sense out of it.
There was a problem hiding this comment.
+1.
The test case should indicate that using a namespace based informator can properly watch objects.
There was a problem hiding this comment.
Will remove it.
| local informer_factory = require("apisix.discovery.kubernetes.informer_factory") | ||
| ngx.sleep(1) | ||
| local response_body = "{" | ||
| local informer, err = informer_factory.new("", "v1", "Endpoints", "endpoints", "default") |
There was a problem hiding this comment.
Informer_factory.new only creates a table object and does not watch resources.
You should call list_watch to attempt to retrieve endpoints resources from apiserver.
There was a problem hiding this comment.
This case just testing Informer_factory.path. e2e case watch apiserver is difficult
| === TEST 12: parse informer_factory information | ||
| --- yaml_config eval: $::yaml_config | ||
| --- request eval | ||
| [ | ||
|
|
||
| "GET /queries | ||
| [ | ||
| \"first/ns-a/ep:p1\",\"first/ns-a/ep:p2\", | ||
| \"second/ns-a/ep:p1\",\"second/ns-a/ep:p2\" | ||
| ]", | ||
|
|
||
| "GET /informer | ||
| []" | ||
|
|
||
| ] | ||
| --- response_body eval | ||
| [ | ||
| "{ 0 0 0 0 }\n", | ||
| "{ 0 }\n", | ||
| ] |
There was a problem hiding this comment.
+1.
The test case should indicate that using a namespace based informator can properly watch objects.
0c6bb82 to
72a23f6
Compare
|
ignore e2e test case |
juststillthinking
changed the title
namespaces in informer factory
|
It's a very little typo, @AlinsRan and i have checked twice, so i think missing test cases is ok |
|
A little weird I want to say, does this mean that before this pr being merged, the apisix k8s discovery function is always broken? |
5 participants
Description
This PR fix url path splicing bug.
endpoints api url: /api/v1/
namespaces/{namespace}/endpoints/{name}ref: https://docs.openshift.com/container-platform/4.8/rest_api/network_apis/endpoints-core-v1.html#apiv1namespacesnamespaceendpointsname
Checklist