feat(core): Support external configuration using Spring Cloud Config #1368
Conversation
There was a problem hiding this comment.
This looks good, with a few small comments.
CC @KathrynLewis who may want to take a look to make sure the changes involving SecretSessionManager look good.
| @@ -55,7 +54,7 @@ public void validate(ConfigProblemSetBuilder p, AbstractCanaryAccount n) { | |||
| if (StringUtils.isNotEmpty(usernamePasswordFile)) { | |||
| String usernamePassword = validatingFileDecrypt(p, usernamePasswordFile); | |||
|
|
|||
| if (Strings.isNullOrEmpty(usernamePassword)) { | |||
| if (usernamePassword != null && StringUtils.isEmpty(usernamePassword)) { | |||
There was a problem hiding this comment.
Shouldn't this && be an || to keep the current behavior (or was the change deliberate)?
There was a problem hiding this comment.
It is hard to guess intent, but here goes...
I'd guess that it should not be && because that leaves the 'else if' condition the chance of throwing an NPE when usernamePassword is null. Making it || is redundant since StringUtils.isEmpty checks for null and "". Given the context, the only thing that makes sense is for it to just be StringUtils.isEmpty(usernamePassword) unless the surrounding code is further modified. I'm going to make that change.
| @@ -7,15 +7,12 @@ | |||
| import com.netflix.spinnaker.halyard.config.model.v1.node.Validator; | |||
| import com.netflix.spinnaker.halyard.config.model.v1.providers.dcos.DCOSCluster; | |||
| import com.netflix.spinnaker.halyard.config.problem.v1.ConfigProblemSetBuilder; | |||
| import com.netflix.spinnaker.halyard.core.secrets.v1.SecretSessionManager; | |||
| import org.springframework.beans.factory.annotation.Autowired; | |||
| import org.apache.commons.lang.StringUtils; | |||
There was a problem hiding this comment.
We generally use org.apache.commons.lang3 (something must be pulling the lang version onto the classpath though).
This PR adds the following features related to external configuration using Spring Cloud Config Server (see the Spinnaker Config Secrets design doc)
configserver:, it doesn't convert to an absolute local path${some.value}) that should be resolved via Config Server at service runtime, then live account validation is bypassedconfigserver:, then the contents of the file is not validated and live account validation is bypassedkubeconfigFilecontaining the prefixconfigserver:, then the contents of the file will be retrieved from a config server backend and used for the deploymentThis PR contains the following related refactorings:
Validatorsubclasses all use theSecretSessionManagerthat already existed in the base class instead of defining their ownSecretSessionManagerValidatingFileReader(viaValidator#validatingFileDecryptwhere possible)ValidatingFileReaderwas moved to eliminate a package cycle