You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/02/12 19:56:01 UTC
[GitHub] [knox] pzampino opened a new pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
pzampino opened a new pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
URL: https://github.com/apache/knox/pull/264
… without synchronization
## What changes were proposed in this pull request?
Added synchronization to getCredentialForCluster(String, String) since it uses/updates the cache (e.g., checkCache(String, String), addToCache(String, String, String)). At a minimum, this avoids potential repeated unnecessary keystore file accesses for the same alias.
## How was this patch tested?
Performed some manual testing, but since the overall behavior should not change, ran all existing tests as validation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
URL: https://github.com/apache/knox/pull/264#discussion_r378491444
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##########
@@ -309,25 +309,30 @@ public void addCredentialForCluster(String clusterName, String alias, String val
@Override
public char[] getCredentialForCluster(String clusterName, String alias)
throws KeystoreServiceException {
- char[] credential = checkCache(clusterName, alias);
- if (credential == null) {
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- if (ks != null) {
- try {
- char[] masterSecret = masterService.getMasterSecret();
- Key credentialKey = ks.getKey( alias, masterSecret );
- if (credentialKey != null) {
- byte[] credentialBytes = credentialKey.getEncoded();
- String credentialString = new String( credentialBytes, StandardCharsets.UTF_8 );
- credential = credentialString.toCharArray();
- addToCache(clusterName, alias, credentialString);
+ char[] credential;
+
+ synchronized (this) {
Review comment:
Instead of synchronizing the whole block wouldn't it be better to add something like a ReadWriteLock ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino merged pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
URL: https://github.com/apache/knox/pull/264
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #264: KNOX-2233 - DefaultKeystoreService getCredentialForCluster uses cache…
URL: https://github.com/apache/knox/pull/264#discussion_r378492834
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##########
@@ -309,25 +309,30 @@ public void addCredentialForCluster(String clusterName, String alias, String val
@Override
public char[] getCredentialForCluster(String clusterName, String alias)
throws KeystoreServiceException {
- char[] credential = checkCache(clusterName, alias);
- if (credential == null) {
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- if (ks != null) {
- try {
- char[] masterSecret = masterService.getMasterSecret();
- Key credentialKey = ks.getKey( alias, masterSecret );
- if (credentialKey != null) {
- byte[] credentialBytes = credentialKey.getEncoded();
- String credentialString = new String( credentialBytes, StandardCharsets.UTF_8 );
- credential = credentialString.toCharArray();
- addToCache(clusterName, alias, credentialString);
+ char[] credential;
+
+ synchronized (this) {
Review comment:
The intention here is to complete the existing synchronization pattern. More drastic changes merit a separate JIRA IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services