search

common-fate / granted

The easiest way to access your cloud.
https://granted.dev
MIT License
956 stars 90 forks source link

Cleanup profile registry implementation + improve testability #622

Closed chrnorm closed 3 months ago

chrnorm commented 3 months ago

What changed?

Refactors the profile registry implementation to improve testability and split the git operations from the AWS config file merging logic. In general this makes the registry code a bit easier to reason about.

This PR also deprecates the --write-on-sync-failure flag for granted profile registry add. We've implemented a very sensible default behaviour for handling failures in #569. I don't see any use cases where continuing to write the AWS config file despite a syncing error would be desirable, and it simplified the implementation to remove it.

I've marked the flag as deprecated - it now returns an error with a link to our GitHub issues tracker if it is used, so we can take feedback and quickly revert if we have broken a workflow here.

Why?

We're considering adding support for an HTTP registry backend in addition to the existing git backend. Our main use case at Common Fate for such a backend is to incorporate a profile registry into our access platform, so that teams using Granted with Common Fate do not have to maintain an additional profile registry. This will also allow Granted users some additional flexibility, as you could implement your own HTTP registry backend by adhering to the API specification.

We're not finalised on a specification here for the HTTP registry backend API and will raise this in a follow-up PR. This PR is to tidy up the existing implementation, so that implementing an additional registry backend does not cause issues with the current git backend. We do not have any plans to deprecate or remove the current git backend either.

How did you test it?

Tested locally + added new unit tests.

Potential risks

Impact to profile registry feature

Eddie023 commented 3 months ago

🙌🏼 Looking good! I did a quick local testing and all seems to work except https://docs.commonfate.io/granted/usage/profile-registry#user-specific-values which seems to be broken in the main branch as well.

sosheskaz commented 3 months ago

HTTP backend seems like a nice idea.

I would ask that it be an open specification, so that it can be linked up to custom logic, potentially even for generating the registry on-the-fly. When using a programmatically-generated registry, this would allow us to define the registry as a service, instead of only in code, which unlocks exciting new paths. I'd imagine this would also be of benefit for common-fate integration as well.

If we go that route, I'd ask for hard API backward-compatibility, strong versioning, and swagger from the start.

chrnorm commented 3 months ago

Thanks @Eddie023 @sosheskaz. Regarding the potential HTTP backend I certainly agree on the open specification. Our Common Fate access platform would then simply implement the API specification.