-
Notifications
You must be signed in to change notification settings - Fork 888
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
Proposal: Support Custom CA Certificate for Karmada Instance in Operator #5794
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Joe Nathan Abellard <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Joe Nathan Abellard <[email protected]>
Signed-off-by: Joe Nathan Abellard <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5794 +/- ##
==========================================
+ Coverage 42.41% 43.18% +0.77%
==========================================
Files 656 656
Lines 55884 55921 +37
==========================================
+ Hits 23705 24152 +447
+ Misses 30659 30212 -447
- Partials 1520 1557 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks great, thank you @jabellard.
Just want to give another look regarding to certificate rotation, we probably need to reserve extension for that.
Let me know your thoughts about rotation feature.
Signed-off-by: Joe Nathan Abellard <[email protected]>
0602866
to
907b4fc
Compare
@RainbowMango , @zhzhuang-zju : Thanks for your comments and questions. Just pushed a change to refine this proposal based on your feedback. Just let me know if you have other comments. |
// +kubebuilder:validation:Enum=Auto;Custom | ||
// +kubebuilder:default=Auto | ||
Mode CertificateMode `json:"mode,omitempty"` | ||
|
||
// Custom contains the configuration for user-provided certificates when in Custom mode. | ||
// +optional | ||
Custom *CustomCertConfig `json:"custom,omitempty"` |
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.
Just a question, if the CA that can be customized is not limited to KarmadaCertRootCA
and the API also allows customization of FrontProxyCA
, how should the Mode
be used in conjunction with Custom
? For example, in a scenario where KarmadaCertRootCA
needs to be customized, but FrontProxyCA
does not need to be customized?
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.
@zhzhuang-zju , good question. Given that the front proxy CA is used exclusively for in-cluster communication, I don't have a use case for customizing it, so right now, the plan is to add support only for customizing the Root/Kubernetes CA. However, if a use case for customizing it does arise, then we can easily add support for. It's a completely independent, self-singed CA:
Certificate:
Data:
Version: 3 (0x2)
Serial Number: 4436933017513869186 (0x3d932712511dbf82)
Signature Algorithm: sha256WithRSAEncryption
Issuer: CN=front-proxy-ca
Validity
Not Before: Nov 2 17:08:48 2024 GMT
Not After : Oct 31 17:08:48 2034 GMT
Subject: CN=front-proxy-ca
Subject Public Key Info:
Public Key Algorithm: rsaEncryption
Public-Key: (3072 bit)
...
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.
Thanks for the explanation. It makes sense. As long as the extensibility is ensured, we can consider supporting more certificate configs when there are relevant use cases in the future.
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.
@RainbowMango , @zhzhuang-zju , any more questions/comments?
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.
Will make name of CA cert more specific. Maybe KarmadaRootCACert
???
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.
Add optional
tag to secret ref for Karmada root CA cert as it is optional.
Signed-off-by: Joe Nathan Abellard <[email protected]>
@RainbowMango , I pushed changes. Any other comments? Also, out of curiosity, is any of the Karmada maintainers part of the multi cluster SIG? I'm currently at KubeCon and have been talking to some of the members of that group. Looks like they're some overlap in the work that they do. |
It's still in my queue, I will get back on this ASAP.
I contributed a lot to that SIG, what kind of work overlap you are talking about? |
Yeah. I was wondering if you've contributed to that SIG and just found out you've contributed to the |
What type of PR is this?
/kind design
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: