-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-19471. ABFS: Support Fixed SAS Token at Container Level #7461
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,7 @@ public class AbfsConfiguration{ | |
|
||
private final Configuration rawConfig; | ||
private final String accountName; | ||
private final String fsName; | ||
// Service type identified from URL used to initialize FileSystem. | ||
private final AbfsServiceType fsConfiguredServiceType; | ||
private final boolean isSecure; | ||
|
@@ -451,17 +452,20 @@ public class AbfsConfiguration{ | |
* Constructor for AbfsConfiguration for specified service type. | ||
* @param rawConfig used to initialize the configuration. | ||
* @param accountName the name of the azure storage account. | ||
* @param fsName the name of the file system (container name). | ||
* @param fsConfiguredServiceType service type configured for the file system. | ||
* @throws IllegalAccessException if the field is not accessible. | ||
* @throws IOException if an I/O error occurs. | ||
*/ | ||
public AbfsConfiguration(final Configuration rawConfig, | ||
String accountName, | ||
String fsName, | ||
AbfsServiceType fsConfiguredServiceType) | ||
throws IllegalAccessException, IOException { | ||
this.rawConfig = ProviderUtils.excludeIncompatibleCredentialProviders( | ||
rawConfig, AzureBlobFileSystem.class); | ||
this.accountName = accountName; | ||
this.fsName = fsName; | ||
this.fsConfiguredServiceType = fsConfiguredServiceType; | ||
this.isSecure = getBoolean(FS_AZURE_SECURE_MODE, false); | ||
|
||
|
@@ -493,7 +497,7 @@ public AbfsConfiguration(final Configuration rawConfig, | |
*/ | ||
public AbfsConfiguration(final Configuration rawConfig, String accountName) | ||
throws IllegalAccessException, IOException { | ||
this(rawConfig, accountName, AbfsServiceType.DFS); | ||
this(rawConfig, accountName, EMPTY_STRING, AbfsServiceType.DFS); | ||
} | ||
|
||
/** | ||
|
@@ -589,6 +593,16 @@ public String accountConf(String key) { | |
return key + "." + accountName; | ||
} | ||
|
||
/** | ||
* Appends the container, account name to a configuration key yielding the | ||
* container-specific form. | ||
* @param key Account-agnostic configuration key | ||
* @return Container-specific configuration key | ||
*/ | ||
public String containerConf(String key) { | ||
return key + "." + fsName + "." + accountName; | ||
} | ||
|
||
/** | ||
* Returns the account-specific value if it exists, then looks for an | ||
* account-agnostic value. | ||
|
@@ -644,16 +658,20 @@ public int getInt(String key, int defaultValue) { | |
} | ||
|
||
/** | ||
* Returns the account-specific password in string form if it exists, then | ||
* Returns the container-specific password if it exists, | ||
* else searches for the account-specific password, else finally | ||
* looks for an account-agnostic value. | ||
* @param key Account-agnostic configuration key | ||
* @return value in String form if one exists, else null | ||
* @throws IOException if parsing fails. | ||
*/ | ||
public String getPasswordString(String key) throws IOException { | ||
char[] passchars = rawConfig.getPassword(accountConf(key)); | ||
if (passchars == null) { | ||
passchars = rawConfig.getPassword(key); | ||
char[] passchars = rawConfig.getPassword(containerConf(key)); | ||
if(passchars == null) { | ||
passchars = rawConfig.getPassword(accountConf(key)); | ||
if(passchars == null){ | ||
passchars = rawConfig.getPassword(key); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be simplified as :- char[] passchars = rawConfig.getPassword(containerConf(key)) != null ? |
||
} | ||
if (passchars != null) { | ||
return new String(passchars); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,11 +32,13 @@ | |
import org.apache.hadoop.fs.azurebfs.services.AuthType; | ||
import org.apache.hadoop.fs.azurebfs.services.FixedSASTokenProvider; | ||
import org.apache.hadoop.fs.azurebfs.utils.AccountSASGenerator; | ||
import org.apache.hadoop.fs.azurebfs.utils.ServiceSASGenerator; | ||
import org.apache.hadoop.fs.azurebfs.utils.Base64; | ||
|
||
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_SAS_FIXED_TOKEN; | ||
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_SAS_TOKEN_PROVIDER_TYPE; | ||
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.accountProperty; | ||
import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.containerProperty; | ||
import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_APP_ID; | ||
import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_APP_SECRET; | ||
import static org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_TEST_APP_SERVICE_PRINCIPAL_OBJECT_ID; | ||
|
@@ -50,6 +52,7 @@ | |
public class ITestAzureBlobFileSystemChooseSAS extends AbstractAbfsIntegrationTest{ | ||
|
||
private String accountSAS = null; | ||
private String containerSAS = null; | ||
private static final String TEST_PATH = "testPath"; | ||
|
||
/** | ||
|
@@ -69,6 +72,7 @@ public void setup() throws Exception { | |
super.setup(); | ||
createFilesystemWithTestFileForSASTests(new Path(TEST_PATH)); | ||
generateAccountSAS(); | ||
generateContainerSAS(); | ||
} | ||
|
||
/** | ||
|
@@ -85,6 +89,22 @@ private void generateAccountSAS() throws AzureBlobFileSystemException { | |
accountSAS = configAccountSASGenerator.getAccountSAS(getAccountName()); | ||
} | ||
|
||
/** | ||
* Generates a Container SAS Token using the Account Shared Key to be used as a fixed SAS Token. | ||
* Container SAS used here will have only read permissions to resources. | ||
* This will be used by individual tests to set in the configurations. | ||
* @throws AzureBlobFileSystemException | ||
*/ | ||
private void generateContainerSAS() throws AzureBlobFileSystemException { | ||
final byte[] accountKey = Base64.decode( | ||
getConfiguration().getStorageAccountKey()); | ||
ServiceSASGenerator configServiceSASGenerator = new ServiceSASGenerator( | ||
accountKey); | ||
// Setting only read permissions. | ||
configServiceSASGenerator.setPermissions("r"); | ||
containerSAS = configServiceSASGenerator.getContainerSASWithFullControl( | ||
getAccountName(), getFileSystemName()); | ||
} | ||
/** | ||
* Tests the scenario where both the custom SASTokenProvider and a fixed SAS token are configured. | ||
* Custom implementation of SASTokenProvider class should be chosen and User Delegation SAS should be used. | ||
|
@@ -126,6 +146,42 @@ public void testBothProviderFixedTokenConfigured() throws Exception { | |
} | ||
} | ||
|
||
/** | ||
* Helper method to get the Fixed SAS token value | ||
*/ | ||
private String getFixedSASToken(AbfsConfiguration config) throws Exception { | ||
return config.getSASTokenProvider().getSASToken(this.getAccountName(), this.getFileSystemName(), getMethodName(), "read"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use constants for string |
||
} | ||
|
||
/** | ||
* Tests the implementation sequence if all fixed SAS configs are set. | ||
* The expected sequence is Container Specific Fixed SAS, Account Specific Fixed SAS, Account Agnostic Fixed SAS. | ||
* @throws IOException | ||
*/ | ||
@Test | ||
public void testFixedTokenPreference() throws Exception { | ||
AbfsConfiguration testAbfsConfig = new AbfsConfiguration( | ||
getRawConfiguration(), this.getAccountName(), this.getFileSystemName(), getAbfsServiceType()); | ||
|
||
// setting all types of Fixed SAS configs (container-specific, account-specific, account-agnostic) | ||
removeAnyPresetConfiguration(testAbfsConfig); | ||
testAbfsConfig.set(containerProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getFileSystemName(), this.getAccountName()), containerSAS); | ||
testAbfsConfig.set(accountProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getAccountName()), accountSAS); | ||
testAbfsConfig.set(FS_AZURE_SAS_FIXED_TOKEN, accountSAS); | ||
|
||
// Assert that Container Specific Fixed SAS is used | ||
Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("sr=c"); | ||
|
||
// Assert that Account Specific Fixed SAS is used if container SAS isn't set | ||
testAbfsConfig.unset(containerProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getFileSystemName(), this.getAccountName())); | ||
Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("ss=bf"); | ||
|
||
//Assert that Account-Agnostic fixed SAS is used if no other fixed SAS configs are set. | ||
// The token is the same as the Account Specific Fixed SAS. | ||
testAbfsConfig.unset(accountProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getAccountName())); | ||
Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("ss=bf"); | ||
} | ||
|
||
/** | ||
* Tests the scenario where only the fixed token is configured, and no token provider class is set. | ||
* Account SAS Token configured as fixed SAS should be used. | ||
|
@@ -189,5 +245,6 @@ private void removeAnyPresetConfiguration(AbfsConfiguration testAbfsConfig) { | |
testAbfsConfig.unset(FS_AZURE_SAS_FIXED_TOKEN); | ||
testAbfsConfig.unset(accountProperty(FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, this.getAccountName())); | ||
testAbfsConfig.unset(accountProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getAccountName())); | ||
testAbfsConfig.unset(containerProperty(FS_AZURE_SAS_FIXED_TOKEN, this.getFileSystemName(), this.getAccountName())); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should add a separate constructor with this new param