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

Proposal: Support Custom CA Certificate for Karmada Instance in Operator #5794

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jabellard
Copy link
Contributor

What type of PR is this?

/kind design

What this PR does / why we need it:

  • Details are contained in the proposal.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Joe Nathan Abellard <[email protected]>
@karmada-bot karmada-bot added the kind/design Categorizes issue or PR as related to design. label Nov 8, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xishanyongye-chang for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2024
Signed-off-by: Joe Nathan Abellard <[email protected]>
Signed-off-by: Joe Nathan Abellard <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.18%. Comparing base (0cc294f) to head (647fd4a).
Report is 16 commits behind head on master.

❗ 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     
Flag Coverage Δ
unittests 43.18% <ø> (+0.77%) ⬆️

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.

Copy link
Member

@RainbowMango RainbowMango left a 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]>
@jabellard
Copy link
Contributor Author

@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.

Comment on lines +79 to +85
// +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"`
Copy link
Contributor

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?

Copy link
Contributor Author

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)
                
                ...

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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???

Copy link
Contributor Author

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]>
@jabellard
Copy link
Contributor Author

@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.

@RainbowMango
Copy link
Member

It's still in my queue, I will get back on this ASAP.

Looks like they're some overlap in the work that they do.

I contributed a lot to that SIG, what kind of work overlap you are talking about?

@jabellard
Copy link
Contributor Author

It's still in my queue, I will get back on this ASAP.

Looks like they're some overlap in the work that they do.

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 Work API. That's awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants