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

Allow users to configure authentication provider #22

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

indigo423
Copy link
Contributor

Created schema validation for the configuration of OAuth and OIDC authentication providers.

Resolve: #21

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, IGNORE.
  • All commits have DCO signoffs.

@indigo423 indigo423 force-pushed the config/oauth branch 3 times, most recently from 3ba3cd6 to 2977722 Compare October 7, 2024 09:24
charts/perses/Chart.yaml Outdated Show resolved Hide resolved
@indigo423 indigo423 force-pushed the config/oauth branch 3 times, most recently from 601d31f to 7b2dea8 Compare October 7, 2024 11:01
@Labiote
Copy link

Labiote commented Oct 8, 2024

Hello, great, I was waiting for this and actually thinking about contributing myself ahah. Although, shouldn't you add changes for Values.yaml file that include all that changes ?

Also, there is an ongoing issue regarding authentication and tls :

perses/perses#2302

That will probably imply some changes in the authentication config :

perses/perses#2311

Maybe it is worth checking it and including the changes in this issue ?

@indigo423
Copy link
Contributor Author

Can you help me understand this a bit better what do you expect to see in the values.yaml file?

Although, shouldn't you add changes for Values.yaml file that include all that changes ?

Sorry, I'm a non-native English speaker and there might be some loss in translation for me :)

Also, there is an ongoing issue regarding authentication and tls :

perses/perses#2302

That will probably imply some changes in the authentication config :

perses/perses#2311

Maybe it is worth checking it and including the changes in this issue?

I'll check it out. Thank you for pointing these out, this is great.

@Labiote
Copy link

Labiote commented Oct 8, 2024

No worries ! So the values.yaml file has some default configuration in it that can be overridden when using the chart. Maybe it would be nice to increment the file with the new values you added regarding authentication to give some hints on what people can override when they use this chart, something like :

[...]
  security:
    # -- Configure Perses instance as readonly
    readOnly: false
    # -- Enable Authentication
    enableAuth: false
    # -- cookie config
    cookie:
      same_site: lax
      secure: false
      # [ same_site: < enum | possibleValue = 'strict' | 'lax' | 'none' > | default = lax ]
      # Set to true if you host Perses behind HTTPS. Default is false
      # [ secure: <boolean> | default = false ]
    # -- Authentication configuration
    authentication:
      # -- Define your providers
      providers:
        oidc:
          ...
        oauth:
          ...

etc
[...]

I don't know if this is clear ^^'

@indigo423
Copy link
Contributor Author

Ok got it, as it is right now, users won't know what's supported from a configuration perspective. I think the README file uses the values.yaml file to generate the "chart.valuesSection", which is just a subset of the configuration defined in the values.schema.json and the template.

@indigo423
Copy link
Contributor Author

indigo423 commented Oct 20, 2024

@Nexucis @Labiote I would like to get some feedback from you about this topic. The main question is about having default settings for authentication providers in the values.yaml file. The values.yaml is used to render the documentation in the README file. That would definitely help to make it transparent, so people know what can be configured. The downside of this method, people wo don't use authentication provider are forced to override the values which adds a bit of friction, because default values probably don't match any deployment scenario.

Do we have other options to a) provide a documention which shows easily what you can configure and b) having a reasonable default values.yaml which works frictionless?

Please don't hesitate ripping this comment apart :)

@indigo423
Copy link
Contributor Author

I think this issue in helm-docs is related to the problem I've described: norwoodj/helm-docs#225

@indigo423 indigo423 closed this Oct 24, 2024
@indigo423 indigo423 reopened this Oct 24, 2024
@indigo423 indigo423 force-pushed the config/oauth branch 2 times, most recently from cbd9555 to 7492c39 Compare October 24, 2024 09:29
@nicolastakashi
Copy link
Contributor

nicolastakashi commented Oct 28, 2024

@indigo423 amazing, thanks for handling this.
Can you also bump version on chart.yaml and update Helm Read me by running make update-helm-readme

Allow users to configure OAuth and OIDC authentication provider in the helm chart. Add validation in the schema.json.

Signed-off-by: Ronny Trommer <[email protected]>
Signed-off-by: Ronny Trommer <[email protected]>
@nicolastakashi nicolastakashi marked this pull request as ready for review October 28, 2024 13:59
@indigo423
Copy link
Contributor Author

@indigo423 amazing, thanks for handling this. Can you also bump version on chart.yaml and update Helm Read me by running make update-helm-readme

Thanks for the hint, done

@nicolastakashi nicolastakashi merged commit 22d6b24 into perses:main Oct 28, 2024
2 checks passed
@indigo423 indigo423 deleted the config/oauth branch October 28, 2024 16:36
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.

Allow users to configure authentication provider
4 participants