fix: avoid create pluginconfig in the tranlsation of route (#836) #845

neverCase · 2022-01-25T10:29:01Z

Type of change:

Bugfix
New feature provided
Improve performance
Backport patches

What this PR does / why we need it:

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? If it is not backward compatible, please discuss on the mailing list first

neverCase · 2022-01-25T11:16:38Z

ci need re-run @tao12345666333

tokers

Need some test cases to make sure the plugin_config_id is not written now.

neverCase · 2022-01-26T02:56:07Z

Need some test cases to make sure the plugin_config_id is not written now.

ok, I will add more test cases today.

…onfigName had’n been specified

codecov-commenter · 2022-01-26T03:58:40Z

Codecov Report

Merging #845 (4547a1a) into master (d30bef5) will increase coverage by 0.07%.
The diff coverage is 17.39%.

@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
+ Coverage   32.01%   32.09%   +0.07%     
==========================================
  Files          70       70              
  Lines        7749     7730      -19     
==========================================
  Hits         2481     2481              
+ Misses       4993     4976      -17     
+ Partials      275      273       -2

Impacted Files	Coverage Δ
pkg/ingress/apisix_route.go	`0.00% <0.00%> (ø)`
pkg/kube/translation/apisix_route.go	`22.43% <100.00%> (+0.60%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d30bef5...4547a1a. Read the comment docs.

neverCase · 2022-01-26T05:21:19Z

e2e test need to re-run @tao12345666333

neverCase · 2022-01-26T10:41:24Z

ci error below:

• Failure [70.707 seconds]
ApisixRoute Testing
/home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/resourcepushing.go:28
  service is referenced by two ApisixRoutes [It]
  /home/runner/work/apisix-ingress-controller/apisix-ingress-controller/test/e2e/ingress/resourcepushing.go:502

  
  	Error Trace:	resourcepushing.go:607
  	            				runner.go:113
  	            				runner.go:64
  	            				it_node.go:26
  	            				spec.go:215
  	            				spec.go:138
  	            				spec_runner.go:200
  	            				spec_runner.go:170
  	            				spec_runner.go:66
  	            				suite.go:79
  	            				ginkgo_dsl.go:238
  	            				ginkgo_dsl.go:213
  	            				e2e_test.go:26
  	Error:      	"[%!s(*v1.Upstream=&{{45e6c8e6 ingress-apisix-e2e-tests-default-428202328_httpbin-service-e2e-test_80 Created by apisix-ingress-controller, DO NOT modify it manually map[managed-by:apisix-ingress-controller]} roundrobin vars  <nil> [{10.244.2.32 80 100}] http <nil> <nil> <nil>})]" should have 0 item(s), but has 1
  	Test:       	ApisixRoute Testing service is referenced by two ApisixRoutes

maybe it need to re-run once again. @tao12345666333

Signed-off-by: Jintao Zhang <[email protected]>

tao12345666333 · 2022-02-10T00:24:30Z

Wait for #859 to fix CI.

tao12345666333 · 2022-02-14T02:06:55Z

#859 has been merged. Re-run the CI.

tao12345666333

LGTM

tao12345666333 · 2022-02-15T00:17:25Z

ping @tokers @gxthrj for review, thanks

tokers · 2022-02-15T08:22:27Z

pkg/kube/translation/apisix_route.go

-				route.PluginConfigId = part.PluginConfigName
-			}
+		if part.PluginConfigName != "" {
+			route.PluginConfigId = part.PluginConfigName


We don't use the plugin config name as the id, see https://github.com/apache/apisix-ingress-controller/blob/master/pkg/kube/translation/apisix_pluginconfig.go#L51 for details.

Yes， This is what needs to be refactored.

ok, I will refactor it right now

…nConfigId

neverCase · 2022-02-18T04:23:52Z

Actually, I found I had covert the plugin config name to plugin id in the apisixRouteController.
https://github.com/apache/apisix-ingress-controller/blob/master/pkg/ingress/apisix_route.go#L240
@tokers

neverCase · 2022-02-18T04:26:03Z

It's been a long time and I almost forgot what I had done

tokers · 2022-02-23T03:07:59Z

pkg/kube/translation/apisix_route.go

@@ -977,13 +964,6 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext,
 		route := apisixv1.NewDefaultRoute()
 		route.Name = apisixv1.ComposeRouteName(ar.Namespace, ar.Name, part.Name)
 		route.ID = id.GenID(route.Name)
-		pluginConfig := apisixv1.NewDefaultPluginConfig()


Why we don't handle the PluginConfigName in this method?

That's because when it creates the ApisixRoute and the field PluginConfigName has been specified, I want to use apisix.Cluster api to check the plugin was whether exist.
And at the same time replace it with the plugin id.
...
Since the method to generate the plugin id is fixed.
I will just remove the replacement and remain checking.

Well, I mean translateHTTPRouteV2beta3NotStrictly is basically same to translateHTTPRouteV2beta3, so here you should also set the plugin config id.

…eplacement

tokers · 2022-02-23T09:21:42Z

pkg/ingress/apisix_route.go

 			if err != nil {
-				log.Errorw("replacePluginNameWithIdIfNotEmptyV2beta3 error:  plugin_config not found",


Should distinguish the error type, not all errors are "not found".

tokers · 2022-02-23T09:23:22Z

pkg/kube/translation/apisix_route.go

@@ -977,13 +964,6 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext,
 		route := apisixv1.NewDefaultRoute()
 		route.Name = apisixv1.ComposeRouteName(ar.Namespace, ar.Name, part.Name)
 		route.ID = id.GenID(route.Name)
-		pluginConfig := apisixv1.NewDefaultPluginConfig()


Well, I mean translateHTTPRouteV2beta3NotStrictly is basically same to translateHTTPRouteV2beta3, so here you should also set the plugin config id.

…onfigId`

neverCase · 2022-02-24T07:44:37Z

@tao12345666333
Ingress case check the ingress lb status is updated run failed.

tao12345666333 · 2022-02-24T07:45:56Z

let me re-run it

gxthrj

LGTM

tao12345666333 · 2022-03-02T12:56:59Z

CI is green. Wait for @tokers approval

fix: aviod create pluginconfig in the tranlsation of route (apache#836)

05dd483

tokers reviewed Jan 25, 2022

View reviewed changes

fix: the Route.PluginConfigId will not be written if the http PluginC…

5aada25

…onfigName had’n been specified

fix: translate route e2e test cases bug

c090ab7

tao12345666333 self-assigned this Jan 26, 2022

tao12345666333 added the bug Something isn't working label Jan 26, 2022

neverCase and others added 2 commits February 8, 2022 19:21

Merge remote-tracking branch 'origin/master' into bug-836

e50920b

chore: rename function name

3905cd7

Signed-off-by: Jintao Zhang <[email protected]>

Merge branch 'master' into bug-836

fa0443e

tao12345666333 approved these changes Feb 15, 2022

View reviewed changes

tokers reviewed Feb 15, 2022

View reviewed changes

bug: apisix shouldn’t use ApisixPluginConfig.Name as the router.Plugi…

6a14979

…nConfigId

Merge branch 'master' into bug-836

8d723a7

tokers reviewed Feb 23, 2022

View reviewed changes

neverCase added 2 commits February 23, 2022 16:39

bug: only check pluginName when creating the ApisixRoute and remove r…

19748ad

…eplacement

Merge branch 'master' into bug-836

1360c6b

tokers changed the title ~~fix: aviod create pluginconfig in the tranlsation of route (#836)~~ fix: avoid create pluginconfig in the tranlsation of route (#836) Feb 23, 2022

tokers reviewed Feb 23, 2022

View reviewed changes

neverCase added 2 commits February 23, 2022 19:41

bug: translateHTTPRouteV2beta3NotStrictly also set the field `PluginC…

4b6cce0

…onfigId`

Merge remote-tracking branch 'origin/master' into bug-836

4547a1a

tao12345666333 assigned lingsamuel Feb 24, 2022

tao12345666333 requested review from tokers and gxthrj March 2, 2022 01:45

gxthrj approved these changes Mar 2, 2022

View reviewed changes

tokers approved these changes Mar 2, 2022

View reviewed changes

tao12345666333 merged commit 5f6a7c1 into apache:master Mar 2, 2022

tao12345666333 linked an issue Mar 2, 2022 that may be closed by this pull request

bug: the tranlsation of route with plugin_config is pathological #836

Closed

tao12345666333 mentioned this pull request Mar 2, 2022

bug: the tranlsation of route with plugin_config is pathological #836

Closed

tao12345666333 mentioned this pull request Mar 30, 2022

request help: How to configure ApisixPluginConfig？ #854

Closed

tao12345666333 pushed a commit to tao12345666333/ingress-controller that referenced this pull request Apr 22, 2022

fix: avoid create pluginconfig in the tranlsation of route (apache#845)

cbcae44

neverCase deleted the bug-836 branch May 18, 2022 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

neverCase commented Jan 25, 2022

neverCase commented Jan 25, 2022

tokers left a comment

neverCase commented Jan 26, 2022

codecov-commenter commented Jan 26, 2022 •

edited

Loading

neverCase commented Jan 26, 2022

neverCase commented Jan 26, 2022

tao12345666333 commented Feb 10, 2022

tao12345666333 commented Feb 14, 2022

tao12345666333 left a comment

tao12345666333 commented Feb 15, 2022

tokers Feb 15, 2022

gxthrj Feb 15, 2022

neverCase Feb 17, 2022

neverCase commented Feb 18, 2022 •

edited

Loading

neverCase commented Feb 18, 2022

tokers Feb 23, 2022

neverCase Feb 23, 2022

tokers Feb 23, 2022

tokers Feb 23, 2022

neverCase Feb 23, 2022

tokers Feb 23, 2022

neverCase commented Feb 24, 2022

tao12345666333 commented Feb 24, 2022

gxthrj left a comment

tao12345666333 commented Mar 2, 2022

		if err != nil {
		log.Errorw("replacePluginNameWithIdIfNotEmptyV2beta3 error: plugin_config not found",

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

Conversation

neverCase commented Jan 25, 2022

Type of change:

What this PR does / why we need it:

Pre-submission checklist:

neverCase commented Jan 25, 2022

tokers left a comment

Choose a reason for hiding this comment

neverCase commented Jan 26, 2022

codecov-commenter commented Jan 26, 2022 • edited Loading

Codecov Report

neverCase commented Jan 26, 2022

neverCase commented Jan 26, 2022

tao12345666333 commented Feb 10, 2022

tao12345666333 commented Feb 14, 2022

tao12345666333 left a comment

Choose a reason for hiding this comment

tao12345666333 commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neverCase commented Feb 18, 2022 • edited Loading

neverCase commented Feb 18, 2022

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

neverCase commented Feb 24, 2022

tao12345666333 commented Feb 24, 2022

gxthrj left a comment

Choose a reason for hiding this comment

tao12345666333 commented Mar 2, 2022

codecov-commenter commented Jan 26, 2022 •

edited

Loading

neverCase commented Feb 18, 2022 •

edited

Loading