-
Notifications
You must be signed in to change notification settings - Fork 11
Chart 1.5.0- Fluffy EOL #343
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
Conversation
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.
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, that's a lot of stuff, looks good small comments!
Also plz attach examples of values to use :)
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! We're getting there!
Left small comments + whatever unresolved
charts/lakefs/templates/secret.yaml
Outdated
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: ldap-secret |
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.
No need in additional secret just add that to the lakefs-secret
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.
I don't want to use the lakefs-secret because it depends on .Values.secrets
.Values.existingSecret
, which complicates things and makes it harder for the user to configure
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.
Let's continue discussion in the OIDC duplicated comment
charts/lakefs/templates/secret.yaml
Outdated
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: oidc-client-secret |
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.
No need in additional secret just add that to the lakefs-secret
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.
I don't want to use the lakefs-secret because it depends on .Values.secrets
.Values.existingSecret
, which complicates things and makes it harder for the user to configure
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.
But that's the whole point, all the idea is if that we either create a secret or allow using an existing secret, meaning they created themselves outside of the chart.
If you notice that's the pattern of this chart (and in many other charts, including our own cloud chart).
Both in OSS and in Fluffy previously, single secrets.
Now it can be unified which makes it even simpler.
Multiple secrets == More Complexity!
You only do that when you must, for example dockerRegistry and saml ceritficates are 2 common things that are cluster wide and shared so they are separate.
P.S, going on a per "value" secret does not make sense keeping the original secret holding the db and encryption key already, so if we change the pattern it should be modified. (I vote 👎 for this approach)
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! see unresolved comments
- returnTo | ||
- https://<lakefs.ingress.domain>/oidc/login | ||
|
||
useDevPostgres: true |
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.
Why in all the examples useDevPostgres
is true
?
If for some reason it's needed then let's revert the decision on making the default false
otherwise, remove.
charts/lakefs/templates/secret.yaml
Outdated
{{- if (((.Values.enterprise).auth).oidc).enabled }} | ||
{{- if (((.Values.enterprise).auth).oidc).clientSecret }} | ||
# LDAP bind password secret, used for LDAP authentication | ||
ldap_bind_password: {{ .Values.enterprise.auth.ldap.bindPassword | b64enc }} |
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.
Notice anything strange between the if condition and the secret value? 🙃
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.
LGTM! 💃
Let's get to docs! :)
Blocked by: #345 |
CHANGELOG.md
Outdated
# 1.5.0 | ||
|
||
### Important | ||
The lakeFS Helm chart now uses lakeFS Enterprise with integrated authentication, removing the need for the separate Fluffy service. |
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.
Additionally please be explicit about:
- fluffy is not longer supported in the chart
- lakeFS-Enterprise image is now the default for this to work for enterprise if you previously used different image must change upgrade to target version
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.
👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏
👏🐶👏👏👏👏👏👏👏🐶🐶🐶🐶👏👏🐶🐶🐶🐶🐶👏👏🐶👏👏👏🐶👏
👏🐶👏👏👏👏👏👏🐶👏👏👏👏👏👏👏👏🐶👏👏👏👏🐶🐶👏🐶🐶👏
👏🐶👏👏👏👏👏👏🐶👏👏🐶🐶👏👏👏👏🐶👏👏👏👏🐶👏🐶👏🐶👏
👏🐶👏👏👏👏👏👏🐶👏👏👏🐶👏👏👏👏🐶👏👏👏👏🐶👏🐶👏🐶👏
👏🐶🐶🐶🐶🐶👏👏👏🐶🐶🐶🐶👏👏👏👏🐶👏👏👏👏🐶👏👏👏🐶👏
👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏👏
What I checked:
.Values.fluffy
returns an error.