You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by GitBox <gi...@apache.org> on 2021/05/07 09:30:19 UTC
[GitHub] [brooklyn-server] zan-mateusz opened a new pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
zan-mateusz opened a new pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171
SMART-153 Notes/Testing done
Original behaviour:
- Brooklyn does not start when vault is set up but sealed or not running
Implementation notes:
- Megan not prevented from starting up with vault sealed/unavailable.
- Attempt to recover from bad state done when vault is accessed (either during deployment or while app running)
- Deployment/Effector fails if vault unavailable
- Re-try mechanism that tries to recover from a bad state in 1s intervals, up to a specified number of times.
- Number of retries specified in a new property config as below, optional with a default value of 10.
"brooklyn.external.vault.recoverTryCount=X"
TESTING
1. Startup
1.1 Vault unavailable
* login successful
* token set to ""
1.2 Vault running and sealed
* login successful
* token set to ""
1.3 Vault running and unsealed
* login successful
* token pulled from vault
2. Deployment with secret pulled from vault on start
2.1 Vault unavailable at point of deployment, tested with all 3 login scenarios as per 1.
* deployment fails
2.2 Vault running and sealed at point of deployment, tested with all 3 login scenarios as per 1.
* deployment fails
2.3 Vault running and unsealed at point of deployment, tested with all 3 login scenarios as per 1.
* successful recovery from bad state
* deployment successful
* secret pulled from vault correctly
2.4 Vault restarted, running and unsealed, tested with 1.3 (token different after restart)
* successful update of token
* deployment successful
* secret pulled from vault correctly
2.5 Vault becomes available (unsealed) while retrying, tested with 1.1 and 1.2
* successful recovery from bad state
* deployment successful
* secret pulled from vault correctly
3. Accessing vault secret while application is running - stop effector
3.1 Vault unavailable at point of invocation
* operation fails
3.2 Vault running and sealed at point of invocation
* operation fails
3.3 Vault running and unsealed at point of invocation
* effector successful
* secret pulled from vault correctly
3.4 Vault restarted, running and unsealed (different token than during deployment)
* successful update of token
* effector successful
* secret pulled from vault correctly
--
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
[GitHub] [brooklyn-server] asfgit merged pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171
--
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
[GitHub] [brooklyn-server] ahgittin commented on pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
ahgittin commented on pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#issuecomment-840518109
looks great. i prefer the new spacing.
some fyi comments but gonna merge this before you feel the urge to do anything about them (because it's probably not worth it right now!)
--
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
[GitHub] [brooklyn-server] zan-mateusz commented on pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
zan-mateusz commented on pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#issuecomment-840500264
> code looks good to me. nice work.
>
> not tested by me however.
I have made some minor changes as discussed - formatting with IntelliJ (note couple of unrelated changes stemming from me using intelliJ reformatter, I considered them OK so left them in) - also changed method used for sleep between retries.
The behaviour has been manually tested by me, I tried to play around with tests in VaultExternalConfigSupplierLiveTest but these require manual steps as well, automating this is painful and probably not worth the time investment.
--
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
[GitHub] [brooklyn-server] zan-mateusz commented on a change in pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
zan-mateusz commented on a change in pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#discussion_r631773429
##########
File path: core/src/main/java/org/apache/brooklyn/core/config/external/vault/VaultExternalConfigSupplier.java
##########
@@ -84,18 +86,19 @@ public VaultExternalConfigSupplier(ManagementContext managementContext, String n
this.version = -1; // satisfy the static analysis :)
errors.add("'kv-api-version' must be either 1 or 2");
}
- recoverTryCount = NumberUtils.toInt(config.get("recoverTryCount"),10);
+ recoverTryCount = NumberUtils.toInt(config.get("recoverTryCount"), 10);
mountPoint = config.get("mountPoint");
if (Strings.isBlank(mountPoint) && this.version == 2) errors.add("missing configuration 'mountPoint'");
- if (!Strings.isBlank(mountPoint) && this.version == 1) errors.add("'mountPoint' is only applicable when kv-api-version=2");
+ if (!Strings.isBlank(mountPoint) && this.version == 1)
+ errors.add("'mountPoint' is only applicable when kv-api-version=2");
Review comment:
thanks for the feedback, it was not actually me to make the change but intelliJ and its formatter, I personally actually find multiple line with even with no braces easier to read than a single line, but as mentioned this is personal preference really :) I will keep that in mind for future, thanks!
--
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
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#discussion_r631767284
##########
File path: core/src/main/java/org/apache/brooklyn/core/config/external/vault/VaultExternalConfigSupplier.java
##########
@@ -81,16 +86,21 @@ public VaultExternalConfigSupplier(ManagementContext managementContext, String n
this.version = -1; // satisfy the static analysis :)
errors.add("'kv-api-version' must be either 1 or 2");
}
+ recoverTryCount = NumberUtils.toInt(config.get("recoverTryCount"), 10);
mountPoint = config.get("mountPoint");
if (Strings.isBlank(mountPoint) && this.version == 2) errors.add("missing configuration 'mountPoint'");
- if (!Strings.isBlank(mountPoint) && this.version == 1) errors.add("'mountPoint' is only applicable when kv-api-version=2");
+ if (!Strings.isBlank(mountPoint) && this.version == 1)
Review comment:
there is some ugly old code here, using `this.version` as an `int` but `String version` local var shadowing it. not your fault but if you encounter it and hate it as much as me feel free to improve in future!
--
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
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#discussion_r631766664
##########
File path: core/src/main/java/org/apache/brooklyn/core/config/external/vault/VaultExternalConfigSupplier.java
##########
@@ -131,27 +134,21 @@ public String get(String key) {
protected JsonObject apiGetRetryable(String path, Map<String, String> headers, int recoverTryCount) {
try {
- if (Strings.isBlank(headers.get("X-Vault-Token"))){
+ if (Strings.isBlank(headers.get("X-Vault-Token"))) {
String currentToken = initAndLogIn(config);
- if (Strings.isBlank(currentToken)){
+ if (Strings.isBlank(currentToken)) {
throw new IllegalStateException("Vault sealed or unavailable.");
}
- headers = MutableMap.copyOf(headers).add("X-Vault-Token",currentToken);
+ headers = MutableMap.copyOf(headers).add("X-Vault-Token", currentToken);
}
return apiGet(path, headers);
- }
- catch (Exception e){
+ } catch (Exception e) {
Review comment:
this (new way) is what almost everyone on the project does
--
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
[GitHub] [brooklyn-server] ahgittin commented on a change in pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
ahgittin commented on a change in pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#discussion_r631766404
##########
File path: core/src/main/java/org/apache/brooklyn/core/config/external/vault/VaultExternalConfigSupplier.java
##########
@@ -84,18 +86,19 @@ public VaultExternalConfigSupplier(ManagementContext managementContext, String n
this.version = -1; // satisfy the static analysis :)
errors.add("'kv-api-version' must be either 1 or 2");
}
- recoverTryCount = NumberUtils.toInt(config.get("recoverTryCount"),10);
+ recoverTryCount = NumberUtils.toInt(config.get("recoverTryCount"), 10);
mountPoint = config.get("mountPoint");
if (Strings.isBlank(mountPoint) && this.version == 2) errors.add("missing configuration 'mountPoint'");
- if (!Strings.isBlank(mountPoint) && this.version == 1) errors.add("'mountPoint' is only applicable when kv-api-version=2");
+ if (!Strings.isBlank(mountPoint) && this.version == 1)
+ errors.add("'mountPoint' is only applicable when kv-api-version=2");
Review comment:
we're well in to the realm of personal preference here (and not worth changing) but most people in the project prefer braces
```
if (x) {
errors.add(...);
}
```
for short things i and some people prefer one line
```
if (x) errors.add(...);
```
multi-line without braces (your change here) is not very common.
--
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
[GitHub] [brooklyn-server] ahgittin commented on pull request #1171: SMART-153 - Fixes around sealed/unavailable vault
Posted by GitBox <gi...@apache.org>.
ahgittin commented on pull request #1171:
URL: https://github.com/apache/brooklyn-server/pull/1171#issuecomment-840428055
code looks good to me. nice work.
not tested by me however.
--
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