-
Notifications
You must be signed in to change notification settings - Fork 589
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
Add 2-step verification for POST requests. #582
Conversation
...rc/main/java/com/linkedin/kafka/cruisecontrol/servlet/parameters/StopProposalParameters.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch, left some comments.
One point about using UUID and/or session in the request that's related to 2-step verification.
When we want to check status of a request, can one just provide a reviewId?
Or if the user wants to check status of a old request with UUID in the header, will this request be handed off to purgatory?
...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/KafkaCruiseControlServlet.java
Outdated
Show resolved
Hide resolved
...java/com/linkedin/kafka/cruisecontrol/servlet/parameters/AddedOrRemovedBrokerParameters.java
Outdated
Show resolved
Hide resolved
...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/ReviewStatus.java
Show resolved
Hide resolved
cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java
Show resolved
Hide resolved
cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/Purgatory.java
Show resolved
Hide resolved
...se-control/src/main/java/com/linkedin/kafka/cruisecontrol/servlet/purgatory/RequestInfo.java
Show resolved
Hide resolved
d8475df
to
4721643
Compare
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Left some comments.
* The Purgatory is thread-safe. | ||
*/ | ||
public class Purgatory implements Closeable { | ||
private static final String FINAL_REASON = "Submitted approved request."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can move this FINAL_REASON
into RequestInfo
and simplify RequestInfo.submitReview(int reviewId, String reason)
to RequestInfo.submitReview(int reviewId)
} | ||
|
||
if (requestInfo.status() == ReviewStatus.SUBMITTED && LOG.isDebugEnabled()) { | ||
LOG.info("Request {} has already been submitted (review: {}).", requestInfo.endpointWithParams(), reviewId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG.info
-> LOG.debug
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentionally info
level -- i.e. we would like to keep track of consecutive accesses -- removed && LOG.isDebugEnabled()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is gated by LOG.isDebugEnabled()
, so this information will not get logged unless the verbose level is DEBUG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should not have had LOG.isDebugEnabled()
, which was removed when I addressed the above feedback -- thanks for catching that.
} | ||
// Apply review to each Request Info | ||
ReviewStatus targetReviewStatus = entry.getKey(); | ||
requestIds.forEach(requestId -> _requestInfoById.get(requestId).applyReview(targetReviewStatus, reason)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we return only what is approved
/discarded
if approve
/discard
parameter is set for review
endpoint, and return the whole list if no parameter it set?
|
||
Integer reviewId = Integer.parseInt(request.getParameter(parameterString)); | ||
// Sanity check: Ensure that if a review id is provided, no other parameter is in the request. | ||
if (request.getParameterMap().keySet().size() != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request.getParameterMap().keySet().size()
-> request.getParameterMap().size()
@@ -31,6 +31,7 @@ | |||
* &json=[true/false]&skip_hard_goal_check=[true/false]&excluded_topics=[pattern] | |||
* &use_ready_default_goals=[true/false]&verbose=[true/false]&exclude_recently_demoted_brokers=[true/false] | |||
* &exclude_recently_removed_brokers=[true/false]&replica_movement_strategies=[strategy1,strategy2...] | |||
* &review_id=[id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing review_id=[id]
for Decommission a broker
section in document above.
@@ -31,6 +31,7 @@ | |||
* &json=[true/false]&skip_hard_goal_check=[true/false]&excluded_topics=[pattern] | |||
* &use_ready_default_goals=[true/false]&verbose=[true/false]&exclude_recently_demoted_brokers=[true/false] | |||
* &exclude_recently_removed_brokers=[true/false]&replica_movement_strategies=[strategy1,strategy2...] | |||
* &review_id=[id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I am thinking should we mention the fact that review_id
parameter is exclusive with other parameter, put something like
POST /kafkacruisecontrol/add_broker?brokerid=[id1,id2...]&dryRun=[true/false]...
Or POST /kafkacruisecontrol/add_broker?review_id=[id]
in Javadoc.
(same apply to other POST endpoint parameters)
} | ||
|
||
private String getJSONString() { | ||
List<Map<String, Object>> jsonRequestInfoList = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize with size of _requestInfoById.size()
.
@@ -295,10 +298,33 @@ synchronized void checkActiveUserTasks() { | |||
} | |||
} | |||
|
|||
private synchronized void removeFromPurgatory(UserTaskInfo userTaskInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether there is a reason why we keeps submitted items in purgatory until the task expires in UserTaskManager
. looks to me it is enough that purgatory only maintains item in Pending
and Approved
status, once an item it discarded or submitted we can directly recycle it in purgatory.
Currently there are 2 place saves the information about approved request information, purgatory
and UserTaskManager
. If we want to know what tasks are approved, we can directly go to USER_TASK
endpoint since we know if and only if a POST request is approved, it will be executed and we can get more information there(like execution result).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought a task can only be in UserTaskManager when it is approved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing up these points -- let me clarify:
Submitted tasks are in purgatory to ensure auditability, abstraction, and to ensure that if the request has already been submitted, the user is not attempting to create another user task with the same parameters and endpoint [added this sanity check in the latest commit]. The auditability lets us know details on an already executed request -- e.g. who submitted the request and when. The abstraction helps us filter out access to UserTasks in purgatory-level.
if (requestInfo.accessToAlreadySubmittedRequest() | ||
&& _userTaskManager.getUserTaskByUserTaskId(_userTaskManager.getUserTaskId(request), request) == null) { | ||
throw new UserRequestException( | ||
String.format("Attempt to start a new user task with an already submitted review. If you are trying to retrieve" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this Exception message seems to be confusing under certain scenario.
Currently the default retention time for Purgatory
is 14 days and the retention time for UserTaskManager
is 1 day, so if a user want to retrieve (with UUID and reviewID) a task which was submitted 2 days, the user will get the this Exception and get confused(since he already provide the UUID).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see UserTaskManager#removeFromPurgatory, which removes request from purgatory if the task expires.
Let me know if I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Addresses the issue #567.
Pending: Documentation on how to use two-phase verification.