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

IAM-224-Oauth interface implementation #278

Merged
merged 21 commits into from
Jan 18, 2024

Conversation

bencekov
Copy link
Contributor

Issue

Grafana is Oauth 2.0 capable, but the charm currently can't use the Identity Platform's oauth interface.

Solution

This pr implements integration with the oauth interface, and includes unit and integration tests to verify correct behavior.

Context

Relevant projects are:
playwright
Identity bundle
Hydra charm

Testing Instructions

Use tox for both unit and integration test.

Release Notes

@bencekov bencekov force-pushed the IAM-224-implement-oauth-interface branch from 5c28065 to 9c3f993 Compare November 24, 2023 10:44
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

I think that the oauth_tools.constants.OAUTH_RELATION var should be removed. It is used in too many places and I find it confusing.

Also not sure if all of the helper functions make much sense, e.g. I would expect access_application_login_page to simply navigate me to a page, but it also expects a redirect to happen. This seems to specific to this use case and I am not sure if we need to abstract it.

tests/integration/oauth_tools/oauth_test_helper.py Outdated 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
src/charm.py Outdated Show resolved Hide resolved
tests/integration/oauth_tools/dex.py Outdated Show resolved Hide resolved
tests/integration/test_grafana_oauth.py Outdated Show resolved Hide resolved
tests/integration/test_grafana_oauth.py Show resolved Hide resolved
tests/integration/oauth_tools/oauth_test_helper.py Outdated Show resolved Hide resolved
tests/integration/oauth_tools/oauth_test_helper.py Outdated Show resolved Hide resolved
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 5, 2023
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 5, 2023
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 5, 2023
refactor: refactors made on comments on pr canonical#278
@bencekov bencekov force-pushed the IAM-224-implement-oauth-interface branch from fe4458f to 2c24f25 Compare December 5, 2023 20:03
@bencekov bencekov requested a review from nsklikas December 6, 2023 18:36
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 6, 2023
feat: added settings for enabling refresh tokens
refactor: refactors made on comments on pr canonical#278
@bencekov bencekov force-pushed the IAM-224-implement-oauth-interface branch from e4e77f2 to c43b0be Compare December 6, 2023 18:58
@bencekov bencekov marked this pull request as ready for review December 6, 2023 18:58
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only some minor comments.

tests/integration/test_grafana_oauth.py Outdated Show resolved Hide resolved
.github/workflows/pull-request.yaml Outdated Show resolved Hide resolved
.github/workflows/release.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/integration/oauth_tools/constants.py Outdated Show resolved Hide resolved
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 8, 2023
bencekov added a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 8, 2023
@bencekov bencekov force-pushed the IAM-224-implement-oauth-interface branch 3 times, most recently from 273027e to f537bf3 Compare December 8, 2023 11:29
@nsklikas nsklikas force-pushed the IAM-224-implement-oauth-interface branch from 5d59ee0 to fdef95c Compare December 13, 2023 11:30
nsklikas pushed a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 13, 2023
feat: added settings for enabling refresh tokens
refactor: refactors made on comments on pr canonical#278
nsklikas pushed a commit to bencekov/grafana-k8s-operator that referenced this pull request Dec 13, 2023
@nsklikas nsklikas force-pushed the IAM-224-implement-oauth-interface branch 4 times, most recently from 45e9af1 to 9a9be8b Compare December 13, 2023 12:48
@nsklikas nsklikas force-pushed the IAM-224-implement-oauth-interface branch from 38fa097 to 6eef76b Compare January 15, 2024 14:28
natalian98 and others added 20 commits January 15, 2024 16:29
pass oauth config as env variables

fix mypy issue

address review comments
send new client config to oauth relation on config_changed event

address review comments

fix lint
fix pyright check error

fix relation-broken event, change oauth provider name

fix integration tests

fix rebase issue
feat: added settings for enabling refresh tokens
refactor: refactors made on comments on pr canonical#278
The ca certs were updated after the services restarted
@nsklikas nsklikas force-pushed the IAM-224-implement-oauth-interface branch from 6eef76b to 1d047b3 Compare January 15, 2024 14:29
@nsklikas nsklikas force-pushed the IAM-224-implement-oauth-interface branch from 1d047b3 to 24283e1 Compare January 15, 2024 14:54
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

couple of minor recommendations/clarification requests from my side, rest looks good.

src/charm.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@lucabello lucabello merged commit 451f87d into canonical:main Jan 18, 2024
12 checks passed
@sed-i
Copy link
Contributor

sed-i commented May 22, 2024

Just for reference:

graph LR
hydra ---|pg-database:database| postgresql-k8s
kratos ---|pg-database:database| postgresql-k8s
kratos ---|endpoint-info| hydra
kratos-external-idp-integrator ---|kratos-external-idp| kratos
hydra ---|admin-ingress:ingress| traefik-admin
hydra ---|public-ingress:ingress| traefik-public
kratos ---|admin-ingress:ingress| traefik-admin
kratos ---|public-ingress:ingress| traefik-public
identity-platform-login-ui-operator ---|ingress| traefik-public
identity-platform-login-ui-operator ---|endpoint-info| hydra
identity-platform-login-ui-operator ---|ui-endpoint-info| hydra
identity-platform-login-ui-operator ---|ui-endpoint-info| kratos
identity-platform-login-ui-operator ---|kratos-endpoint-info| kratos
identity-platform-admin-ui-operator --- oathkeeper
identity-platform-admin-ui-operator --- hydra
identity-platform-admin-ui-operator --- kratos
openfga --- postgresql-k8s
openfga --- identity-platform-admin-ui-operator
openfga --- identity-platform-login-ui-operator
Loading

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.

9 participants