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

chore: hydra DSN is empty, maester in singleNamespaceMode with Role #509

Merged
merged 3 commits into from
Sep 13, 2022
Merged

chore: hydra DSN is empty, maester in singleNamespaceMode with Role #509

merged 3 commits into from
Sep 13, 2022

Conversation

roko99
Copy link
Contributor

@roko99 roko99 commented Sep 9, 2022

Related Issue or Design Document

  1. Hydra: to not pass DSN environment variable, when .Values.hydra.config.dsn isn't specified. fix: Do not pass DSN if it's empty #416
  2. Hydra Maester: to not create ClusterRole, when singleNamespaceMode is enabled, add relevant permission to Role.
  3. Hydra Maester: remove .Release.Namespace suffixes for Roles/RoleBindings as they are already per namespace (to unify naming).

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation within the code base (if appropriate).

Further comments

DSN change.
When .Values.hydra.config.dsn isn't specified - DSN environment variable is passed into the pod with empty value, as it is read from secret, dns key (it is created in any case, with or without value). My suggestion is to not pass DSN environment variable into the pod, when it is empty, and allow user overwrite it with extraEnv. If user specifies dsn in .Values.hydra.config.dsn it will behave as it does now.

Hydra Maester changes.

  • We don't need ClusterRole when singleNamespaceMode is enabled, but relevant permissions should be grant to the Role. In our case we are using multiple hydras per environment/namespaces - and if we try to keep unified naming in namespaces, it cause conflicts in ClusterRole names.
  • Also I've removed namespace suffixes for Roles/RoleBinding, as they are created per namespace.

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2022

CLA assistant check
All committers have signed the CLA.

@roko99 roko99 mentioned this pull request Sep 9, 2022
5 tasks
Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

Hi there,
The change is great in its simplicity :) I am still conflicted regarding passing the empty DSN, as the contract was that we expect it to be provided by the user. This however is still valid as hydra won't start without it, but the way we provide it changes.

Please add a segment to the hydra document with an example :)

@roko99
Copy link
Contributor Author

roko99 commented Sep 12, 2022

Hello @Demonsthere
Yes, I agree, that we expect user to provide dsn configuration. And when user doesn't do that application will fail and it is expected. My suggestion is to give people some flexibility with their configuration (basically as my team needs to integrate it with postgres operator 😄, as well as in related PR guys are looking for such opportunity) and not to break existing functionality.

Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

LGTM :)

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

3 participants