HDDS-7398. Tool to remove old certs from the scm db #3972

Galsza · 2022-11-17T10:01:03Z

What changes were proposed in this pull request?

Create a command line tool to remove expired certificates from the SCM metadata store.

What is the link to the Apache JIRA

How was this patch tested?

Added unit test for the command. There is also an ongoing test with a real cluster to see if the certs really get removed.

…adata store

adoroszlai

Thanks @Galsza for the patch. Looks good overall.

Executing the following command (even on a datanode) returns success (rc=0), no error messages.

ozone admin cert clean --db /tmp --name asdf

I guess it should complain about not being able to open the DB.

adoroszlai · 2022-11-28T19:38:21Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java

+import org.apache.hadoop.hdds.utils.db.cache.TableCache.CacheType;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;


Please avoid moving around imports.

There are also imports movings in other java files.

adoroszlai · 2022-11-28T19:42:14Z

hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/cert/CleanExpired.java

+  @CommandLine.Option(names = {"--db"},
+      required = true,
+      description = "Database metadata directory")
+  private String dbDirectory;
+
+  @CommandLine.Option(names = {"--name"},
+      required = true,
+      description = "Database file name")
+  private String dbName;


For convenience I would prefer having a single option, then splitting dirname/filename.

ChenSammi · 2022-12-01T05:50:41Z

hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/cert/CleanExpired.java

+ */
+@CommandLine.Command(
+    name = "clean",
+    description = "Clean expired certificates from the SCM metadata",


Please add description about that this command only support to run when scm is shutdown.

ChenSammi · 2022-12-01T05:55:05Z

hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/cert/CleanExpired.java

+        configuration, new File(dbDirectory), dbName, new SCMDBDefinition())) {
+      removeExpiredCertificates(dbStore);
+    } catch (IOException e) {
+      System.out.println("Error trying to open" + dbName +


Please include the exception reason or message in the output error message.

ChenSammi · 2022-12-01T06:08:03Z

hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/cert/CleanExpired.java

+        }
+      }
+    } catch (IOException e) {
+      System.out.println("Error when trying to open certificate table from db");


Please include the detail error cause.

symious · 2022-12-01T06:17:47Z

@Galsza Thanks for the work.

I was just wondering, is it also an option to add this feature as a daemon service in SCM, so that we don't have to do the cleaning manually?

Restore import order, add detailed exception message and use only one parameter for the db.

Galsza · 2022-12-05T14:53:48Z

@Galsza Thanks for the work.

I was just wondering, is it also an option to add this feature as a daemon service in SCM, so that we don't have to do the cleaning manually?

@symious Thanks for the comment. This is more of a housekeeping tool for older certificates. There is no need to add this feature as a daemon in SCM. The current plan for certificate rotation is that whenever a certificate is about the expire a new one will be created. This new certificate is used after instead of the old one. After some propagation time, when we can finally ensure that the old certificate is successfully replaced everywhere by the new one, the old certificate can be safely removed, which would be done there.

Restore import order, add detailed exception message and use only one parameter for the db.

fapifta · 2022-12-12T21:41:17Z

...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMMetadataStoreImpl.java

      metaTable = META.getTable(store);

-      checkAndPopulateTable(moveTable, META.getName());
+      checkAndPopulateTable(metaTable, META.getName());


Ouch...
Fortunately this affects just metrics as I see the tableMap that is populated here is not used elsewhere...

I believe this is irrelevant for the CLI tool, and this should be in a separate patch under a separate JIRA describing the problem we fix, and we should not hide it here.

FYI, this is already fixed in HDDS-7490.

fapifta

Hi @Galsza thank you for taking this on, the patch overall seems to be good, and you had a very nice catch in the db definition. Let's extract that from this patch as it is irrelevant, and I am happy to +1 and commit this change.

As noted for that typo fix we should have a separate JIRA outlining the issue we had due to the typo ;)

…e_SCM_DB

fapifta

Thank you for your work on this @Galsza the changes look good, and as I see all the comments were addressed.

@ChenSammi @adoroszlai @symious please let me know if you have any further comments. if not, I will merge this before EOW.

* master: (88 commits) HDDS-7463. SCM Pipeline scrubber never able to cleanup allocated pipeline. (apache#4093) HDDS-7683. EC: ReplicationManager - UnderRep maintenance handler should not request nodes if none needed (apache#4109) HDDS-7635. Update failure metrics when allocate block fails in preExecute. (apache#4086) HDDS-7565. FSO purge directory for old bucket can update quota for new bucket (apache#4021) HDDS-7654. EC: ReplicationManager - merge mis-rep queue into under replicated queue (apache#4099) HDDS-7621. Update SCM term in datanode from heartbeat without any commands (apache#4101) HDDS-7649. S3 multipart upload EC release space quota wrong for old version (apache#4095) HDDS-7399. Enable specifying external root ca (apache#4053) HDDS-7398. Tool to remove old certs from the scm db (apache#3972) HDDS-6650. S3MultipartUpload support update bucket usedNamespace. (apache#4081) HDDS-7605. Improve logging in Container Balancer (apache#4067) HDDS-7616. EC: Refactor Unhealthy Replicated Processor (apache#4063) HDDS-7426. Add a new acceptance test for Streaming Pipeline. (apache#4019) HDDS-7478. [Ozone-Streaming] NPE in when creating a file with o3fs. (apache#3949) HDDS-7425. Add documentation for the new Streaming Pipeline feature. (apache#3913) HDDS-7438. [Ozone-Streaming] Add a createStreamKey method to OzoneBucket. (apache#3914) HDDS-7431. [Ozone-Streaming] Disable data steam by default. (apache#3900) HDDS-6955. [Ozone-streaming] Add explicit stream flag in ozone shell (apache#3559) HDDS-6867. [Ozone-Streaming] PutKeyHandler should not use streaming to put EC key. (apache#3516) HDDS-6842. [Ozone-Streaming] Reduce the number of watch requests in StreamCommitWatcher. (apache#3492) ...

Galsza added 3 commits November 14, 2022 18:04

HDDS-7398 Create CLI tool to remove expired certificates from SCM met…

70b3b47

…adata store

HDDS-7398 Fix test, refine CleanExpired

9f160fc

HDDS-7398 Fix checkstyle issues, minor refinements.

e48e5f6

Galsza changed the title ~~Hdds 7398 tool to remove old certs from the scm db~~ HDDS-7398. Tool to remove old certs from the scm db Nov 17, 2022

kerneltime requested review from ChenSammi and fapifta November 21, 2022 17:07

adoroszlai reviewed Nov 28, 2022

View reviewed changes

ChenSammi reviewed Dec 1, 2022

View reviewed changes

HDDS-7398 Fix review comments.

76834db

Restore import order, add detailed exception message and use only one parameter for the db.

Galsza added 3 commits December 6, 2022 09:09

HDDS-7398 Fix review comments.

a59e22b

Restore import order, add detailed exception message and use only one parameter for the db.

HDDS-7398. Prevent invalid paths and directories

541742e

HDDS-7398. Remove unnecessary changes

f42ba32

fapifta reviewed Dec 12, 2022

View reviewed changes

Galsza added 2 commits December 15, 2022 16:41

Merge branch 'master' into HDDS-7398_Tool_to_remove_old_certs_from_th…

84805e0

…e_SCM_DB

HDDS-7398. Remove whitespace change

d60ec4c

fapifta approved these changes Dec 15, 2022

View reviewed changes

fapifta merged commit 2ac31e1 into apache:master Dec 19, 2022

HDDS-7398. Tool to remove old certs from the scm db #3972

HDDS-7398. Tool to remove old certs from the scm db #3972

Uh oh!

Conversation

Galsza commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

symious commented Dec 1, 2022

Uh oh!

Galsza commented Dec 5, 2022

Uh oh!

fapifta Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

fapifta left a comment

Choose a reason for hiding this comment

Uh oh!

fapifta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Galsza commented Nov 17, 2022 •

edited

Loading

ChenSammi Dec 1, 2022 •

edited

Loading