fix: wrong namespace related endpoint in k8s #10917

dongjiang1989 · 2024-02-05T13:52:37Z

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

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: dongjiang1989 <dongjiang1989@126.com>

shreemaan-abhishek · 2024-02-06T04:54:54Z

@dongjiang1989 please explain your bugfix, why is it required and also add test cases. Thanks.

dongjiang1989 · 2024-02-06T08:16:27Z

@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/namespaces/{namespace}/endpoints/{name}
ref: https://docs.openshift.com/container-platform/4.8/rest_api/network_apis/endpoints-core-v1.html#apiv1namespacesnamespaceendpointsname

monkeyDluffy6017 · 2024-02-06T09:55:55Z

Could you add test cases to cover this?

monkeyDluffy6017 · 2024-02-07T10:12:33Z

@dongjiang1989 you could follow these test cases: https://github.com/apache/apisix/blob/master/t/kubernetes/discovery/kubernetes2.t

dongjiang1989 · 2024-02-08T01:07:47Z

@dongjiang1989 you could follow these test cases: https://github.com/apache/apisix/blob/master/t/kubernetes/discovery/kubernetes2.t

Thanks. I got it!

shreemaan-abhishek · 2024-02-08T16:37:48Z

apisix/discovery/kubernetes/informer_factory.lua

@@ -355,7 +355,7 @@ function _M.new(group, version, kind, plural, namespace)
    end

    if namespace and namespace ~= "" then
-        path = path .. "/namespace/" .. namespace
+        path = path .. "/namespaces/" .. namespace


why have you made the same change in another PR? #10916

Hmmm... Just split PR into bugfix and feature

please remove this change if it is not needed.

Okay my bad, it seems this is not needed in the newer PR.

shreemaan-abhishek · 2024-02-10T17:28:34Z

t/kubernetes/discovery/kubernetes2.t

@@ -99,6 +99,21 @@ _EOC_
    $block->set_value("main_config", $main_config);

    my $config = $block->config // <<_EOC_;
+        location /informer {


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?

Thanks for your review. It's difficult to verify in e2e case. Is there an example of unittest case?

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

Just fix Informer_factory.path splicing problem

shreemaan-abhishek · 2024-02-10T17:30:36Z

If possible, please also provide some reproduction steps to show how the current state leads to a bug/failure. Thanks.

monkeyDluffy6017 · 2024-02-18T03:37:59Z

@zhixiongdu027 Could you help to review this pr?

monkeyDluffy6017 · 2024-02-18T03:54:12Z

t/kubernetes/discovery/kubernetes2.t

+=== 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",
+]


The test case seems meanless?

exactly, I struggle to make any sense out of it.

+1.
The test case should indicate that using a namespace based informator can properly watch objects.

Will remove it.

AlinsRan · 2024-02-18T08:57:57Z

t/kubernetes/discovery/kubernetes2.t

+              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")


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.

This case just testing Informer_factory.path. e2e case watch apiserver is difficult

AlinsRan · 2024-02-18T09:00:25Z

t/kubernetes/discovery/kubernetes2.t

+=== 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",
+]


+1.
The test case should indicate that using a namespace based informator can properly watch objects.

dongjiang1989 · 2024-02-20T01:49:32Z

ignore e2e test case

monkeyDluffy6017 · 2024-02-22T06:39:15Z

It's a very little typo, @AlinsRan and i have checked twice, so i think missing test cases is ok

caibirdme · 2024-07-15T15:30:47Z

A little weird I want to say, does this mean that before this pr being merged, the apisix k8s discovery function is always broken?

fix informer factory namespaces bug

72a23f6

Signed-off-by: dongjiang1989 <dongjiang1989@126.com>

shreemaan-abhishek added the need test cases label Feb 7, 2024

github-actions bot added the user responded label Feb 8, 2024

shreemaan-abhishek reviewed Feb 8, 2024

View reviewed changes

shreemaan-abhishek removed need test cases user responded labels Feb 9, 2024

shreemaan-abhishek reviewed Feb 10, 2024

View reviewed changes

monkeyDluffy6017 reviewed Feb 18, 2024

View reviewed changes

monkeyDluffy6017 added the discuss label Feb 18, 2024

AlinsRan reviewed Feb 18, 2024

View reviewed changes

dongjiang1989 force-pushed the fixbug branch from 0c6bb82 to 72a23f6 Compare February 20, 2024 01:12

github-actions bot added the user responded label Feb 20, 2024

monkeyDluffy6017 requested a review from AlinsRan February 20, 2024 06:47

monkeyDluffy6017 changed the title ~~fix: fix namespaces in informer factory~~ fix: wrong namespace related endpoint in k8s Feb 22, 2024

monkeyDluffy6017 approved these changes Feb 22, 2024

View reviewed changes

AlinsRan approved these changes Feb 22, 2024

View reviewed changes

monkeyDluffy6017 merged commit 72f6364 into apache:master Feb 22, 2024
85 of 89 checks passed

dongjiang1989 deleted the fixbug branch February 22, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wrong namespace related endpoint in k8s #10917

fix: wrong namespace related endpoint in k8s #10917

dongjiang1989 commented Feb 5, 2024 •

edited

Loading

shreemaan-abhishek commented Feb 6, 2024

dongjiang1989 commented Feb 6, 2024 •

edited

Loading

monkeyDluffy6017 commented Feb 6, 2024

monkeyDluffy6017 commented Feb 7, 2024

dongjiang1989 commented Feb 8, 2024

shreemaan-abhishek Feb 8, 2024

dongjiang1989 Feb 8, 2024

shreemaan-abhishek Feb 9, 2024

shreemaan-abhishek Feb 9, 2024

shreemaan-abhishek Feb 10, 2024

dongjiang1989 Feb 18, 2024

monkeyDluffy6017 Feb 18, 2024

dongjiang1989 Feb 19, 2024

shreemaan-abhishek commented Feb 10, 2024

monkeyDluffy6017 commented Feb 18, 2024

monkeyDluffy6017 Feb 18, 2024

shreemaan-abhishek Feb 18, 2024

AlinsRan Feb 18, 2024

dongjiang1989 Feb 19, 2024

AlinsRan Feb 18, 2024

dongjiang1989 Feb 19, 2024

AlinsRan Feb 18, 2024

dongjiang1989 commented Feb 20, 2024

monkeyDluffy6017 commented Feb 22, 2024

caibirdme commented Jul 15, 2024

fix: wrong namespace related endpoint in k8s #10917

fix: wrong namespace related endpoint in k8s #10917

Conversation

dongjiang1989 commented Feb 5, 2024 • edited Loading

Description

Checklist

shreemaan-abhishek commented Feb 6, 2024

dongjiang1989 commented Feb 6, 2024 • edited Loading

monkeyDluffy6017 commented Feb 6, 2024

monkeyDluffy6017 commented Feb 7, 2024

dongjiang1989 commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreemaan-abhishek commented Feb 10, 2024

monkeyDluffy6017 commented Feb 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjiang1989 commented Feb 20, 2024

monkeyDluffy6017 commented Feb 22, 2024

caibirdme commented Jul 15, 2024

dongjiang1989 commented Feb 5, 2024 •

edited

Loading

dongjiang1989 commented Feb 6, 2024 •

edited

Loading