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

[kratos] Add Ory Kratos service #266

Merged
merged 12 commits into from
Jun 18, 2024
Merged

[kratos] Add Ory Kratos service #266

merged 12 commits into from
Jun 18, 2024

Conversation

pvannierop
Copy link

Description of the change

This PR will add the Ory Kratos service. The Ory Kratos service will be the new solution for User and Identity management in RADAR-base.

@pvannierop pvannierop self-assigned this May 29, 2024
@pvannierop pvannierop changed the base branch from main to dev May 29, 2024 09:42
@pvannierop pvannierop force-pushed the ory-kratos branch 4 times, most recently from 00fa56e to bdc9be3 Compare May 29, 2024 10:12
@@ -0,0 +1,85 @@
bases:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Ory is needed for all installations let's add it to 10-base.yaml instead of having its own separate file

Copy link
Author

Choose a reason for hiding this comment

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

Can't the same be said about Management Portal? It is in its own file. As an alternative, we can rename 10-managementportal.yaml to 10-authentication.yaml or 10-identity-management.yaml and add both MP and Ory-Kratos. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I'd like to reduce number of helmfiles and merge them whenever it makes sense. I'm fine with changing the names, maybe we can merge both MP and Ory into 10-base.yaml.
If you won't want to put them into 10-base.yaml, since MP and appconfig aren't only about authentication or identity management I think we should have a more descriptive name.

Choose a reason for hiding this comment

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

IMO, put it in 10-base.yaml.

app-config is the most optional but you can still disable it. I don't think there is a big benefit to it being in a separate helmfile

Copy link
Author

Choose a reason for hiding this comment

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

Merged into 10-base.yaml!

Copy link
Author

Choose a reason for hiding this comment

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

Because postgres is difined in the 10-managementporta.yaml file, I moved kratos to this helmfile.

helmfile.d/15-ory.yaml Outdated Show resolved Hide resolved
etc/base.yaml Outdated Show resolved Hide resolved
helmfile.d/15-ory.yaml Outdated Show resolved Hide resolved
helmfile.d/15-ory.yaml Outdated Show resolved Hide resolved
etc/base.yaml Outdated Show resolved Hide resolved

courier:
smtp:
from_address: [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be configurable

Copy link
Author

Choose a reason for hiding this comment

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

But it is configurable, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is but I think it should be in base.yaml since it will be changed in every installation.

Choose a reason for hiding this comment

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

agreed

Copy link
Author

Choose a reason for hiding this comment

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

Ok the most simple solution would be to write this entry in base.yaml like so:

kratos:
  _install: true
  _chart_version: 0.43.0
  _extra_timeout: 0
  ....
  kratos:
    courier:
      smtp:
        from_address: [email protected]

However, since SMTP settings are also used by MP (and possibly other services), we should consider defining this as global properties and inject this in the set: section of helmfile where needed.

etc/kratos_ui/values.yaml Outdated Show resolved Hide resolved

releases:
- name: kratos
chart: ory/kratos
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please mirror the chart in external directory of radar-helm-charts and refer it from there similar to other external charts

Copy link
Author

Choose a reason for hiding this comment

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

I copied the Ory helm charts the radar-helm-charts repo. The PR is here. I updated the chart references in 15-ory.html accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, please also do a quick check if kratos can be successfully installed from our mirror

Choose a reason for hiding this comment

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

@mpgxvii Did you ever install kratos using the latest radar-kubernetes and helm chart?

@pvannierop pvannierop requested a review from keyvaann June 11, 2024 11:25
@keyvaann
Copy link
Collaborator

@pvannierop
Copy link
Author

@keyvaann Yes, the problem was that we moved the kratos definitions to the 10-base.yaml file, but the postgres dependency was only defined in 10-managementportal.yaml. I corrected this and now it installs fine.

@keyvaann keyvaann merged commit 8c85447 into dev Jun 18, 2024
4 of 6 checks passed
@keyvaann keyvaann deleted the ory-kratos branch June 18, 2024 10:43
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