-
Notifications
You must be signed in to change notification settings - Fork 891
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
Adding TLS Certificate Authentication to gRPC #5040
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5040 +/- ##
=======================================
Coverage 28.22% 28.23%
=======================================
Files 632 632
Lines 43431 43517 +86
=======================================
+ Hits 12258 12286 +28
- Misses 30278 30336 +58
Partials 895 895
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Please rebase the code to address the failing tests. |
314461b
to
3bce736
Compare
/retest |
ab7f741
to
7bd0bcf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, only some nits.
// ClientCertAuth when this is set, server will check all incoming HTTPS requests for a client certificate signed by the trusted CA, | ||
// requests that don’t supply a valid client certificate will fail. If authentication is enabled, | ||
// the certificate provides credentials for the user name given by the Common Name field. | ||
ClientCertAuth bool | ||
// CertFile the certificate used for SSL/TLS connections. | ||
CertFile string | ||
// KeyFile the key for the certificate. | ||
KeyFile string | ||
// TrustedCAFile Trusted certificate authority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable naming and comments.
bb42ed4
to
6cf427c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
I updated the release notes by the way.
6cf427c
to
ed1bb8d
Compare
Signed-off-by: zhzhuang-zju <m17799853869@163.com>
ed1bb8d
to
acdd211
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
For now, the grpc connection between
karmada-scheduler-estimator
andkarmada-scheduler/karmada-descheduler
is insecure by default, and does not provide a way to config TLS Certificate Authentication. So I would like to introduce the ability to authenticate grpc tls certificates, including:---client-cert-auth=true
)Which issue(s) this PR fixes:
Parts of #5041
Special notes for your reviewer:
Does this PR introduce a user-facing change?: