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

Add oauth library #29

Merged
merged 28 commits into from
Mar 22, 2023
Merged

Add oauth library #29

merged 28 commits into from
Mar 22, 2023

Conversation

nsklikas
Copy link
Contributor

@nsklikas nsklikas commented Mar 3, 2023

Adds the library implementing https://github.com/canonical/charm-relation-interfaces/blob/main/interfaces/oauth/v0/README.md

TODO:

  • Add ca_chain to relation (Will be done in future PR)
  • More tests?
  • Do we want integration tests (Will be done in future PR)

lib/charms/hydra/v0/oauth.py Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@nsklikas nsklikas force-pushed the IAM-66-oauth-interface branch 2 times, most recently from b23827c to ec7b1f0 Compare March 6, 2023 09:18
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@nsklikas nsklikas force-pushed the IAM-66-oauth-interface branch 2 times, most recently from 3ead799 to a05d592 Compare March 7, 2023 11:45
@nsklikas nsklikas marked this pull request as ready for review March 7, 2023 11:48
@nsklikas nsklikas requested a review from a team as a code owner March 7, 2023 11:48
@nsklikas nsklikas requested a review from gruyaume March 7, 2023 11:48
lib/charms/hydra/v0/oauth.py Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Show resolved Hide resolved
tests/unit/test_oauth_requirer.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
@nsklikas
Copy link
Contributor Author

nsklikas commented Mar 7, 2023

There are some features missing from the current implementation, but I think it's best to add them in separate PRs in order not to overload this one:

  • Currently traefik does not pass its ca_chain from the relation interface, making it impossible to get the ca_chain. We will add that feature in the future.
  • When the relation is broken, the client should be deleted. This is not handled right now, as when we get the relation broken event we have no way of knowing which client we must delete. I think the most common solution to such a problem would be to use the peer relation to store a mapping from relation_id to client_id (@gruyaume any thoughts on this?).

@massigori
Copy link

Currently traefik does not pass its ca_chain from the relation interface, making it impossible to get the ca_chain. We will add that feature in the future.

I agree, let's keep this as a separate PR

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@nsklikas nsklikas force-pushed the IAM-66-oauth-interface branch 2 times, most recently from e0deaab to 4aa7d0e Compare March 9, 2023 12:09
@nsklikas nsklikas requested a review from gruyaume March 9, 2023 13:00
@nsklikas
Copy link
Contributor Author

On the requirer side do we want to emit an event once the relation is broken or are the charms expected to handle that on their own?

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@gruyaume
Copy link
Contributor

gruyaume commented Mar 14, 2023

There are some features missing from the current implementation, but I think it's best to add them in separate PRs in order not to overload this one:

  • Currently traefik does not pass its ca_chain from the relation interface, making it impossible to get the ca_chain. We will add that feature in the future.
  • When the relation is broken, the client should be deleted. This is not handled right now, as when we get the relation broken event we have no way of knowing which client we must delete. I think the most common solution to such a problem would be to use the peer relation to store a mapping from relation_id to client_id (@gruyaume any thoughts on this?).
  • It's fine not to add "everything" from a feature in a PR, as long as the code in the PR does what it's supposed to do and doesn't break the current behaviors. Make sure you have a user story that covers the changes that weren't included here. Either your current user story or a new one.
  • The question of the relation broken is a good one. I'm not a fan of having to maintain a mapping like this one and would avoid it if possible. In hydra when we create a client, we can choose its ID right? Can't we write a semantic ID based on the relation ID (and other stuff if necessary)?

@nsklikas
Copy link
Contributor Author

* The question of the relation broken is a good one. I'm not a fan of having to maintain a mapping like this one and would avoid it if possible. In `hydra` when we create a client, we can choose its ID right? Can't we write a semantic ID based on the relation ID (and other stuff if necessary)?

Unfortunately, there is no way in hydra to choose the client_id. The reasoning behind it is that they create client_ids using some hash function to optimize SQL queries for deployments with millions (?) of clients. I tried to think of a way to avoid having to use the peer relation but I don't think we can (we could do a full diff, ie check the relations and compare them with the clients in the database, but that seems way worse).

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
tests/unit/test_oauth_provider.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
Fix return types, check leadership, convert SUPPORTED_SCOPES to list
tests/unit/test_oauth_provider.py Outdated Show resolved Hide resolved
lib/charms/hydra/v0/oauth.py Outdated Show resolved Hide resolved
@nsklikas
Copy link
Contributor Author

I tried to refactor the tests and improve their naming. Please have another go at reviewing it.

Copy link
Contributor

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on the unit tests, they look much cleaner. I just gave 1 comment on coupling

tests/unit/test_oauth_requirer.py Outdated Show resolved Hide resolved
@nsklikas nsklikas requested a review from gruyaume March 22, 2023 12:48
Copy link
Contributor

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@nsklikas nsklikas merged commit 8bdd2f9 into main Mar 22, 2023
@nsklikas nsklikas deleted the IAM-66-oauth-interface branch March 22, 2023 15:26
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.

4 participants