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

RBAC server implementation documentation #8886

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Annaseli
Copy link
Contributor

@Annaseli Annaseli commented Mar 27, 2025

Closes https://github.com/treeverse/product/issues/684

Change Description

Added documentation for RBAC server implementation. The documentation includes the following:

  1. A step-by-step guide for implementing the RBAC server, available in: docs/security/RBAC-server-implementation.md.
  2. Descriptions for all APIs and schemas in the api/authorization.yml file, provided under the description fields within the YAML.
  3. A Swagger-style UI to visualize the authorization.yml file.:
    image
    To support this, the following files were added:
    • docs/security/authorization-yaml.md
    • docs/_includes/authorization.html
    • docs/assets/js/swagger-ui-authorization.js
    • docs/assets/js/authorization.yml

@Annaseli Annaseli requested review from nopcoder, guy-har and a team March 27, 2025 13:58
Copy link

github-actions bot commented Mar 27, 2025

🎊 PR Preview 21dc28c has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8886.surge.sh

🕐 Build time: 0.011s

🤖 By surge-preview

Copy link

E2E Test Results - Quickstart

12 passed

Copy link

github-actions bot commented Mar 27, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed, 1 skipped

@Annaseli Annaseli added docs Improvements or additions to documentation include-changelog PR description should be included in next release changelog labels Mar 27, 2025
@N-o-Z
Copy link
Member

N-o-Z commented Mar 27, 2025

@Annaseli Is this ready for review?
If so can you remove it from draft please

@Annaseli
Copy link
Contributor Author

@Annaseli Is this ready for review? If so can you remove it from draft please

@N-o-Z
Guy asked me to keep it in draft for now, as he wants to make sure it’s fully ready for review.

Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

added some comments - partial review will do it in parts

parameters:
PaginationPrefix:
description: Indicates the prefix that all returned items must start with. lakeFS uses this value to filter
Copy link
Contributor

Choose a reason for hiding this comment

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

Relevant to the complete file - remove the use of lakeFS in the description as it makes the spec focus on lakeFS service while it is important to describe the use-case without specifying the component name.

Copy link
Contributor Author

@Annaseli Annaseli Mar 30, 2025

Choose a reason for hiding this comment

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

@nopcoder
I removed all mentions of lakeFS except for this one:

- in: query
   description: Used only in lakeFS Enterprise; not applicable in the lakeFS OSS version.
   name: external_id
   schema:
   type: string

Should I replace "Used only in lakeFS Enterprise; not applicable in the lakeFS OSS version." with "For internal use only"?

max_per_page:
type: integer
minimum: 0
description: Maximal number of entries per page
description: Maximum number of entries per page. Not currently used, so it can be set to 0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maximum number of entries per page. Set to zero when not in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder
I wanted to note that while we expect this Pagination schema in the response, we don’t actually use the max_per_page parameter. Should I update the description to:
“Maximum number of entries per page. Not used internally.”?

external_id:
type: string
description: For internal usage. Can be empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

The field is not required so no need to add the description for this one or the other that it can be empty - unless empty value "" have a meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nopcoder
Should I write: "description: For internal use only."
or would it be better to remove the description completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

general description at the level of the operation/api will explain that the data should be loaded/stored. on the field itself if we want to specify what the field does we can - in this case we use the field for external system association.

@Annaseli Annaseli marked this pull request as ready for review March 30, 2025 16:49
@Annaseli
Copy link
Contributor Author

@nopcoder
Should I push the fixes I made based on your comments now, or wait until you've finished the full review to avoid disrupting the review process?

@nopcoder
Copy link
Contributor

feel free to push updates, I'll restart tomorrow

@Annaseli Annaseli force-pushed the docs/document-RBAC-implementation-684 branch from 21ea119 to ce2843e Compare April 1, 2025 08:53
@Annaseli Annaseli requested a review from nopcoder April 1, 2025 09:04
Comment on lines +42 to +43
description: Specifies the number of items the server should return. It is used to determine how many results to
display on the current page.
Copy link
Contributor

Choose a reason for hiding this comment

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

without the "display" and "current page" - we describe API and not the usage

encryptedPassword:
deprecated: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this is new? we no longer use this field?

Comment on lines 415 to +423
- in: query
deprecated: true
name: id
allowEmptyValue: true
schema:
type: integer
format: int64
- in: query
deprecated: true
Copy link
Contributor

Choose a reason for hiding this comment

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

If the deprecation is new I suggest to do it in a non documentation PR

name: email
allowEmptyValue: true
schema:
type: string
- in: query
description: Used only in lakeFS Enterprise; not applicable in the lakeFS OSS version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Used by the implementor to link external system

- `PUT /auth/policies/{policyId}`
- `DELETE /auth/policies/{policyId}`

### 2. LakeFS Configuration
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
### 2. LakeFS Configuration
### 2. lakeFS Configuration

rbac: internal
api:
endpoint: {ENDPOINT_TO_YOUR_RBAC_SERVER} # e.g., http://localhost:9006/api/v1
token:
Copy link
Contributor

Choose a reason for hiding this comment

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

sample of how token looks like or comment

Comment on lines +119 to +120
> The `auth.api.token` parameter is optional. If unspecified, lakeFS uses the `auth.encrypt.secret_key` as
> the token. If specified, provide a JWT token directly or via the environment variable `LAKEFS_AUTH_API_TOKEN`.
Copy link
Contributor

Choose a reason for hiding this comment

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

secret_key is not used as token as it doesn't leave the local environment.
explain the process of how the request get token when no explicit token is specified.

Comment on lines +124 to +126
When lakeFS starts for the first time, it initializes users, groups, and policies. Once initialized,
the authorization method cannot change. If lakeFS starts without an RBAC server and later tries connecting to one,
it will fail to authenticate. You must re-initialize lakeFS from scratch to connect to a new RBAC server.
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear - if we must "re-initialize" it means we can initialize and we said that lakeFS done it automatically.

  • verify that lakeFS create something on startup without explicit command
  • if we instruct the user to "re-iinitialize" we need to explain how

2. Run lakeFS with the updated configuration file:

```shell
./lakefs -c {PATH_TO_YOUR_CONFIG_FILE} run
Copy link
Contributor

Choose a reason for hiding this comment

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

Running a command similar to ./lakefs -c config.yaml run provides more detailed output.
Or note to remember to replace "config.yaml" with the actual path to your configuration file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants