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: support sni based tls route #1051

Merged
merged 7 commits into from
Nov 4, 2022

Conversation

mangoGoForward
Copy link
Contributor

Signed-off-by: mango xu.weiKyrie@foxmail.com

Type of change:

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

What this PR does / why we need it:

Please see #547

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 May 27, 2022

Codecov Report

Merging #1051 (dd48936) into master (e51a2c7) will increase coverage by 0.80%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
+ Coverage   40.62%   41.42%   +0.80%     
==========================================
  Files          75       82       +7     
  Lines        7006     7234     +228     
==========================================
+ Hits         2846     2997     +151     
- Misses       3847     3891      +44     
- Partials      313      346      +33     
Impacted Files Coverage Δ
pkg/providers/apisix/translation/apisix_route.go 31.42% <50.00%> (+12.28%) ⬆️
pkg/apisix/stream_route.go 36.42% <100.00%> (ø)
...kg/providers/apisix/translation/apisix_upstream.go 55.26% <0.00%> (-44.74%) ⬇️
pkg/apisix/resource.go 56.79% <0.00%> (-6.23%) ⬇️
pkg/providers/utils/manifest.go 42.79% <0.00%> (-2.83%) ⬇️
pkg/config/config.go 64.13% <0.00%> (-2.22%) ⬇️
pkg/apisix/nonexistentclient.go 41.20% <0.00%> (-2.07%) ⬇️
pkg/apisix/ssl.go 35.91% <0.00%> (-0.45%) ⬇️
pkg/apisix/consumer.go 36.30% <0.00%> (-0.44%) ⬇️
pkg/apisix/global_rule.go 36.05% <0.00%> (-0.44%) ⬇️
... and 33 more

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

@tokers
Copy link
Contributor

tokers commented May 30, 2022

Copy link
Contributor

@tokers tokers left a comment

Choose a reason for hiding this comment

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

Please also add some e2e test cases to cover the SNI route feature.

@mangoGoForward
Copy link
Contributor Author

@mangoGoForward Please make the CI pass, thanks! See https://github.com/apache/apisix-ingress-controller/runs/6623221714?check_suite_focus=true.

I got some connection refused errors, this feature seems wouldn't effect the CI?

@mangoGoForward
Copy link
Contributor Author

Please also add some e2e test cases to cover the SNI route feature.

Thanks, I will do.

@mangoGoForward
Copy link
Contributor Author

Hi @tokers How can I generate a crd? The Makefile seems has no related command.

@tao12345666333
Copy link
Member

n I generate a crd? Th

@mangoGoForward Unfortunately, current CRDs are manually maintained. We have #515 to track it.

Signed-off-by: mango <xu.weiKyrie@foxmail.com>
@tao12345666333
Copy link
Member

Please merge master latest code, and resolve conflicts. Thanks

@tao12345666333 tao12345666333 self-assigned this Jun 1, 2022
Signed-off-by: mango <xu.weiKyrie@foxmail.com>

# Conflicts:
#	test/e2e/suite-features/global_rule.go
#	test/e2e/suite-ingress/ingress.go
@mangoGoForward
Copy link
Contributor Author

Please merge master latest code, and resolve conflicts. Thanks

OK. I will solve it later.

Signed-off-by: mango <xu.weiKyrie@foxmail.com>
@tokers
Copy link
Contributor

tokers commented Jun 2, 2022

@mangoGoForward Please make the CI passed, it has some failures. Thanks!

@mangoGoForward
Copy link
Contributor Author

@mangoGoForward Please make the CI passed, it has some failures. Thanks!

OK, I haven't found the the target of this error, it may takes a while to resolve.

@tao12345666333
Copy link
Member

@mangoGoForward hi, thanks for your contribution, we plan to enter v1.5 release window.

I have a few suggestions for this PR

  • I would like to put this PR into v1.6
  • Currently we have unified all resources to the v2 version of the API, I recommend implementing this feature only for the v2 version of ApisixRoute. Because when the v1.5 version is released, we will mark the v2beta3 version as deprecated. You can check proposal: use a uniform CRD APIVersion #707 for more detail

@tao12345666333 tao12345666333 added the enhancement New feature or request label Jul 27, 2022
@tao12345666333 tao12345666333 added this to the v1.6.0 milestone Jul 27, 2022
@tao12345666333 tao12345666333 linked an issue Jul 27, 2022 that may be closed by this pull request
@tao12345666333
Copy link
Member

Hi, do you have time to pick up this one? Thanks

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

# Conflicts:
#	pkg/providers/apisix/translation/apisix_route.go
#	test/e2e/suite-ingress/suite-ingress-resource/stream.go
@mangoGoForward
Copy link
Contributor Author

Hi, do you have time to pick up this one? Thanks

Yes, the file conflicts solved

@tao12345666333
Copy link
Member

tao12345666333 commented Sep 23, 2022

@mangoGoForward Thanks for your active contribution!
As I said, we can implement this feature into the v2 API version.

@mangoGoForward hi, thanks for your contribution, we plan to enter v1.5 release window.

I have a few suggestions for this PR

  • I would like to put this PR into v1.6
  • Currently we have unified all resources to the v2 version of the API, I recommend implementing this feature only for the v2 version of ApisixRoute. Because when the v1.5 version is released, we will mark the v2beta3 version as deprecated. You can check proposal: use a uniform CRD APIVersion #707 for more detail

samples/deploy/crd/v1/ApisixRoute.yaml Outdated Show resolved Hide resolved
pkg/kube/apisix/apis/config/v2beta3/types.go Outdated Show resolved Hide resolved
Signed-off-by: mango <xu.weiKyrie@foxmail.com>
Signed-off-by: mango <xu.weiKyrie@foxmail.com>
@tao12345666333
Copy link
Member

LGTM !

could you please add e2e test cases to covet this one? thanks!

@mangoGoForward
Copy link
Contributor Author

LGTM !

could you please add e2e test cases to covet this one? thanks!

Sure, but have no time to deal with it recently, I will improve it as soon as possible

@tao12345666333
Copy link
Member

Now that we're in the v1.6 development cycle, do you have time to add e2e test cases to covet this one? Thanks!

@mangoGoForward
Copy link
Contributor Author

Now that we're in the v1.6 development cycle, do you have time to add e2e test cases to covet this one? Thanks!

Sorry reply to late, will be completed this week

@tao12345666333
Copy link
Member

Thanks! @mangoGoForward

Signed-off-by: mango <xu.weiKyrie@foxmail.com>
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

But I think a more complete case can be added in e2e.
Create a certificate pair and use it for proxying.

@tao12345666333
Copy link
Member

But we can do it in a follow-up PR #1438 @mangoGoForward

@tao12345666333 tao12345666333 merged commit 38b12fb into apache:master Nov 4, 2022
@mangoGoForward
Copy link
Contributor Author

LGTM

But I think a more complete case can be added in e2e. Create a certificate pair and use it for proxying.

Thanks, it absolutely need more test cases to cover it. I need to study how the TLS route with SNI works and how to test it, I am so poor about knowledge in this area.

@mangoGoForward mangoGoForward deleted the feat/support-sni branch November 4, 2022 09:16
@tao12345666333
Copy link
Member

Don't worry, you can continue if you are interested. Otherwise see if others are interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

feat: Support SNI based TLS Route
5 participants