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

NAS-133206 / 25.10 / Convert boot.* to versioned API #15524

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

Conversation

creatorcary
Copy link
Contributor

@creatorcary creatorcary commented Jan 29, 2025

@creatorcary creatorcary requested a review from a team January 29, 2025 21:16
@bugclerk
Copy link
Contributor

@bugclerk bugclerk changed the title Convert boot.* to versioned API NAS-133206 / 25.10 / Convert boot.* to versioned API Jan 29, 2025
@themylogin
Copy link
Contributor

@yocalebo I see a little bit of roles chaos here. How are they assigned? Should they be assigned here at all?

@creatorcary
Copy link
Contributor Author

I see a little bit of roles chaos here. How are they assigned? Should they be assigned here at all?

As of #15347, endpoints that require auth must list roles. I do think that with this change we should better document our roles, maybe give each one a docstring describing specifically what each one is to be used for in role.py (The difference between read/write roles is self explanatory).

@anodos325
Copy link
Contributor

Removing roles from private endpoints goes beyond scope of this PR.

@creatorcary
Copy link
Contributor Author

creatorcary commented Jan 30, 2025

Removing roles from private endpoints goes beyond scope of this PR.

@anodos325 You're right, I've made a ticket to address this later https://ixsystems.atlassian.net/browse/NAS-133901. The refactor I'd like to make if it has enough support is to condense the roles, private, authorization_required and authentication_required kwargs into a single required keyword arg access with the following (exclusive) options from most to least restrictive:

  • "PRIVATE"
  • list of roles
  • "NO_AUTHZ_REQUIRED"
  • "NO_AUTH_REQUIRED"

which would eliminate all the messy and unclear relationships between our current kwargs.

@anodos325
Copy link
Contributor

Removing roles from private endpoints goes beyond scope of this PR.

@anodos325 You're right, I've made a ticket to address this later https://ixsystems.atlassian.net/browse/NAS-133901. The refactor I'd like to make if it has enough support is to condense the roles, private, authorization_required and authentication_required kwargs into a single required keyword arg access with the following (exclusive) options from most to least restrictive:

* "PRIVATE"

* list of roles

* "NO_AUTHZ_REQUIRED"

* "NO_AUTH_REQUIRED"

which would eliminate all the messy and unclear relationships between our current kwargs.

Okay. You'll need to approach that very carefully as changes can have subtle security implications.

@creatorcary
Copy link
Contributor Author

Okay. You'll need to approach that very carefully as changes can have subtle security implications.

Will definitely request your review on it

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.

4 participants