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

[cleanup][proxy] Remove authorization config #19797

Closed

Conversation

nodece
Copy link
Member

@nodece nodece commented Mar 13, 2023

Motivation

In #1002, authorization is no longer done by proxy, so I clean up the proxy authorization.

Modifications

  • Remove superUserRoles, authorizationEnabled, and authorizationProvider from the proxy config
  • Update the authorization tests

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 13, 2023
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece force-pushed the remove-authorization-conf-from-proxy branch from 4ee0086 to 8bde2e7 Compare March 15, 2023 07:58
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Does passing unknown key-value pairs cause fatal? I'm afraid that users will continue using existing conf file that contains now removed key value pair.

@nodece
Copy link
Member Author

nodece commented Mar 21, 2023

Does passing unknown key-value pairs cause fatal? I'm afraid that users will continue using existing conf file that contains now removed key value pair.

No impact. Users can continue the old config, the proxy didn't use these configs.

@nodece nodece self-assigned this Mar 21, 2023
@nodece nodece added this to the 3.0.0 milestone Mar 21, 2023
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

I see that the proxy itself does not use the AuthorizationService. However, proxy extensions can use the AuthorizationService, so I don't think we can remove it without discussion on the mailing list. I know, for example, that the DataStax project starlight-for-kafka uses the AuthorizationService here. That being said, I could see an argument for removing the AuthorizationService from the proxy and then we can expect extension maintainers to build their own AuthorizationService.

@nodece
Copy link
Member Author

nodece commented Mar 22, 2023

Good catch! I didn't notice the proxy extensions, I can add the @Deprecated to the these fields and methods.

What do you think of that? @michaeljmarshall

@michaeljmarshall
Copy link
Member

I thought that might work, but @eolivelli thinks we should keep it around. It sounds like the kind of decision to make on the mailing list.

@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label May 11, 2023
@nodece nodece closed this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants