Skip to content

HDDS-7339. Implement Certificate renewal task for services #3982

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.

Sign up for GitHub

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

Jump to bottom
Merged
ChenSammi merged 10 commits into from
Jan 5, 2023
Merged

HDDS-7339. Implement Certificate renewal task for services #3982

ChenSammi merged 10 commits into from
Jan 5, 2023

Conversation

@ChenSammi
Copy link
Contributor

@ChenSammi ChenSammi commented Nov 21, 2022

@adoroszlai adoroszlai marked this pull request as draft November 21, 2022 07:01
@kerneltime kerneltime requested a review from fapifta November 28, 2022 17:10
@ChenSammi ChenSammi marked this pull request as ready for review November 29, 2022 12:36
@ChenSammi ChenSammi changed the title HDDS-7339. Implement Certificate renewal task for services. HDDS-7339. Implement Certificate renewal task for services Nov 29, 2022
@ChenSammi ChenSammi force-pushed the HDDS-7339 branch 2 times, most recently from 1f0a707 to 0bbef31 Compare December 1, 2022 05:23
@ChenSammi
Copy link
Contributor Author

ChenSammi commented Dec 1, 2022

Changes,

  1. change constructor of DefaultCertificateClient from using SecurityConfig to OzoneConfiguration.
  2. OM and DN generate certificate process refactor. Add signAndStoreCertificate and getCSRBuilder function in DNCertificateClient and OMCertificateClient by leveraging the current corresponding code in OzoneManager and HddsDatanodeService. So the code will be used in both first time cert generation and later cert rotation.
  3. move OMCertificateClient from hdds-server-framework to ozone-manager module so that OMCertificateClient can use classed in this module.
  4. add the certificate monitor task in OzoneManager and HddsDatanodeService. re-generate om/dn key and do the cert rotation.
  5. add unit test for cert rotation.

@ChenSammi
Copy link
Contributor Author

ChenSammi commented Dec 1, 2022

@fapifta , I have finished the Recon part code change locally. Then I found that it looks like recon certificate client is not used anymore after it's been initialized.

fapifta
fapifta reviewed Dec 12, 2022
View reviewed changes
Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ChenSammi I am really sorry about taking this much time to conduct the review, please find one general thought here, and the rest inline.
I think the MonitorTask itself is pretty much repetitive, while also the setup of the executorService code added to OM DN and Recon seems to be pretty much similar.

Wouldn't it be better to move this into a separated class, and provide the details as some functionals? The only difference I spot for the first time is how the code saves the certificate's serial id to it storage. The rest seems to be pretty much the same code.
What do you think, can we put it into a separate class where we can setup the background exeutor start it, and we also can generally define the monitoring task implementation?

@fapifta
Copy link
Contributor

fapifta commented Dec 12, 2022

Sorry github just lost all my inline comments, and I have to leave now, will write them again sometime later today.

@ChenSammi
Copy link
Contributor Author

ChenSammi commented Dec 13, 2022 •
edited
Loading

Hi @ChenSammi I am really sorry about taking this much time to conduct the review, please find one general thought here, and the rest inline. I think the MonitorTask itself is pretty much repetitive, while also the setup of the executorService code added to OM DN and Recon seems to be pretty much similar.

Wouldn't it be better to move this into a separated class, and provide the details as some functionals? The only difference I spot for the first time is how the code saves the certificate's serial id to it storage. The rest seems to be pretty much the same code. What do you think, can we put it into a separate class where we can setup the background exeutor start it, and we also can generally define the monitoring task implementation?

@fapifta , thanks for the review. It's quite a big patch.
There are two differences for each component's monitoring task, one is the way to terminate the service, one is how component certificate id is persisted. Let me try if it can be put into the CertificateClient implementation, then every certificate client can monitor it's certificate lifetime internally. But we need some callbacks implemented by component service to provide the certificate ID persist and service shutdown. Given overall there is just a few lines of code in monitoring task, not sure if these type of refactor will look more concise.

fapifta
fapifta reviewed Dec 16, 2022
View reviewed changes
Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ChenSammi, thank you for your thorough work on this one, and for addressing my concerns/suggestions.

I found a few minor things, please find them inline.

fapifta
fapifta approved these changes Jan 5, 2023
View reviewed changes
Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing all the things over this time. I think we should commit this now.

@ChenSammi
Copy link
Contributor Author

ChenSammi commented Jan 5, 2023

Thanks @fapifta for the code review and all the suggestions.

@ChenSammi ChenSammi merged commit 41f0488 into apache:master Jan 5, 2023
errose28 added a commit to errose28/ozone that referenced this pull request Jan 9, 2023
@errose28
Merge branch 'master' into HDDS-7496
c72428c 
* master: (176 commits)
  HDDS-7726. EC: Enhance datanode reconstruction log message (apache#4155)
  HDDS-7739. EC: Increase the information in the RM sending command log message (apache#4153)
  HDDS-7652. Volume Quota not enforced during write when bucket quota is not set (apache#4124)
  HDDS-7628. Intermittent failure in TestOzoneContainerWithTLS (apache#4142)
  HDDS-7695. EC metrics related to replication commands don't add up (apache#4152)
  HDDS-7729. EC: ECContainerReplicaCount should handle pending delete of unhealthy replicas (apache#4146)
  HDDS-7738. SCM terminates when adding container to a closed pipeline (apache#4154)
  HDDS-7243. Remove RequestFeatureValidator from echoRPC method which supports only ValidationCondition.OLDER_CLIENT_REQUESTS (apache#4051)
  HDDS-7708. No check for certificate duration config scenarios. (apache#4149)
  HDDS-7727. EC: SCM unregistered event handler for DatanodeCommandCountUpdated (apache#4147)
  HDDS-7606. Add SCM HA support in intellij run (apache#4058)
  HDDS-7666. EC: Unrecoverable EC containers with some remaining replicas may block decommissioning (apache#4118)
  HDDS-7339. Implement Certificate renewal task for services (apache#3982)
  HDDS-7696. MisReplicationHandler does not consider QUASI_CLOSED replicas as sources (apache#4144)
  HDDS-7714. Docker cluster ozone-om-ha fails during docker-compose up (apache#4137)
  HDDS-7716. Log read requests rejected with permission denied in OM audit (apache#4136)
  HDDS-7588. Intermittent failure in TestObjectStoreWithLegacyFS#testFlatKeyStructureWithOBS (apache#4040)
  HDDS-7633. Compile error with Java 11: package com.sun.jmx.mbeanserver is not visible (apache#4077)
  HDDS-7648. Add a servername tag in UGI metrics. (apache#4094)
  HDDS-7564. Update Ozone version after 1.3.0 release (apache#4115)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fapifta fapifta fapifta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ChenSammi @fapifta