----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38865/#review101117 -----------------------------------------------------------
Ship it! Big review :) My preference is to use feature branches for this type of work so that you can make smaller, more iterative contributions. Larger commits have more chances of having problems missed. And often times, we rush to get the bare minimum tests in just so we could post a review. Not saying that's what I see here - just an overall observation so that we can make reviews less painful and easier to understand. ambari-server/src/main/java/org/apache/ambari/server/api/services/CredentialService.java (line 59) <https://reviews.apache.org/r/38865/#comment158439> I don't think that there should be a body on an @GET method. ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1319) <https://reviews.apache.org/r/38865/#comment158440> Doc. ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1323) <https://reviews.apache.org/r/38865/#comment158442> StringUtils.isEmpty() ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1334) <https://reviews.apache.org/r/38865/#comment158441> Doc. ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1338) <https://reviews.apache.org/r/38865/#comment158443> StringUtils.isEmpty() ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1349) <https://reviews.apache.org/r/38865/#comment158444> Doc. ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java (line 1372) <https://reviews.apache.org/r/38865/#comment158445> Doc. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java (lines 172 - 173) <https://reviews.apache.org/r/38865/#comment158446> This is an interesting way to retrieve the resource provider. Any reason you use a factory instead of instantiating a singleton? Seems like this controller class does it both ways. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java (lines 61 - 83) <https://reviews.apache.org/r/38865/#comment158447> Creating a new static anonymous class for each set is something I don't see often. Although it should not maintain a reference to the parent since it's static, I'd rather no use java compiler voodoo for something simple here. Maybe just a static block to add the properties to a normal static Set? ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java (line 127) <https://reviews.apache.org/r/38865/#comment158448> Null or also empty? ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java (lines 67 - 68) <https://reviews.apache.org/r/38865/#comment158450> Wouldn't this be taken care of by your `hasPermission(...);` calls inside of ClusterService ? ambari-server/src/main/java/org/apache/ambari/server/security/credential/Credential.java (line 21) <https://reviews.apache.org/r/38865/#comment158451> Doc ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java (lines 63 - 65) <https://reviews.apache.org/r/38865/#comment158453> Concurrency issue here? What happens if two threads are updating the same keystore with different credentials? ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java (line 157) <https://reviews.apache.org/r/38865/#comment158454> AES as a default is fine, I think. Any reason we need to make this configurable to adhere to international regulations on strength of encryption? ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreService.java (line 120) <https://reviews.apache.org/r/38865/#comment158449> Thank you for doc'ing this. However, I still do not like using Boolean objects for 3-way values. It's too easy to accidentally unbox it and cause a NPE. - Jonathan Hurley On Sept. 30, 2015, 8:28 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38865/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2015, 8:28 a.m.) > > > Review request for Ambari, Jonathan Hurley, John Speidel, and Robert > Nettleton. > > > Bugs: AMBARI-13214 > https://issues.apache.org/jira/browse/AMBARI-13214 > > > Repository: ambari > > > Description > ------- > > Storage of the credentials is to be done using the existing _secure_ > credentials provider API which already exits within Ambari. > > Credential may be stored in either Ambari's persistent or temporary secure > storage facilities. > > # Testing capabilities > > * Request > ``` > GET api/v1/clusters/{CLUSTER_NAME} > ``` > > * Responses > ``` > 200 OK > { > ... > "credential_store_properties" : { > "storage.persistent" : "true", > "storage.temporary" : "true" > }, > ... > } > ``` > > # Creating credentials > > * Request > ``` > POST /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS} > { > "Credential" : { > "principal" : "USERNAME", > "key" : "SECRET", > "persist" : true > } > } > > Where: > ** principal: the principal (or username) part of the credential to store > ** key: the secret key part of the credential to store > ** persist: a boolean value indicating whether to store this credential in a > persisted (true) or temporary (false) secure credential store > ``` > > * Responses > ``` > 200 OK > ``` > ``` > 400 Bad Request > { > "status": 400, > "message": "Cannot persist credential in Ambari's secure credential store > since secure storage has not yet be configured. Use ambari-server > setup-security to enable this feature." > } > ``` > ``` > 403 Forbidden > { > "status": 403, > "message": "You do not have permissions to access this resource." > } > ``` > > # Updating credentials > > * Request > ``` > PUT /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS} > { > "Credential" : { > "principal" : "USERNAME", > "key" : "SECRET1", > "persist" : true > } > } > > Where: > ** principal: the principal (or username) part of the credential to store > ** key: the secret key part of the credential to store > ** persist: a boolean value indicating whether to store this credential in a > persisted (true) or temporary (false) secure credential store > ``` > > * Responses > ``` > 200 OK > ``` > ``` > 400 Bad Request > { > "status": 400, > "message": "Cannot persist credential in Ambari's secure credential store > since secure storage has not yet be configured. Use ambari-server > setup-security to enable this feature." > } > ``` > ``` > 403 Forbidden > { > "status": 403, > "message": "You do not have permissions to access this resource." > } > ``` > > # Removing credentials > > * Request > ``` > DELETE /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS} > ``` > > * Responses > ``` > 200 OK > ``` > ``` > 404 Not Found > { > "status": 404, > "message": "Not Found" > } > ``` > ```403 Forbidden > { > "status": 403, > "message": "You do not have permissions to access this resource." > } > ``` > > # Listing credentials > > * Request > ``` > GET /api/v1/clusters/{CLUSTER_NAME}/credentials > ``` > > * Responses > ``` > 200 OK > { > "href" : "http://host:8080/api/v1/clusters/c1/credentials";, > "items" : [ > { > "href" : > "http://host:8080/api/v1/clusters/c1/credentials/kdc.admin.credentials";, > "Credential" : { > "alias" : "kdc.admin.credentials", > "cluster_name" : "c1" > } > }, > { > "href" : > "http://host:8080/api/v1/clusters/c1/credentials/service.admin.credentials";, > "Credential" : { > "alias" : "service.admin.credentials", > "cluster_name" : "c1" > } > } > ] > } > ``` > ``` > 404 Not Found > { > "status": 404, > "message": "Not Found" > } > ``` > ``` > 403 Forbidden > { > "status": 403, > "message": "You do not have permissions to access this resource." > } > ``` > > # Retrieving credentials > > * Request > ``` > GET /api/v1/clusters/{CLUSTER_NAME}/credentials/{ALIAS} > ``` > > * Responses > ``` > 200 OK > { > "href" : > "http://host:8080/api/v1/clusters/c1/credentials/kdc.admin.credentials";, > "Credential" : { > "alias" : "kdc.admin.credentials", > "cluster_name" : "c1", > "persist" : true > } > } > ``` > ``` > 404 Not Found > { > "status": 404, > "message": "Not Found" > } > ``` > ``` > 403 Forbidden > { > "status": 403, > "message": "You do not have permissions to access this resource." > } > ``` > > > Diffs > ----- > > ambari-server/docs/api/v1/credential-create.md PRE-CREATION > ambari-server/docs/api/v1/credential-delete.md PRE-CREATION > ambari-server/docs/api/v1/credential-get.md PRE-CREATION > ambari-server/docs/api/v1/credential-list.md PRE-CREATION > ambari-server/docs/api/v1/credential-resources.md PRE-CREATION > ambari-server/docs/api/v1/credential-update.md PRE-CREATION > ambari-server/docs/api/v1/index.md c1e464c > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/CredentialResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > 1e219ff > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > 7bb0a72 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/CredentialService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > e3686ac > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 6ba6bac > > ambari-server/src/main/java/org/apache/ambari/server/controller/ClusterResponse.java > bb6d88e > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > a40fae6 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > a1cd5b8 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ResourceProviderFactory.java > 5d1143a > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java > 9163656 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterResourceProvider.java > 7e75a75 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 1b208fb > > ambari-server/src/main/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilter.java > 44c9613 > > ambari-server/src/main/java/org/apache/ambari/server/security/credential/Credential.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/credential/CredentialFactory.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/credential/GenericKeyCredential.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/credential/InvalidCredentialValueException.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/credential/PrincipalKeyCredential.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/AbstractCredentialStore.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialProvider.java > b812337 > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStore.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreService.java > 4aa3b0a > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceImpl.java > 968e96a > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/FileBasedCredentialStore.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/FileBasedCredentialStoreService.java > 41ff71b > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/InMemoryCredentialStore.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/InMemoryCredentialStoreService.java > 08d84fc > > ambari-server/src/test/java/org/apache/ambari/server/api/resources/CredentialResourceDefinitionTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/api/services/CredentialServiceTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java > 074fbb4 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > 23ce914 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CredentialResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UserResourceProviderTest.java > b0e1018 > > ambari-server/src/test/java/org/apache/ambari/server/security/authorization/AmbariAuthorizationFilterTest.java > 1824486 > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialProviderTest.java > ef1a9c8 > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceImplTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreServiceTest.java > 9725746 > > ambari-server/src/test/java/org/apache/ambari/server/security/encryption/CredentialStoreTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/38865/diff/ > > > Testing > ------- > > Units tests updated and passed > Manually testing in existing cluster (upgrade scenario) and new cluster > > # Local test results: > > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 54:46.952s > [INFO] Finished at: Tue Sep 29 18:02:43 EDT 2015 > [INFO] Final Memory: 66M/1534M > [INFO] > ------------------------------------------------------------------------ > > # Jenkins test results: > > Tests run: 3231, Failures: 0, Errors: 0, Skipped: 25 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 01:15 h > [INFO] Finished at: 2015-09-30T04:19:21+00:00 > [INFO] Final Memory: 48M/564M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Robert Levas > >
