-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: Add oidc integration example for nextcloud (#252) #253
Conversation
Thanks for your contribution on the first glance it already looks very promissing. We might want to wrap the authentication into a parent object, so we can also support SAML2.0 and make it the interface future proof. Edit: As OICD is a separated app the configuration of a potential SAML 2.0 configuration can be in a different location. As @kosmoz will be able to review the PR next Wednesday the earliest it might take a little longer this time to get the interface right :-). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jens and thank you for your contribution! 🚀
I think it makes a lot of sense to support OIDC configuration for the tools that support it. This looks like a good start, however I have some suggestions and questions below.
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/NextcloudAppsSpec.kt
Outdated
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/NextcloudAppsSpec.kt
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudDeployment.kt
Outdated
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudDeployment.kt
Outdated
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudDeployment.kt
Outdated
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudDeployment.kt
Outdated
Show resolved
Hide resolved
6acebb4
to
8bc1697
Compare
I just iterated once more over this PR and hope that I met your ideas. Once again, I really appreciate a review and will iterate once more if desired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
I tested this feature using a local Keycloak and Nextcloud instance and it basically worked out of the box (see below)! ✨
There are still some smaller improvements/suggestions I would like to propose before we merge this (open for discussion, see below).
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudConfigMap.kt
Outdated
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/dependent/NextcloudDeployment.kt
Show resolved
Hide resolved
operator/src/main/kotlin/eu/glasskube/operator/apps/nextcloud/NextcloudAppsSpec.kt
Show resolved
Hide resolved
6c7762a
to
e7ab565
Compare
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Moreover, read clientId and clientSecret from a Kubernetes secret and feed it into the nextcloud pod as an environment variable. Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
…#252) Signed-off-by: Jens Schneider <jens.schneider.ac@posteo.de>
e7ab565
to
e3b360f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This PR introduces a very simple example how the integration of oidc could be done. Please let me know what you think about having secret data directly in the
Nextcloud
resource and theDeployment
. This PR is meant as a starting point for discussion and implementation.