-
Notifications
You must be signed in to change notification settings - Fork 66
✨ Enabled Aggregate API for Virtual Cluster #167
Conversation
christopherhein
left a comment
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.
Once change, otherwise this looks good, thank you for adding support for these.
| - --proxy-client-key-file=/etc/kubernetes/pki/apiserver/tls.key | ||
| - --proxy-client-cert-file=/etc/kubernetes/pki/apiserver/tls.crt |
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.
It's not suggested to use the same CA and thus Cert/Key as the APIServer check out - https://kubernetes.io/docs/tasks/extend-kubernetes/configure-aggregation-layer/#ca-reusage-and-conflicts for more information, we actually already create a new CA/Cert/Key for "front-proxy" which is meant to be used for this purpose - https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/controlplane/nested/controllers/nestedapiserver_controller.go#L228-L246 you'll need to add this to the volume mounts to add support for it.
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.
Done
This PR is mainly for virtualcluster.
|
Did you mean to remove the changes you made in |
@christopherhein for now, yes, I want to use this PR for virtual cluster and then create another PR for nested control plane soon, hope it is OK. |
|
Sounds good, just wanted to make sure. 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christopherhein, gyliu513 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 |
|
@gyliu513 @christopherhein, One more comment even though the PR was merged. |
Good find, @wangjsty can you make sure an issue is filled and @gyliu513 can you do this as well? |
|
@christopherhein Done at #179 |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #166
@christopherhein @Fei-Guo
This PR only covered VirtualCluster but does not cover nested control plane, will fix nested control plane in another PR.
@wangjsty ^^