diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java index 4f1f66faccba..122b04b715d3 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java @@ -44,13 +44,7 @@ public void put(String id, S3SecretValue secretValue) { @Override public void invalidate(String id) { - S3SecretValue secret = cache.getIfPresent(id); - if (secret == null) { - return; - } - secret.setDeleted(true); - secret.setAwsSecret(null); - cache.put(id, secret); + cache.asMap().computeIfPresent(id, (k, secret) -> secret.deleted()); } /** diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java index e97adc0a50f4..cb1ed0976a08 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java @@ -27,7 +27,7 @@ /** * S3Secret to be saved in database. */ -public class S3SecretValue { +public final class S3SecretValue { private static final Codec CODEC = new DelegatedCodec<>( Proto2Codec.get(S3Secret.getDefaultInstance()), S3SecretValue::fromProtobuf, @@ -38,16 +38,29 @@ public static Codec getCodec() { } // TODO: This field should be renamed to accessId for generalization. - private String kerberosID; - private String awsSecret; - private boolean isDeleted; - private long transactionLogIndex; + private final String kerberosID; + private final String awsSecret; + private final boolean isDeleted; + private final long transactionLogIndex; - public S3SecretValue(String kerberosID, String awsSecret) { - this(kerberosID, awsSecret, false, 0L); + public static S3SecretValue of(String kerberosID, String awsSecret) { + return of(kerberosID, awsSecret, 0); } - public S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted, + public static S3SecretValue of(String kerberosID, String awsSecret, long transactionLogIndex) { + return new S3SecretValue( + Objects.requireNonNull(kerberosID), + Objects.requireNonNull(awsSecret), + false, + transactionLogIndex + ); + } + + public S3SecretValue deleted() { + return new S3SecretValue(kerberosID, "", true, transactionLogIndex); + } + + private S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted, long transactionLogIndex) { this.kerberosID = kerberosID; this.awsSecret = awsSecret; @@ -59,26 +72,14 @@ public String getKerberosID() { return kerberosID; } - public void setKerberosID(String kerberosID) { - this.kerberosID = kerberosID; - } - public String getAwsSecret() { return awsSecret; } - public void setAwsSecret(String awsSecret) { - this.awsSecret = awsSecret; - } - public boolean isDeleted() { return isDeleted; } - public void setDeleted(boolean status) { - this.isDeleted = status; - } - public String getAwsAccessKey() { return kerberosID; } @@ -87,12 +88,8 @@ public long getTransactionLogIndex() { return transactionLogIndex; } - public void setTransactionLogIndex(long transactionLogIndex) { - this.transactionLogIndex = transactionLogIndex; - } - public static S3SecretValue fromProtobuf(S3Secret s3Secret) { - return new S3SecretValue(s3Secret.getKerberosID(), s3Secret.getAwsSecret()); + return S3SecretValue.of(s3Secret.getKerberosID(), s3Secret.getAwsSecret()); } public S3Secret getProtobuf() { diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java index 548b1b303501..38044805f3b4 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java @@ -264,7 +264,7 @@ public void testS3Auth() throws Exception { // Add secret to S3Secret table. s3SecretManager.storeSecret(accessKey, - new S3SecretValue(accessKey, secret)); + S3SecretValue.of(accessKey, secret)); OMRequest writeRequest = OMRequest.newBuilder() .setCmdType(OzoneManagerProtocolProtos.Type.CreateVolume) @@ -313,7 +313,7 @@ public void testS3Auth() throws Exception { // Override secret to S3Secret store with some dummy value s3SecretManager - .storeSecret(accessKey, new S3SecretValue(accessKey, "dummy")); + .storeSecret(accessKey, S3SecretValue.of(accessKey, "dummy")); // Write request with invalid credentials. omResponse = cluster.getOzoneManager().getOmServerProtocol() diff --git a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java index d1c4372ca915..0f3af5f3a3b1 100644 --- a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java +++ b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java @@ -43,7 +43,7 @@ public void testCodecWithCorrectData() throws Exception { final Codec codec = getCodec(); S3SecretValue s3SecretValue = - new S3SecretValue(UUID.randomUUID().toString(), + S3SecretValue.of(UUID.randomUUID().toString(), UUID.randomUUID().toString()); byte[] data = codec.toPersistedFormat(s3SecretValue); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java index 4e46adc66b1e..195ff816bc50 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java @@ -62,8 +62,7 @@ public S3SecretValue getSecret(String kerberosID) throws IOException { // purposely deleted the secret. Hence, we do not have to check the DB. return null; } - return new S3SecretValue(cacheValue.getKerberosID(), - cacheValue.getAwsSecret()); + return cacheValue; } S3SecretValue result = s3SecretStore.getSecret(kerberosID); if (result != null) { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java index 1edece52da23..aa7fe46992ea 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java @@ -113,26 +113,20 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn try { omClientResponse = ozoneManager.getS3SecretManager() .doUnderLock(accessId, s3SecretManager -> { - // Intentionally set to final so they can only be set once. - final S3SecretValue newS3SecretValue; // Update legacy S3SecretTable, if the accessId entry exists - if (s3SecretManager.hasS3Secret(accessId)) { - // accessId found in S3SecretTable. Update S3SecretTable - LOG.debug("Updating S3SecretTable cache entry"); - // Update S3SecretTable cache entry in this case - newS3SecretValue = new S3SecretValue(accessId, secretKey); - // Set the transactionLogIndex to be used for updating. - newS3SecretValue.setTransactionLogIndex(termIndex.getIndex()); - s3SecretManager - .updateCache(accessId, newS3SecretValue); - } else { + if (!s3SecretManager.hasS3Secret(accessId)) { // If S3SecretTable is not updated, // throw ACCESS_ID_NOT_FOUND exception. throw new OMException("accessId '" + accessId + "' not found.", OMException.ResultCodes.ACCESS_ID_NOT_FOUND); } + // Update S3SecretTable cache entry in this case + // Set the transactionLogIndex to be used for updating. + final S3SecretValue newS3SecretValue = S3SecretValue.of(accessId, secretKey, termIndex.getIndex()); + s3SecretManager.updateCache(accessId, newS3SecretValue); + // Compose response final SetS3SecretResponse.Builder setSecretResponse = SetS3SecretResponse.newBuilder() diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java index dcf5688e3958..90c27038eb42 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java @@ -150,21 +150,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn try { omClientResponse = ozoneManager.getS3SecretManager() .doUnderLock(accessId, s3SecretManager -> { - S3SecretValue assignS3SecretValue; - S3SecretValue s3SecretValue = - s3SecretManager.getSecret(accessId); + final S3SecretValue assignS3SecretValue; + S3SecretValue s3SecretValue = s3SecretManager.getSecret(accessId); if (s3SecretValue == null) { // Not found in S3SecretTable. if (createIfNotExist) { // Add new entry in this case - assignS3SecretValue = - new S3SecretValue(accessId, awsSecret.get()); - // Set the transactionLogIndex to be used for updating. - assignS3SecretValue.setTransactionLogIndex(termIndex.getIndex()); + assignS3SecretValue = S3SecretValue.of(accessId, awsSecret.get(), termIndex.getIndex()); // Add cache entry first. - s3SecretManager.updateCache(accessId, - assignS3SecretValue); + s3SecretManager.updateCache(accessId, assignS3SecretValue); } else { assignS3SecretValue = null; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java index a69b357419ac..3179a7d0f376 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java @@ -271,10 +271,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn } } - final S3SecretValue s3SecretValue = - new S3SecretValue(accessId, awsSecret); - // Set the transactionLogIndex to be used for updating. - s3SecretValue.setTransactionLogIndex(transactionLogIndex); + final S3SecretValue s3SecretValue = S3SecretValue.of(accessId, awsSecret, transactionLogIndex); // Add to tenantAccessIdTable final OmDBAccessIdInfo omDBAccessIdInfo = new OmDBAccessIdInfo.Builder() diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java index 49d19d3bc31a..ce7c0c848f11 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java @@ -106,9 +106,9 @@ public void setUp() throws Exception { serviceRpcAdd = new Text("localhost"); final Map s3Secrets = new HashMap<>(); s3Secrets.put("testuser1", - new S3SecretValue("testuser1", "dbaksbzljandlkandlsd")); + S3SecretValue.of("testuser1", "dbaksbzljandlkandlsd")); s3Secrets.put("abc", - new S3SecretValue("abc", "djakjahkd")); + S3SecretValue.of("abc", "djakjahkd")); om = mock(OzoneManager.class); OMMetadataManager metadataManager = new OmMetadataManagerImpl(conf, om); when(om.getMetadataManager()).thenReturn(metadataManager); diff --git a/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java b/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java index 892b86eaa7e4..c9bb4d6435ea 100644 --- a/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java +++ b/hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java @@ -115,7 +115,7 @@ public S3SecretValue getSecret(String kerberosID) throws IOException { return null; } - return new S3SecretValue(kerberosID, s3Secret); + return S3SecretValue.of(kerberosID, s3Secret); } catch (VaultException e) { LOG.error("Failed to read secret", e); throw new IOException("Failed to read secret", e); diff --git a/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java b/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java index 4700a283cd49..082a7da3f270 100644 --- a/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java +++ b/hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java @@ -89,7 +89,7 @@ public void clean() { @Test public void testReadWrite() throws IOException { SUCCESS_OPERATION_LIMIT.set(2); - S3SecretValue secret = new S3SecretValue("id", "value"); + S3SecretValue secret = S3SecretValue.of("id", "value"); s3SecretStore.storeSecret( "id", secret); @@ -101,7 +101,7 @@ public void testReadWrite() throws IOException { public void testReAuth() throws IOException { SUCCESS_OPERATION_LIMIT.set(1); AUTH_OPERATION_PROVIDER.set(1); - S3SecretValue secret = new S3SecretValue("id", "value"); + S3SecretValue secret = S3SecretValue.of("id", "value"); s3SecretStore.storeSecret("id", secret); assertEquals(secret, s3SecretStore.getSecret("id")); @@ -112,7 +112,7 @@ public void testReAuth() throws IOException { @Test public void testAuthFail() throws IOException { SUCCESS_OPERATION_LIMIT.set(1); - S3SecretValue secret = new S3SecretValue("id", "value"); + S3SecretValue secret = S3SecretValue.of("id", "value"); s3SecretStore.storeSecret("id", secret); assertThrows(IOException.class, diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java index f0528facbb67..ebd441567a37 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java @@ -379,7 +379,7 @@ public void cancelDelegationToken(Token token) @Override @Nonnull public S3SecretValue getS3Secret(String kerberosID) throws IOException { - return new S3SecretValue(STUB_KERBEROS_ID, STUB_SECRET); + return S3SecretValue.of(STUB_KERBEROS_ID, STUB_SECRET); } @Override diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index e6ff4024d1e1..007fa9099ee3 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -42,14 +42,14 @@ import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.notNull; import static org.mockito.Mockito.when; /** * Test for S3 secret generate endpoint. */ @ExtendWith(MockitoExtension.class) -public class TestSecretGenerate { +class TestSecretGenerate { private static final String USER_NAME = "test"; private static final String OTHER_USER_NAME = "test2"; private static final String USER_SECRET = "test_secret"; @@ -69,12 +69,11 @@ public class TestSecretGenerate { private static S3SecretValue getS3SecretValue(InvocationOnMock invocation) { Object[] args = invocation.getArguments(); - return new S3SecretValue((String) args[0], USER_SECRET); + return S3SecretValue.of((String) args[0], USER_SECRET); } @BeforeEach - void setUp() throws IOException { - when(proxy.getS3Secret(any())).then(TestSecretGenerate::getS3SecretValue); + void setUp() { OzoneConfiguration conf = new OzoneConfiguration(); OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy)); @@ -89,23 +88,20 @@ void setUp() throws IOException { @Test void testSecretGenerate() throws IOException { - when(principal.getName()).thenReturn(USER_NAME); - when(securityContext.getUserPrincipal()).thenReturn(principal); - when(context.getSecurityContext()).thenReturn(securityContext); + setupSecurityContext(); + hasNoSecretYet(); S3SecretResponse response = (S3SecretResponse) endpoint.generate().getEntity(); + assertEquals(USER_SECRET, response.getAwsSecret()); assertEquals(USER_NAME, response.getAwsAccessKey()); } @Test void testIfSecretAlreadyExists() throws IOException { - when(principal.getName()).thenReturn(USER_NAME); - when(securityContext.getUserPrincipal()).thenReturn(principal); - when(context.getSecurityContext()).thenReturn(securityContext); - when(proxy.getS3Secret(any())).thenThrow(new OMException("Secret already exists", - OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS)); + setupSecurityContext(); + hasSecretAlready(); Response response = endpoint.generate(); @@ -116,9 +112,27 @@ void testIfSecretAlreadyExists() throws IOException { @Test void testSecretGenerateWithUsername() throws IOException { + hasNoSecretYet(); + S3SecretResponse response = (S3SecretResponse) endpoint.generate(OTHER_USER_NAME).getEntity(); assertEquals(USER_SECRET, response.getAwsSecret()); assertEquals(OTHER_USER_NAME, response.getAwsAccessKey()); } + + private void setupSecurityContext() { + when(principal.getName()).thenReturn(USER_NAME); + when(securityContext.getUserPrincipal()).thenReturn(principal); + when(context.getSecurityContext()).thenReturn(securityContext); + } + + private void hasNoSecretYet() throws IOException { + when(proxy.getS3Secret(notNull())) + .then(TestSecretGenerate::getS3SecretValue); + } + + private void hasSecretAlready() throws IOException { + when(proxy.getS3Secret(notNull())) + .thenThrow(new OMException("Secret already exists", OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS)); + } }