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

HDDS-9203. Allow generating/revoking S3 secret for other users via REST #5233

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

ivanzlenko
Copy link
Contributor

What changes were proposed in this pull request?

Add an ability to generate or revoke secret for custom user using S3 REST.

What is the link to the Apache JIRA

Please replace this section with the link to the Apache JIRA)](https://issues.apache.org/jira/browse/HDDS-9203)

How was this patch tested?

Patch is tested manually, via unit and smoke tests.


@POST
@Path("/{username}")
public Response generate(@PathParam("username") String username)
Copy link
Contributor

@ChenSammi ChenSammi Aug 31, 2023

Choose a reason for hiding this comment

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

The current URL and HTTP action of each operation is
POST /secret/generate
POST /secret/generate/testuser2
POST /secret/revoke
POST /secret/revoke/testuser2

For a REST API, it usually leverages HTTP operations instead of put the operation/action name in the URL. I'd like to recommend change the URL and HTTP action to,
PUT /secret/
PUT /secret/testuser2
DELETE /secret/
DELETE /secret/testuser2

@ivanzlenko , the rest looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChenSammi thanks! I've changed this API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, does it make sense to add "Administration of S3 Secret" on this page https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/security/securings3.html ? If that's the case then I create a follow ticket to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's good to have a page for this new RESTful solution.

There is one Security section on this page https://ci-hadoop.apache.org/view/Hadoop%20Ozone/job/ozone-doc-master/lastSuccessfulBuild/artifact/hadoop-hdds/docs/public/interface/s3.html. about the CLI S3 Secret Management. It's better to co locate the new content with it. Maybe we can move the Security section content from s3.html to the Securing S3.html, then add a link in s3.html to refer to the new location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivanzlenko ivanzlenko requested a review from ChenSammi August 31, 2023 10:34
import javax.ws.rs.core.Response;
import java.io.IOException;

import static javax.ws.rs.core.Response.Status.NOT_FOUND;

/**
* Revoke secret endpoint.
* Endpoint to generate and return S3 secret.
Copy link
Contributor

@ChenSammi ChenSammi Sep 5, 2023

Choose a reason for hiding this comment

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

Endpoint to generate and return S3 secret. ->
Endpoint to manage S3 secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ivanzlenko ivanzlenko requested a review from ChenSammi September 5, 2023 14:40
@ChenSammi ChenSammi merged commit 4ee6c07 into apache:master Sep 6, 2023
@ChenSammi
Copy link
Contributor

Thanks @ivanzlenko for the contribution.

@ivanzlenko ivanzlenko deleted the HDDS-9218 branch September 6, 2023 08:46
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.

2 participants