feat(rfc): externalized account management

[ncknt]

Consistent account management across services and providers in Spinnaker.

[ezimanyi]

I think this would be great; both moving the repository to be per credential type (and thus strongly-typed) as well as making it possible to store these in some other persistence store.

I've definitely wanted for a while to move at least Kubernetes credentials to SQL for the reasons you enumerate here; this work would lay the groundwork for being able to do that.

[robzienert]

I like the idea, long overdue.

[robzienert]

I'm interested to hear more about why we need a distinction between Account and Credentials. Do we have an opportunity to simplify things?

[ncknt]

Account is supposed to be a POJO that can be read from simple properties. Credentials is what services use and may also have injected beans (e.g. KubernetesNamedAccountCredentials use a factory. NetflixAmazonCredentials uses a AWSCredentialsProvider). By separating account source (list of props) from credentials, it becomes easy to create new sources without having to worry about what else is needed.

We could do without Account but then each time you want to add a new source, you have to deal with the wiring. Alternatively, we could move the beans outside of the Credentials object but that would be a bigger change.

[ajordens]

I think there's a lot of clumsiness in the current implementation and split between Account/Credentials. It's made deserialization tricky (number of places that need to be changed to introduce a new attribute) and has been a sore spot for some time now.

I don't think it's a terribly clean distinction right now, feels like the wiring could be handled but I've not looked through every implementation.

[ncknt]

It's true that there's a lot of repetition in some of the providers. Unless we do a major redesign and move all managed beans out of these credentials objects, we will always need to enrich these beans. The same class could optionally represent Account and Credentials and keeping the same abstraction, AccountLifecyleHandler could be in charge of autowiring beans. We could even provide a default handler that does the autowiring.

e.g.

class NetflixAmazonCredentials extends AmazonCredentials implements Account {
	...
}

class AwsLifecycleHandler extends AutowiringLifecycleHandler<NetflixAmazonCredentials> {
	
	accountAdded(NetflixAmazonCredentials cred) {
	    // autowire beans
		super.accountAdded(cred)
		// enable agents etc.
	} 
	...
}

I was hoping to start small (easy migration to Account and Credentials) and refactors could be kept for later.

[robzienert]

I was hoping to start small [...] and refactors could be kept for later.

This is a statement I've been seeing a lot more of lately in the community and I think we can do better. I appreciate the sentiment of making forward movement, but I have not seen frequent evidence that these smaller changes are followed up with a well-considered design and subsequent refactors. We definitely want to make incremental improvement and we definitely want to keep forward momentum; but more importantly, we want to make forward-looking, incremental improvement drawing on lessons we've already learned. We should be striving to make fundamental fixes with these proposals.

Account and credential management, especially in Clouddriver, have been long-identified as problematic and non-ergonomic, so I'm confused why we wouldn't want to tackle these problems immediately while we're in here, rather than copy-forward things we already know were mistakes. Doubly so, considering this code is a proposal for standardizing the way of representing account management across services, it's concerning we'd choose to implement the design knowing it isn't what we want.

Is there a path forward that we can think about what we want account and credential management to be, and work backwards from there to figure out what is possible in the short-term, and have a clear vision of where we're headed? I'd feel much more comfortable with incremental improvements when the destination is known, rather than incremental improvements without a known destination.

[ncknt]

I was hoping to start small (easy migration to Account and Credentials) and refactors could be kept for later.

In the context of the comment, I meant implement the described design in smaller incremental change (by provider) - the goal of the document doesn't change.

I didn't feel the RFC's stated goals would be controversial but I'm okay revising them. I just don't understand what I'm missing. Adam had a good point (perhaps we don't need Account and Credentials) and I was proposing a way forward that remains manageable.

We're committed to implementing the change for Kubernetes and AWS providers (likely more providers and services) but we won't do all services and providers so we need an intermediary solution.

it's concerning we'd choose to implement the design knowing it isn't what we want

I'm happy with the proposal. I took Adam's comment in the context of the AWS provider, because as far as I can tell other providers don't suffer from the many layers of abstractions that the AWS provider has.

Is there a path forward that we can think about what we want account and credential management to be, and work backwards from there [...] ?
I'd feel much more comfortable with incremental improvements when the destination is known

That's how I (think I) worked. I want each type of account to be independently managed, I want the effort to adopt this model to be low so all providers can get onboard, and I want to do it across services.

Edit: answered the actual questions

[robzienert]

I have no issues with the proposed goals, like I said earlier, I think this is a great idea and long overdue! I misinterpreted the context of your comment, and thought we were taking a step back. I apologize.

It is unclear to me what the actual proposed work is for the AWS provider, since that one is evidently the worst provider in terms of account mgmt. I see a bunch of work done for Kubernetes in your branch, but nothing for AWS - are we planning to rip out what is there and improve, or add this stuff on top?

[ncknt]

I just wanted to define the framework here and leave the discussion for each provider into the PRs that will ensue. I believe you (Netflix) have additional glue code around Spinnaker and I didn't want to break anyone's environment. But I'd be happy to simplify further.

[ajordens]

Things may get clearer in the draft PR if we can just be consistent about what we are calling things.

public interface CredentialsRepository<T extends Credentials> {
  T getOne(String key);
  Set<T> getAll();
  T save(String key, T account);
  T update(String key, T account);
  void delete(String key);
  String getType();
}

Minor point but potentially leading to confusion is the somewhat arbitrary use of credentials vs account. To someone not intimately familiar with the distinction, this will stand out. I think it's largely and easily correctable.

As a user of all this, I still don't think I should care about a distinction between Account and Credentials. Even more so looking across different Spinnaker services (clouddriver has made a mess of this but it's not as bad in igor right?).

I may be beating a dead horse here and not saying anything materially different than the actual intent of this work.

[ncknt]

That is a good point. I actually changed the words in the middle of the RFC - hence the remaining account in the code snippet above. I used to have Account as AccountProperties (and Credentials as Account).

Would AccountProperties (or AccountInformation) be more explicit? Naming things is hard.

Edit: I've updated the branch to be more consistent with the terms it uses.

[robzienert]

uses credentials across credentials

I don't understand this part.

[ncknt]

Sorry there's a typo! What I meant is that the elasticsearch module seems to need to access credentials across providers - or at least it seemed to but I've never used it so it may very well be wrong. I took it as an exercise: what if we need to access multiple types of credentials? Hence the CompositeCredentialsRepository.

[ajordens]

The clouddriver-elasticsearch module may not be all that relevant, I don't believe it's widely/if-at-all used outside of Netflix for entity tags.

There is some lingering experimental indexing code that I suspect can be rm -rf'd ... years ago we experimented with maintaining a separate index in elasticsearch but it didn't go anywhere. Removing this code will likely simplify this composite conversation.

[ajordens]

Things may get clearer in the draft PR if we can just be consistent about what we are calling things.

public interface CredentialsRepository<T extends Credentials> {
  T getOne(String key);
  Set<T> getAll();
  T save(String key, T account);
  T update(String key, T account);
  void delete(String key);
  String getType();
}

Minor point but potentially leading to confusion is the somewhat arbitrary use of credentials vs account. To someone not intimately familiar with the distinction, this will stand out. I think it's largely and easily correctable.

As a user of all this, I still don't think I should care about a distinction between Account and Credentials. Even more so looking across different Spinnaker services (clouddriver has made a mess of this but it's not as bad in igor right?).

I may be beating a dead horse here and not saying anything materially different than the actual intent of this work.

[ajordens]

Is that preferable? We've got a weird mix of APIs that sometimes include account + provider and sometimes just account.

Historically I've come down on the side of appreciating uniqueness in account names (but I could possibly be swayed here).

[ncknt]

I couldn't spot where the account was ambiguous if you know the provider (or the implied provider based on where you are in Deck or in the code). It clearly doesn't mean there aren't any!

I think it is important to allow different providers to use different sources. For instance, some users store their Kubernetes account information in a separate system but have a handful of manually configured AWS accounts that remain in Spring props.

The non uniqueness is a by-product of having separate sources of accounts. Currently, I am not even sure what happens when the same name is used in the different providers. The last one wins? And it probably changes if accounts are dynamic. Regardless, it's probably not what the user expects.

IMO we should make sure that the provider is always displayed next to the account (in Deck) or present in the URL in APIs.

[ajordens]

I didn't think clouddriver would even allow you to have duplicate account names across providers (we have two cloud providers that ultimately land resources in the same aws account but we give them distinct names.

[ncknt]

I believe you can configure it that way (in the yaml files) and I don't believe you'd get an error, just unexpected behavior.

[ncknt]

One thing that I forgot to explicitly call out is that this is not just about creating accounts but also providing stable account management interfaces for users.

Yes we could simplify to: class MyCredentials { // autowired props and beans } but as soon as properties don't come from Spring, users will need to build MyCredentials objects and manually add the beans and there's no stability guarantee. I realize that this could also break if fields are removed from account definitions but that's very rarely happens.

That explains the AccountParser and CredentialsLifecycleHandler abstractions. In reality these are tiny interfaces.

I realize that there's also the issue of the AWS provider which has 5 credentials classes and a complicated loader mechanism. Ideally we'd keep that discussion for the AWS provider PR but for what it's worth, I share the concern that it doesn't look easily maintainable. I can also give it a try on that not-for-real PR if it helps clarify the end state.

[ajordens]

Putting the prototypical PR aside, I'm a +1 with this proposal. Let's do it!

I think any concerns I've had are more related to potential implementations that can be tackled at that point.

[robzienert]

+1 on the proposal.

[plumpy]

Yeah, I'd love to look into the PR more, but as a high-level proposal, this sounds like a great idea, thanks.

[ethanfrogers]

Sorry for the review delay. I didn't realize I was holding us up. +1, I'm excited to see this come to life as well.