You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "fapifta (via GitHub)" <gi...@apache.org> on 2023/03/11 22:13:24 UTC
[GitHub] [ozone] fapifta opened a new pull request, #4382: HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod() test fails as we approach DST.
fapifta opened a new pull request, #4382:
URL: https://github.com/apache/ozone/pull/4382
## What changes were proposed in this pull request?
In my local environment the TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod() test started to fail, as we are approaching DST, and with that the return value of DefaultCertificateClient#timeBeforeExpiryGracePeriod(X509Certificate) is off by an hour.
This is because the test sets a 28 days expiration to the certificate, but due to switch to DST, at 3am in one of the mornings we switch to 2am, therefore the certificate expiration is 28days and 1 hour away instead of the expected 0.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8144
## How was this patch tested?
After the change the named test runs fine in my local env.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4382: HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4382:
URL: https://github.com/apache/ozone/pull/4382#issuecomment-1468389468
Thanks @fapifta for the patch, @Galsza for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4382: HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST.
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4382:
URL: https://github.com/apache/ozone/pull/4382#discussion_r1134583905
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -922,7 +922,7 @@ public Duration timeBeforeExpiryGracePeriod(X509Certificate certificate) {
Duration gracePeriod = securityConfig.getRenewalGracePeriod();
Date expireDate = certificate.getNotAfter();
LocalDateTime gracePeriodStart = expireDate.toInstant()
- .atZone(ZoneId.systemDefault()).toLocalDateTime().minus(gracePeriod);
+ .minus(gracePeriod).atZone(ZoneId.systemDefault()).toLocalDateTime();
Review Comment:
Yes, thank you @adoroszlai for spotting this one.
In general this one did not caused a problem, as this implementation is used with functionality that does not happen around certificate renewal, and where it does, the grace period is a few seconds, so if it remains this way it does not cause trouble just for very short periods of time, so probably we would not recognize it, but nonetheless, I have pushed the change to the test class in a follow up commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #4382: HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod fails as we approach DST.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4382:
URL: https://github.com/apache/ozone/pull/4382
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4382: HDDS-8144. TestDefaultCertificateClient#testTimeBeforeExpiryGracePeriod() test fails as we approach DST.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4382:
URL: https://github.com/apache/ozone/pull/4382#discussion_r1133196421
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -922,7 +922,7 @@ public Duration timeBeforeExpiryGracePeriod(X509Certificate certificate) {
Duration gracePeriod = securityConfig.getRenewalGracePeriod();
Date expireDate = certificate.getNotAfter();
LocalDateTime gracePeriodStart = expireDate.toInstant()
- .atZone(ZoneId.systemDefault()).toLocalDateTime().minus(gracePeriod);
+ .minus(gracePeriod).atZone(ZoneId.systemDefault()).toLocalDateTime();
Review Comment:
Same logic appears in `CertificateClientTestImpl`, should be fixed there, too?
https://github.com/apache/ozone/blob/111161d78253e2afe920b14d2a14be282ea57e90/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClientTestImpl.java#L158-L159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org