[
https://issues.apache.org/jira/browse/HDDS-696?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16699998#comment-16699998
]
Xiaoyu Yao commented on HDDS-696:
---------------------------------
Thanks [~anu] for the patch. It looks pretty good to me. Here are a few minor
comments:
*BlockTokenException.java*
Line 26: NIT: accidental change can be removed.
*CertificateCodec.java*
Line 103-106: can we put isPosix() into a util class so that the same code
can be shared between CertificateCodec and KeyCodec.
After second check, I think we don't need this as it is not being used in the
code. Line 221 calls Files.setPosixFilePermissions already have it coverred.
Line 117-118: should we have a static JcaX509CertificateConverter so that we
don't have to create each time.
This will be useful for CA. Also, we need to call setProvider() to honor the
"BC" as provider from the
Securityconfig.
Line 201: basePath is not hornored in the code. (Same on Line 248)
Line 203: SCMSecurityException is not needed to be declared here as it is a
subclass of IOException.
Line 255: need to use the getInstance with provider name parameter to honor
"BC" provider from security config.
*CertificateServer.java*
Line 56: SCMSecurityException can be removed.
*CertificateSignRequest.java*
The file location does not match the package declaration.
*DefaultCAServer.java*
Line 63: NIT: can we start a new line for "1. Success…"
Line 84: NIT: typo: "success"
Line 227/245: should we remove the securityConfig parameter and use the member
variable config instead if we could
let SecurityConfig passed into DefaultCAServer contstructor (like other class
such as KeyCodec/HDDSKeyGenerator) and it has been initialized outside the
DefaultCAServer anyway?
*KeyCodec.java*
Line 65-68: NIT: let's be consistent with the order of "final static"
Line 315-319: Line 324 will throw if it is not posix, do we still need a
separate check here?
*OmMetadataManagerImpl.java*
Line 160: NIT: empty line change can be removed.
*Package-info.java*
Line 22: the package for
main/…/x509/certificates should not change its package name to
main/…/x509.certificate.utils
test/…/x509/certificates should not change its package name to
test/…/x509.certificate.utils
If they are moved under utils, we might be able to remove these files.
*SelfSignedCertificate.java*
Line 20: file need to be moved under certificate.utils with the package name
change.
Line 132-133: I think we should simply use endDate.atTime(LocalTime.MAX) to
indicate proper end time or
a slightly complex one like
endDate.atStartOfDay().plusDays(1).minusSeconds(1).toInstant(zoneOffset);
Line 216: do we need to +1 considering we allow the certificate to be valid
from the begin of the beginDate to the end of the endDate.
Line 219: this should be > 0, i.e., when certDuration > maxDuration we throw.
> Bootstrap genesis SCM(CA) with self-signed certificate.
> -------------------------------------------------------
>
> Key: HDDS-696
> URL: https://issues.apache.org/jira/browse/HDDS-696
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
> Reporter: Xiaoyu Yao
> Assignee: Anu Engineer
> Priority: Major
> Attachments: HDDS-696-HDDS-4.001.patch, HDDS-696-HDDS-4.002.patch,
> HDDS-696-HDDS-4.003.patch
>
>
> If security is enabled, SCM will generate the CA certs and bootstrap a CA. If
> it is already bootstrapped it the keys and root certificates are read from
> the secure store, if not, they are generated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
