You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/09/21 12:06:54 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #2946: Made CryptoException extend IOException
dlmarion opened a new pull request, #2946:
URL: https://github.com/apache/accumulo/pull/2946
Modified CryptoException such that it no longer extends RuntimeException and extends IOException instead. Initially I was going to modify it to extend GeneralSecurityException but that touched a lot of code and caused issues with Accumulo classes that implemented other interfaces that declared only IOException.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#discussion_r976911478
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/PerTableCryptoServiceFactory.java:
##########
@@ -46,12 +48,24 @@ public class PerTableCryptoServiceFactory implements CryptoServiceFactory {
private static final TableId REC_FAKE_ID = TableId.of("RECOVERY_CryptoService_FAKE_ID");
@Override
- public CryptoService getService(CryptoEnvironment environment, Map<String,String> props) {
+ public CryptoService getService(CryptoEnvironment environment, Map<String,String> props)
+ throws CryptoException {
if (environment.getScope() == CryptoEnvironment.Scope.WAL) {
- return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> getWALServiceInitialized(props));
+ return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> {
+ try {
+ return getWALServiceInitialized(props);
+ } catch (CryptoException e) {
+ throw new UncheckedIOException("Error creating WAL crypto service", e);
Review Comment:
If getService() is already throwing CryptoException, why catch and rethrow as Unchecked?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#discussion_r976915091
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/PerTableCryptoServiceFactory.java:
##########
@@ -46,12 +48,24 @@ public class PerTableCryptoServiceFactory implements CryptoServiceFactory {
private static final TableId REC_FAKE_ID = TableId.of("RECOVERY_CryptoService_FAKE_ID");
@Override
- public CryptoService getService(CryptoEnvironment environment, Map<String,String> props) {
+ public CryptoService getService(CryptoEnvironment environment, Map<String,String> props)
+ throws CryptoException {
if (environment.getScope() == CryptoEnvironment.Scope.WAL) {
- return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> getWALServiceInitialized(props));
+ return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> {
+ try {
+ return getWALServiceInitialized(props);
+ } catch (CryptoException e) {
+ throw new UncheckedIOException("Error creating WAL crypto service", e);
Review Comment:
It's for the lambda, can't throw checked exceptions
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#discussion_r977695377
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/PerTableCryptoServiceFactory.java:
##########
@@ -46,12 +48,24 @@ public class PerTableCryptoServiceFactory implements CryptoServiceFactory {
private static final TableId REC_FAKE_ID = TableId.of("RECOVERY_CryptoService_FAKE_ID");
@Override
- public CryptoService getService(CryptoEnvironment environment, Map<String,String> props) {
+ public CryptoService getService(CryptoEnvironment environment, Map<String,String> props)
+ throws CryptoException {
if (environment.getScope() == CryptoEnvironment.Scope.WAL) {
- return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> getWALServiceInitialized(props));
+ return cryptoServiceMap.computeIfAbsent(WAL_FAKE_ID, (id) -> {
+ try {
+ return getWALServiceInitialized(props);
+ } catch (CryptoException e) {
+ throw new UncheckedIOException("Error creating WAL crypto service", e);
Review Comment:
OK that makes sense. Check out my other comment about checked vs unchecked.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#discussion_r977611522
##########
core/src/main/java/org/apache/accumulo/core/spi/crypto/AESCryptoService.java:
##########
@@ -110,7 +110,7 @@ protected Cipher initialValue() {
try {
return Cipher.getInstance(KEY_WRAP_TRANSFORM);
} catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
- throw new CryptoException("Error creating Cipher for AESWrap", e);
+ throw new IllegalArgumentException("Error creating Cipher for AESWrap", e);
Review Comment:
I like this better since the 2 exceptions being caught are very likely to come from a mis-configuration.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#issuecomment-1257886957
We are going to leave CryptoException as a RuntimeException per comment in #2930
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#issuecomment-1255147525
> [This](https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html) is a nice clarification from Oracle docs: "If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception."
Do you think this should not be merged then and leave CryptoException as a RuntimeException. I was thinking that having a known checked Exception in the SPI methods makes sense so that errors specific to crypto can be handled.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#issuecomment-1255231894
> Do you think this should not be merged then and leave CryptoException as a RuntimeException. I was thinking that having a known checked Exception in the SPI methods makes sense so that errors specific to crypto can be handled.
For most situations, I don't think there will be much we can do if we catch a CryptoException (coming from a TabletServer, for example) other than restart the server. Since these interfaces are in the SPI, the "client" who handles the exception will usually be server code. And with the way the CryptoService runs within the tserver, there is a good chance a restart will have to happen to reload the service. Eventually, once the Crypto is properly tested we could drop the experimental tag and be able to determine a better way of handling the errors. Or if we create a designated Server running just for Crypto, there may be better ways to handle the errors. The RFile API may be a good place to throw a checked exception for example, allowing the client to handle encryption errors.
I do think there is room for improving the overall error handling though. But maybe changing the exception type isn't the right approach.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2946:
URL: https://github.com/apache/accumulo/pull/2946#issuecomment-1253614659
Merging this issue should complete the subtask "Make CryptoException extend GeneralSecurityException" in #2930
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed pull request #2946: Made CryptoException extend IOException
Posted by GitBox <gi...@apache.org>.
dlmarion closed pull request #2946: Made CryptoException extend IOException
URL: https://github.com/apache/accumulo/pull/2946
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org