open-cluster-management-io / registration

hub / spoke registration controllers
Apache License 2.0
42 stars 58 forks source link

add expirationSeconds in CSROption to create CSR #312

Closed youhangwang closed 1 year ago

youhangwang commented 1 year ago

https://github.com/open-cluster-management-io/registration/issues/311

skeeey commented 1 year ago

@youhangwang thanks for your contributions!

Would you also help to add an option in https://github.com/open-cluster-management-io/registration/blob/main/pkg/spoke/spokeagent.go#L56

and expose the option with flags https://github.com/open-cluster-management-io/registration/blob/main/pkg/spoke/spokeagent.go#L410

youhangwang commented 1 year ago

yes, sure. Thanks for the review @skeeey

please help to review the flag name.

skeeey commented 1 year ago

/approve

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skeeey, youhangwang

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: - ~~[OWNERS](https://github.com/open-cluster-management-io/registration/blob/main/OWNERS)~~ [skeeey] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
skeeey commented 1 year ago

LGTM

@youhangwang we need a sign-off for your commits

/cc @qiujian16 please also take a look

qiujian16 commented 1 year ago

/lgtm

Thanks for your contribution. @skeeey we need to also think how to expose these flags from operator API, same as --approve-user-list flag.

youhangwang commented 1 year ago

@skeeey @qiujian16

The minimum valid value for expirationSeconds in CSR spec is 600.

But for the clientCertificateController in registration, 600 is too short:

  1. clientCertificateController to check if cert is expired every 5min.
  2. kubeclient resync the client cert every 5min.
  3. clientCertificateController will refresh the cert if the cert has less than a random percentage range from 20% to 25% of its life remaining.

so the minimum expiration second would be (5 * 60)/(1/5) = 1500s for clientCertificateController. otherwise, the kube client will use a expired client cert to send request to the remote cluster.

Considering to add some margin. 1h(3600s) could be a good choice. Any thoughts on this?

skeeey commented 1 year ago

@youhangwang you are right, I forget the cert renew time,  I think we need increase the min expired time

youhangwang commented 1 year ago

https://github.com/open-cluster-management-io/registration/pull/320