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

External auth example #245

Merged
merged 8 commits into from
Apr 14, 2024
Merged

External auth example #245

merged 8 commits into from
Apr 14, 2024

Conversation

idanovo
Copy link
Contributor

@idanovo idanovo commented Apr 10, 2024

Closes #247

@idanovo idanovo self-assigned this Apr 10, 2024
Comment on lines 61 to 63
valid_sts_hosts:
- "sts.amazonaws.com"
- "sts.us-east-1.amazonaws.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for that, let's leave it with the default in fluffy (all)

Comment on lines 55 to 56
required_headers:
required-key: "custom-value"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder maybe it's better to use the ingress host provided to add X-LakeFS-Server-ID ? it will save us a lot of typos and troubleshooting with clients. so by default configure required_headers with X-LakeFS-Server-ID: <lakefs.ingress.domain> using helpers and environment variables in the chart. Only if the client configures here in the values required_headers it will override it! Thoughts?

Suggested change
required_headers:
required-key: "custom-value"
# required_headers:
# required-key: "custom-value"
# this header added by default in all clients to make sure one does authenticate with staging server and then reuses the token in production, so each request should be done per host
# X-LakeFS-Server-ID: <lakefs.ingress.domain>

examples/lakefs/enterprise/values-external-aws.yaml Outdated Show resolved Hide resolved
type: local
auth:
authentication_api:
endpoint: http://localhost:8080/api/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this still here?

Comment on lines 49 to 51
#valid_sts_hosts:
# - "sts.amazonaws.com"
# - "sts.us-east-1.amazonaws.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

that's really an advanced use case for someone to use that, i think its confusing, remove

Comment on lines 43 to 44
#required_headers:
# x-lakefs-custom-key: "custom-value"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#required_headers:
# x-lakefs-custom-key: "custom-value"
# headers that must be present by the client when doing login request
required_headers:
# same host as the lakeFS server ingress
X-LakeFS-Server-ID: <lakefs.ingress.domain>

Comment on lines 2 to 5
logging:
level: "INFO"
blockstore:
type: local
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Comment on lines 33 to 35
logging:
format: "json"
level: "INFO"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@@ -53,6 +53,10 @@ env:
- name: LAKEFS_AUTH_UI_CONFIG_LOGOUT_URL
value: /logout
{{- end }}
{{- if (.Values.fluffy.sso).enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think that this if is not required because there's one in L35 that wraps it and ends in L60

please verify me

{{- if (.Values.fluffy.sso).enabled }}
- name: LAKEFS_AUTH_AUTHENTICATION_API_ENDPOINT
value: {{ printf "http://%s/api/v1" (include "fluffy.ssoServiceName" .) | quote }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

bump Chart.yaml version

Copy link
Contributor

@Isan-Rivkin Isan-Rivkin left a comment

Choose a reason for hiding this comment

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

beautiful

@idanovo idanovo merged commit fdc0d74 into master Apr 14, 2024
3 checks passed
@idanovo idanovo deleted the external-auth-example branch April 14, 2024 07:27
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.

Chart to External Principals
2 participants