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 TLS connection with etcd. #2548

Merged
merged 5 commits into from
Nov 3, 2020

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Oct 28, 2020

What this PR does / why we need it:

Support the TLS connection when communicating with etcd cluster. We added a configuration item to custom the certificate verification. Whether to setup TLS connection or not depends on the endpoints' scheme, for instance, when endpoints are:

etcd:
  host:
    - "https://127.0.0.1:2379"
    - "https://127.0.0.1:3379"

APISIX will originate TLS connection automatically, and the Server Name Indication extention will be set by the endpoint host (127.0.0.1 in above case). Note by default APISIX will verify the certificate, close the verification in configuration explicitly if you want to bypass it.

etcd:
  tls:
    verfiy: false

Commits will be rebased after the changes are LGTM.

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?

@membphis
Copy link
Member

missing test case ^_^

@tokers
Copy link
Contributor Author

tokers commented Oct 28, 2020 via email

@Miss-you
Copy link
Member

Miss-you commented Oct 29, 2020

I think we can also use the etcd proxy localily as an interim solution

[Apache APISIX -> etcd proxy(cert) ]--http mTLS-->[etcd]

@moonming
Copy link
Member

I think we can also use the etcd proxy localily as an interim solution

[Apache APISIX -> etcd proxy(cert) ]--http mTLS-->[etcd]

Do you means add sidecar for etcd proxy(cert)?

@tokers tokers force-pushed the feature/etcd_tls_support branch 2 times, most recently from 8911931 to 2084efa Compare October 30, 2020 02:52
@tokers
Copy link
Contributor Author

tokers commented Oct 30, 2020

I think we can also use the etcd proxy localily as an interim solution
[Apache APISIX -> etcd proxy(cert) ]--http mTLS-->[etcd]

Do you means add sidecar for etcd proxy(cert)?

I think not the sidecar but a "central bus", a bunch of etcd proxy instances to delegate the backend etcd cluster.
Actually it loses the real effect to use client certificate auth, since the proxy can be accessed by any users.

@tokers
Copy link
Contributor Author

tokers commented Nov 2, 2020

Will add test cases after the corresponding pr in lua-resty-etcd is merged.

-- Zhang Chao
On 2020年10月28日 at 22:32:05, YuanSheng Wang @.***) wrote: missing test case ^_^ — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2548 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPR7LPLLIBF5UR7XRALZLDSNATOLANCNFSM4TB6Q56Q .

I have added some test cases.

@juzhiyuan
Copy link
Member

We'd better keep far away from git force push if possible because this action will flush all review histories.

@tokers
Copy link
Contributor Author

tokers commented Nov 2, 2020

We'd better keep far away from git force push if possible because this action will flush all review histories.

OK.

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

we need more test cases.
After enabling TLS, confirm that we can successfully set/get route.

@tokers
Copy link
Contributor Author

tokers commented Nov 2, 2020

we need more test cases.
After enabling TLS, confirm that we can successfully set/get route.

Fair enough.

@tokers
Copy link
Contributor Author

tokers commented Nov 3, 2020

we need more test cases.
After enabling TLS, confirm that we can successfully set/get route.

@membphis Cases supplemented

@membphis membphis merged commit 5191374 into apache:master Nov 3, 2020
@membphis
Copy link
Member

membphis commented Nov 3, 2020

@tokers merged, many thx for your contribution.

@tokers tokers deleted the feature/etcd_tls_support branch November 3, 2020 09:36
@moonming moonming added this to the 2.1 milestone Nov 21, 2020
@moonming moonming mentioned this pull request Nov 29, 2020
4 tasks
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.

6 participants