[pulsar-admin] allow tenant admin to manage subscription permission

[rdhabalia]

Motivation

In #2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission.
So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user.

[rdhabalia]

rerun java8 tests

[jiazhai]

Since @tuteng and @wolfstudy are also doing some auth related improvement, added them into the reviewer.

[sijie]

Since we are changing it from super-user to tenant admin, can we please document this behavior change?

[rdhabalia]

@sijie
I don't think we add doc for such permission stuff. However, I have added as part of api doc.

[rdhabalia]

ping

[rdhabalia]

@sijie I think PR is blocked by you so, can you please review the PR.

[sijie]

@rdhabalia I think we should introduce v4 api and make the change there. I don't think it is a good idea to change the base which would impact existing v1, v2, and v3 api. we should try to avoid introducing breaking changes to old APIs.

regarding documentation, I meant to say adding a note to release note. because users should realize the behavior change. You can add a release note section for 2.6.0-SNAPSHOT in release notes and document this change. Otherwise, this can be missed in release notes in the future.

[rdhabalia]

I think we should introduce v4 api and make the change there. I don't think it is a good idea to change the base which would impact existing v1, v2, and v3 api.

This is not a breaking change but it was a bug. Owner-Tenant can already manage all property level access rights such as : granting produce/consume permission to other tenants. and we have also added support to let consumer manage cursor level api in #2981 and admin-tenant should be able to access this api to grant permission.
We should not introduce v4 api for this functionality, this is definitely not required.

You can add a release note section for 2.6.0-SNAPSHOT in release notes and document this change.

I think we add release not once we create a release. can you please point me which file should be updated with this documentation.

[sijie]

@rdhabalia I understand that's a bug. However this behavior has been there from v1 to v3. If this "buggy" behavior was exposed publicly to the user, that means some existing tools might already rely on this behavior. Changing such an API is breaking the behavior that people saws before. Hence I think a better option is to introduce a newer API to amend this buggy behavior.

For any behavior changes, it should be highlighted in a release note. I understand that we used t create release notes when cutting a release. But it has introduced many troubles to the release manager as well as for the users. Because it is hard for a release manager to grab all the changes at the time of cutting a release. Users are frustrated about the breaking changes that are not documented and have complained about the documentation here and there. Hence I would suggest we should adding a place to track breaking changes and behavior during developing a release. Adding a section for an under-developing release in release notes for accumulating breaking changes seems to be a good way to go. Because the release manager can find it when cutting a release and users can find potential breaking changes in the upcoming release.

I think the conversation here should be brought to the mailing list for a discussion and a potential vote since it is related to behavior change and also release process/document change.

[rdhabalia]

that means some existing tools might already rely on this behavior.

I don't understand what can it break? this is a new api and was added in 2.3. It is not a breaking change because It is not adding any restriction but adding the flexibility for owner to access this api.

[sijie]

It is 2.6.0 now. It has been 2 major releases since 2.3. If an API is exposed publicly, changing the behavior should be treated as a “breaking” change. Users should have been notified of this behavior change.

This change is not just about adding flexibility. From security point of view, this endpoint was only allowed for super-user but this change relaxes the permissions to a tenant admin. This is a red flag to a lot of enterprises. Because you are allowing a different set of people accessing the API. Correct me if I misunderstood this change here.

[rdhabalia]

From security point of view, this endpoint was only allowed for super-user but this change relaxes the permissions to a tenant admin.

This api was only accessed by super-admin because of concern of increasing metadata footprint size and I had added it into the code-comment which has been removed in this PR. also, it will not create any security concern because this will allow tenant to grant permission and this semantics already exists eg: tenant grants users produce/consume messages, tenant increase quota, etc.. tenant is already allowed to manage property/namespace permission. so, It should not create red flag for any enterprises and I can agree if there will be or if any else also suggests same.

[sijie]

@rdhabalia I understand the original concern about why only super-users can grant permission. However, that was the behavior released in the previous version. Writing a comment in code is different from writing that in the documentation and release notes. I am happy to see how other people feel about from the security perspective.

There are a lot of good articles on the internet talking about what is a breaking change for an API. (e.g. https://www.bennadel.com/blog/3501-when-is-a-change-a-breaking-change-for-an-api.htm)

At a minimum, changing a return code is commonly treated as a breaking change. In this case, when using versions older than 2.6.0, if non-super-user issues an HTTP request to this endpoint, the error is 401. After this change, if a tenant admin (who is not a super-user) issues an HTTP request to this endpoint, this returned code is 200. The return code is different. This should be treated as a breaking change. At a minimum, it should be highlighted in the release notes.

I think we should bring this conversation to the mailing thread to settle down a practice about how to define what are the breaking changes and how do we deal with them.

[rdhabalia]

This should be treated as a breaking change. At a minimum, it should be highlighted in the release notes.

Sure. We should document into release notes. I think we create release-note after the release if that's not correct and if already have document then can you please point me to that doc and I will update the release-note.

[sijie]

I think we create release-note after the release if that's not correct and if already have document then can you please point me to that doc and I will update the release-note.

I pointed it out in my previous comment. Currently, we don't accumulate these release notes during development. This causes trouble for release managers and technical writers. So I suggested adding a section to the release notes doc for documenting these behavioral changes. It can be "2.6.0-SNAPSHOT (under development)".

I have started a discussion in dev@ mailing list to formalize the process for PIP. I will raise the discussion of having a release notes section for the development version.

[tuteng]

ping @sijie @rdhabalia PTAL

[jiazhai]

Seems there is some discussion needed. @rdhabalia Do you think it is Ok to remove it from 2.5.1?

[rdhabalia]

Do you think it is Ok to remove it from 2.5.1?

we need this change soon but unfortunately, we are not able to merge some essential and useful features and their releases keep changing.. :-(

[sijie]

@rdhabalia I think my main concern is changing a permission level from super-user to tenant admin is a big breaking change. I am fine with such a change if we have it well documented. It is a bit bad that everyone just wants to get a chance in but didn't think carefully how this would impact other users in the community. I don't want to be the one who sounds preventing merging a change. so if you can document this well and highlight it in the release notes. I am fine with that change.

[Anonymitaet]

@rdhabalia thanks for your great work.

Could you please help answer the following questions?

1. I see the grant-subscription-permission command of namespaces has been already shown on Pulsar Admin doc.

Is that enough for your code change? Or it (they) should be added for tenant as well?

2. Same question for REST API doc: is the following enough?

[Anonymitaet]

Doc has been added as @sijie indicated that this item should be added to 2.5.1 release notes.
Done in #7276