feat(core): Update account management API #5666
Conversation
|
The following commits need their title changed:
Please format your commit title into the form:
This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here. |
|
Yeah yeah yeah, bot, I'll be squashing this PR into one commit. As for the test failure, this is strange as I didn't get that locally. I'm not sure how that happened :/ |
...river-core/src/main/groovy/com/netflix/spinnaker/clouddriver/config/CloudDriverConfig.groovy
Show resolved
Hide resolved
...src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionSecretManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Integration tests are failing because of a dependency version mismatch for io.zipkin.brave:brace-instrumentation-http: some part of the code is using version 5.12.3 and other part is using 5.10.1. Not sure if this is a result of the recent version bump (and then revert) of spring boot: spinnaker/kork#941
cc @dbyron-sf
| return true; | ||
| } | ||
| var userRoles = authorizer.getRoles(username); | ||
| if (userRoles.isEmpty()) { |
There was a problem hiding this comment.
What happens if Spinnaker doesn't have authn or authz enabled? As I understand, userRoles will be empty and this method returns false. Does that have unwanted side effects?
There was a problem hiding this comment.
If Fiat is disabled, isAdmin returns true, so this code doesn't get executed. Good catch, though!
@j-sandy is working on a fix for the brave version. More details here, but I expect a fix to come in kork. Moving to enforcedPlatform (e.g. here in igor) is what started the struggles. |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
Hey @jvz did you mention some deck changes that go along with changing the type discriminator? Or is it that deck works better but there aren't any changes? |
|
I said I changed the type discriminator in this PR which goes with the Gate PR, and it now matches how the existing type discriminator works in Deck (no changes to be made there). |
This refactors some things in preparation for user secrets support along with updating the type discriminator handling for account definitions.
| @ConditionalOnMissingBean | ||
| public AccountDefinitionAuthorizer accountDefinitionAuthorizer( | ||
| Optional<FiatPermissionEvaluator> maybePermissionEvaluator, | ||
| @Value("${services.fiat.enabled:false}") boolean isFiatEnabled) { |
There was a problem hiding this comment.
I'd love to pass a configuration object here instead of @Value but I'm not sure such an object exists today...
There was a problem hiding this comment.
Well, there's the object where the evaluator comes from, but that wasn't enough of a hint here.
* Extract AccountDefinitionService class * feat(core): Update account management API This refactors some things in preparation for user secrets support along with updating the type discriminator handling for account definitions. * fix(test): Ensure Fiat is enabled with bean Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Extract AccountDefinitionService class * feat(core): Update account management API This refactors some things in preparation for user secrets support along with updating the type discriminator handling for account definitions. * fix(test): Ensure Fiat is enabled with bean Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Extract AccountDefinitionService class * feat(core): Update account management API This refactors some things in preparation for user secrets support along with updating the type discriminator handling for account definitions. * fix(test): Ensure Fiat is enabled with bean Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
This refactors some things in preparation for user secrets support along with updating the type discriminator handling for account definitions.
These changes are independent of spinnaker/fiat#928 and spinnaker/kork#942, and I've noted TODO comments on where relevant updates related to those PRs will be added (with additional tests).