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

feat: supports stream route API #2104

Merged
merged 37 commits into from
Nov 4, 2021
Merged

feat: supports stream route API #2104

merged 37 commits into from
Nov 4, 2021

Conversation

Xu-Mj
Copy link
Contributor

@Xu-Mj Xu-Mj commented Aug 30, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

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

What changes will this PR take into?

supports dynamic configuration of stream-route

Related issues

fix/resolve #1986

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

@netlify
Copy link

netlify bot commented Aug 30, 2021

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: 1647f57

🔍 Inspect the deploy log: https://app.netlify.com/sites/apisix-dashboard/deploys/61385b17600b060008573d8b

😎 Browse the preview: https://deploy-preview-2104--apisix-dashboard.netlify.app

Copy link
Contributor

@starsz starsz left a comment

Choose a reason for hiding this comment

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

Good feature.Thank you very much.

And there are still two things that need to be done.

  1. Add the e2e test for it.
  2. Use JSON schema to check the stream route.

We can do it in the next PR.

r.POST("/apisix/admin/stream_routes", wgin.Wraps(h.Create,
wrapper.InputType(reflect.TypeOf(entity.StreamRoute{}))))
r.PUT("/apisix/admin/stream_routes", wgin.Wraps(h.Update,
wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a little confused: how do we create a stream-route , we only support get and delete handler?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use POST to create a stream route. Right?

Refer: http://apisix.apache.org/docs/apisix/admin-api#stream-route

"github.com/stretchr/testify/mock"
"net/http"
"testing"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -18,6 +18,7 @@ package internal

import (
"fmt"
"github.com/apisix/manager-api/internal/handler/stream_route"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@starsz starsz requested a review from nic-chen August 30, 2021 14:50
@Xu-Mj
Copy link
Contributor Author

Xu-Mj commented Aug 30, 2021

Good feature.Thank you very much.

And there are still two things that need to be done.

  1. Add the e2e test for it.

  2. Use JSON schema to check the stream route.

We can do it in the next PR.

OK,got it!

@nic-chen
Copy link
Member

@Xu-Mj
My suggestion is to update the schema and struct first and then submit the APIs (including test cases) in two to three times.

@liuxiran
Copy link
Contributor

@Xu-Mj
My suggestion is to update the schema and struct first and then submit the APIs (including test cases) in two to three times.

Agree with @nic-chen , and we plan to update the schema to latest APISIX 2.9 this week in other pr by @bzp2010 , @Xu-Mj can focus on improve code and test cases, unit test and e2e test,thanks a lot

@Xu-Mj
Copy link
Contributor Author

Xu-Mj commented Sep 1, 2021

@Xu-Mj
My suggestion is to update the schema and struct first and then submit the APIs (including test cases) in two to three times.

ok,got it

@Xu-Mj
Copy link
Contributor Author

Xu-Mj commented Sep 1, 2021

@Xu-Mj
My suggestion is to update the schema and struct first and then submit the APIs (including test cases) in two to three times.

Agree with @nic-chen , and we plan to update the schema to latest APISIX 2.9 this week in other pr by @bzp2010 , @Xu-Mj can focus on improve code and test cases, unit test and e2e test,thanks a lot

ok

@starsz
Copy link
Contributor

starsz commented Sep 2, 2021

@Xu-Mj
My suggestion is to update the schema and struct first and then submit the APIs (including test cases) in two to three times.

Agree with @nic-chen , and we plan to update the schema to latest APISIX 2.9 this week in other pr by @bzp2010 , @Xu-Mj can focus on improve code and test cases, unit test and e2e test,thanks a lot

Got it. 👍

@liuxiran
Copy link
Contributor

liuxiran commented Sep 6, 2021

Hi @Xu-Mj our schema has already updated, see: #2117

and you can merge master to go on your test cases, any question can comment here, thanks a lot

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2021

Codecov Report

Attention: Patch coverage is 72.47706% with 30 lines in your changes missing coverage. Please review.

Project coverage is 69.39%. Comparing base (9f17637) to head (6cdc584).
Report is 189 commits behind head on master.

Files with missing lines Patch % Lines
api/internal/handler/stream_route/stream_route.go 67.53% 16 Missing and 9 partials ⚠️
api/internal/handler/upstream/upstream.go 80.00% 2 Missing and 1 partial ⚠️
api/internal/core/store/storehub.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2104      +/-   ##
==========================================
- Coverage   69.52%   69.39%   -0.14%     
==========================================
  Files         189      190       +1     
  Lines        7275     7381     +106     
  Branches      823      824       +1     
==========================================
+ Hits         5058     5122      +64     
- Misses       1921     1952      +31     
- Partials      296      307      +11     
Flag Coverage Δ
backend-e2e-test 45.69% <31.19%> (-0.52%) ⬇️
backend-e2e-test-ginkgo 50.16% <68.80%> (+0.41%) ⬆️
backend-unit-test 49.13% <10.18%> (-1.26%) ⬇️
frontend-e2e-test 68.13% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@Xu-Mj
Copy link
Contributor Author

Xu-Mj commented Sep 16, 2021

Hi @Xu-Mj our schema has already updated, see: #2117

and you can merge master to go on your test cases, any question can comment here, thanks a lot

Hi,@liuxiran i followed your suggestion,and completed test module. is there still any problem? and how to validate a stream route in apisix?

@bzp2010
Copy link
Contributor

bzp2010 commented Sep 20, 2021

Hi, @nic-chen @LiteSun. Do we need to actually test the stream route created in the dashboard and how we should do it? Do you have any suggestions?

@bzp2010 bzp2010 changed the title feat: supports dynamic configuration of stream-route feat: supports stream route API Oct 19, 2021
api/internal/core/entity/entity.go Outdated Show resolved Hide resolved
r.POST("/apisix/admin/stream_routes", wgin.Wraps(h.Create,
wrapper.InputType(reflect.TypeOf(entity.StreamRoute{}))))
r.PUT("/apisix/admin/stream_routes", wgin.Wraps(h.Update,
wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use POST to create a stream route. Right?

Refer: http://apisix.apache.org/docs/apisix/admin-api#stream-route

api/internal/handler/stream_route/stream_route.go Outdated Show resolved Hide resolved
api/internal/handler/stream_route/stream_route.go Outdated Show resolved Hide resolved
api/test/e2enew/base/base.go Show resolved Hide resolved
@bzp2010
Copy link
Contributor

bzp2010 commented Oct 21, 2021

Hi, @nic-chen @starsz.

We can use POST to create a stream route. Right?

Yes, we can use it.

Better to add it. It's important.

TCP and UDP test case added.

@bzp2010 bzp2010 requested a review from starsz October 25, 2021 02:20
@bzp2010 bzp2010 self-assigned this Nov 4, 2021
@bzp2010 bzp2010 merged commit e53a607 into apache:master Nov 4, 2021
@caibirdme
Copy link

Which version support this feature? I don't see any place to config stream_route in v3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: supports dynamic configuration of stream-route
8 participants