ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Levas" <rle...@hortonworks.com>
Subject Re: Review Request 38865: Create a credentials resource used to securely set, update, and remove credentials used by Ambari
Date Thu, 01 Oct 2015 12:37:48 GMT


> On Sept. 30, 2015, 11:20 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java,
lines 61-83
> > <https://reviews.apache.org/r/38865/diff/1/?file=1087423#file1087423line61>
> >
> >     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?
> 
> Nate Cole wrote:
>     I don't mean to barge in, but I've used com.google.common.collect.Sets.newHashSet(String...)
as a nice alternative to the compiler voodoo.  +1 to filling the map in a static{} block.
> 
> Robert Levas wrote:
>     I am not opposed to change this (and I will do so), but I see this _voodoo_ all over
the Ambari code. :)  Actaully, I thought this notation is nice, clear, and compact. 
>     
>     @ncole, thanks for barging in.  I will check out `com.google.common.collect.Sets.newHashSet(String...)`.

I went with expliciting the sets and map in a static block rather then using the Google common
collection class(es). This decision was made mainly because there was no newHashMap equivelent
to newHashSet, and I likye symmetry. :)


> On Sept. 30, 2015, 11:20 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CredentialResourceProvider.java,
line 127
> > <https://reviews.apache.org/r/38865/diff/1/?file=1087423#file1087423line127>
> >
> >     Null or also empty?

both, thanks.


> On Sept. 30, 2015, 11:20 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/security/encryption/CredentialStoreService.java,
line 120
> > <https://reviews.apache.org/r/38865/diff/1/?file=1087434#file1087434line120>
> >
> >     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.
> 
> Robert Levas wrote:
>     Good point. I didn't like this either, but it seemed to be the most efficient way
to do it.  I will fix.

Thanks for this comment. Becuase of this, I changed from using a simple (relatively unclear)
Boolean value to indicate which store to use; to using an explicit Enum. I like this approach
a lot better.


- Robert


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38865/#review101117
-----------------------------------------------------------


On Oct. 1, 2015, 8:33 a.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38865/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2015, 8:33 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/CredentialStoreType.java
PRE-CREATION 
>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message