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

fix: allow passing plugin config name for route with no backends #1578

Conversation

ikatlinsky
Copy link
Contributor

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

This PR fixes the issue of ignoring passed PluginConfigName in case ApisixRoute definition contains only upstreams and not backends configuration.

The issue is described here in detail - #1577.

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 3, 2023

Codecov Report

Merging #1578 (a6668b5) into master (486b46a) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1578      +/-   ##
==========================================
+ Coverage   41.44%   41.52%   +0.08%     
==========================================
  Files          87       87              
  Lines        7420     7420              
==========================================
+ Hits         3075     3081       +6     
+ Misses       3990     3986       -4     
+ Partials      355      353       -2     
Impacted Files Coverage Δ
pkg/providers/apisix/translation/apisix_route.go 31.21% <100.00%> (+0.81%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AlinsRan
Copy link
Contributor

AlinsRan commented Jan 3, 2023

Please add relevant e2e test cases

@tao12345666333 tao12345666333 added bug Something isn't working area/controller labels Jan 3, 2023
@tao12345666333 tao12345666333 added this to the v1.7.0 milestone Jan 3, 2023
@tao12345666333
Copy link
Member

As @AlinsRan said, you can use the use case you described in #1577 as an e2e test case to easily verify that this function works as expected. Thanks

@tao12345666333
Copy link
Member

When this PR is merged, I will cherry-pick it into v1.6.1

@ikatlinsky
Copy link
Contributor Author

As @AlinsRan said, you can use the use case you described in #1577 as an e2e test case to easily verify that this function works as expected. Thanks

yep, just trying to configure the environment locally for e2e tests, have a couple of issues to solve with existing tests, like following one

Begin Captured StdOut/StdErr Output >>
    2023-01-03T10:10:21+01:00   error   apisix/route.go:118     failed to list routes: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:21+01:00   error   apisix/cluster.go:222   failed to list routes in APISIX: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:23+01:00   error   apisix/route.go:118     failed to list routes: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:23+01:00   error   apisix/cluster.go:222   failed to list routes in APISIX: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:25+01:00   error   apisix/route.go:118     failed to list routes: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:25+01:00   error   apisix/cluster.go:222   failed to list routes in APISIX: Get "http://localhost:31358/apisix/admin/routes": dial tcp [::1]:31358: connect: connection refused
    2023-01-03T10:10:25+01:00   error   apisix/cluster.go:182   failed to sync cache    {"cost_time": "8.016621029s", "cluster": ""}
  << End Captured StdOut/StdErr Output

@ikatlinsky
Copy link
Contributor Author

ikatlinsky commented Jan 3, 2023

@tao12345666333 added a new e2e test into suite-plugins-other, hope that this location is ok, make me know if the test should be moved to another suite.

Also, I placed my test directly under the suite-plugins-other: scaffold v2 section, as it uses API which is not available in v2beta3.

@tao12345666333
Copy link
Member

@ikatlinsky good job! This location is fine.

@ikatlinsky
Copy link
Contributor Author

@tao12345666333 is anything else required from my side to push forward this fix for being merged?

thanks

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tao12345666333 tao12345666333 merged commit 32561d0 into apache:master Jan 10, 2023
tao12345666333 pushed a commit to tao12345666333/ingress-controller that referenced this pull request Jan 10, 2023
…che#1578)

Co-authored-by: Katlinsky, Ilya <i.katlinsky@itransition.com>
tao12345666333 added a commit that referenced this pull request Jan 10, 2023
…) (#1594)

Co-authored-by: Jintao Zhang <zhangjintao@apache.org>
Co-authored-by: ikatlinsky <ilya.katlinsky@gmail.com>
Co-authored-by: Katlinsky, Ilya <i.katlinsky@itransition.com>
@ikatlinsky ikatlinsky deleted the feat/fix-plugin-config-name-propagation-for-route-with-upstreams branch January 12, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

bug: PluginConfigId is not assigned to the route in case upstreams used instead of the backends
6 participants