Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup profile registry implementation + improve testability #622

Merged
merged 6 commits into from Apr 3, 2024

Conversation

chrnorm
Copy link
Contributor

@chrnorm chrnorm commented Mar 14, 2024

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
Copy link
Collaborator

🙌🏼 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
Copy link
Contributor

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
Copy link
Contributor Author

chrnorm commented Apr 2, 2024

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.

@shwethaumashanker shwethaumashanker merged commit 1ce074b into main Apr 3, 2024
3 checks passed
@chrnorm chrnorm deleted the cleanup-profile-registry branch April 3, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants