Skip to content

feat(kafka acl): add acl delete command#1218

Merged
craicoverflow merged 6 commits intoredhat-developer:mainfrom
craicoverflow:delete-acls
Oct 19, 2021
Merged

feat(kafka acl): add acl delete command#1218
craicoverflow merged 6 commits intoredhat-developer:mainfrom
craicoverflow:delete-acls

Conversation

@craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Oct 12, 2021

This PR adds a command to delete ACLs which match criteria passed via filters.

Example interaction:

$ ./rhoas kafka acl delete --permission allow --operation all --topic rhoas --prefix --service-account srvc-acct-11924479-43fe-42b4-9676-cf0c9aca8136
⚠ The following ACLs will be deleted from Kafka instance "enda":

  PRINCIPAL                                        PERMISSION   OPERATION   DESCRIPTION                
 ------------------------------------------------ ------------ ----------- --------------------------- 
  srvc-acct-11924479-43fe-42b4-9676-cf0c9aca8136   ALLOW        ALL         TOPIC starts with "rhoas"  

? Are you sure you want to delete these ACLs? Yes

⣽ Deleting ACLs from Kafka instance "enda" 
✔️ Deleted 1 ACL from Kafka instance "enda"

I also refactored some of the ACL util files into one package.

Verification

This allows numerous operation combinations so will require extensive verification and testing. Run rhoas kafka acl delete and try a number of combinations.

@craicoverflow craicoverflow force-pushed the delete-acls branch 2 times, most recently from 8840632 to ef9274d Compare October 12, 2021 16:49
@wtrocki wtrocki changed the title feat(kafka acl): add base and list command (#1173) feat(kafka acl): delete acl (#1173) Oct 12, 2021
@craicoverflow craicoverflow force-pushed the delete-acls branch 4 times, most recently from 82343aa to e5ec951 Compare October 13, 2021 15:52
@craicoverflow craicoverflow changed the title feat(kafka acl): delete acl (#1173) feat(kafka acl): add acl delete command Oct 13, 2021
@craicoverflow craicoverflow marked this pull request as ready for review October 13, 2021 15:56
@craicoverflow
Copy link
Contributor Author

This ended up quite large, lots of boilerplate involved.

I will continue testing it tomorrow. But ready for review.

@rkpattnaik780
Copy link
Contributor

./rhoas kafka acl delete --transactional-id "*" --operation all --permission allow --service-account srvc-acct-8c95ca5e1225-94a-41f1-ab97-aacf3df1

preview: All accounts ALLOW ALL TOPIC is "*"

@craicoverflow craicoverflow force-pushed the delete-acls branch 6 times, most recently from 2a8df77 to ab05437 Compare October 14, 2021 11:10
@craicoverflow
Copy link
Contributor Author

./rhoas kafka acl delete --transactional-id "*" --operation all --permission allow --service-account srvc-acct-8c95ca5e1225-94a-41f1-ab97-aacf3df1

preview: All accounts ALLOW ALL TOPIC is "*"

Is this a question?

@rkpattnaik780
Copy link
Contributor

./rhoas kafka acl delete --transactional-id "*" --operation all --permission allow --service-account srvc-acct-8c95ca5e1225-94a-41f1-ab97-aacf3df1

preview: All accounts ALLOW ALL TOPIC is "*"

Is this a question?

Message got lost. On trying to delete a ACL for transactional id, topic is being accessed.

Command: ./rhoas kafka acl delete --user rkpattnaik780 --permission allow --operation describe --transactional-id all
Request URL: /rest/acls?operation=DESCRIBE&patternType=LITERAL&permission=ALLOW&principal=User%3Arkpattnaik780&resourceName=%2A&resourceType=TOPIC

@craicoverflow
Copy link
Contributor Author

Thanks for catching that, it is fixed now.

@rkpattnaik780
Copy link
Contributor

Command: ./rhoas kafka acl delete --user rkpattnaik780 --topic all --prefix --permission allow --operation create
Request URL: /rest/acls?operation=DELETE&patternType=PREFIXED&permission=ALLOW&principal=User%3Arkpattnaik780&resourceName=%2A&resourceType=TOPIC

@rkpattnaik780
Copy link
Contributor

rkpattnaik780 commented Oct 18, 2021

Command: ./rhoas kafka acl delete --user ephelan_kafka_devexp --permission allow --operation describe-config --topic dddddd --prefix
Error: Invalid operation "describe-config" for resource type "topic".
Describe config and alter configs should be a valid operation type for topic. Able to create using UI.

@rkpattnaik780
Copy link
Contributor

Command: ./rhoas kafka acl delete --user ephelan_kafka_devexp --permission allow --operation delete --group my-group.
Request URL: /rest/acls?operation=&patternType=LITERAL&permission=ALLOW&principal=User%3Aephelan_kafka_devexp&resourceName=my-group&resourceType=GROUP
Error message: [Bad Request] Validation error for parameter operation in location QUERY: Input doesn't match one of allowed values of enum: [READ, ALL, ALTER, DELETE, CREATE, ALTER_CONFIGS, ANY, DESCRIBE, DESCRIBE_CONFIGS, WRITE]

OperationFilterDELETE = "delete"
OperationFilterALTER = "alter"
OperationFilterDESCRIBE = "describe"
OperationFilterDESCRIBE_CONFIGS = "describe-config"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use plural like alter-configs?

Suggested change
OperationFilterDESCRIBE_CONFIGS = "describe-config"
OperationFilterDESCRIBE_CONFIGS = "describe-configs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

return withMarkRequiredFunc(fs.cmd, flagName)
}

// AddUser adds a flag to pass a user ID principal
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// AddUser adds a flag to pass a user ID principal
// AddServiceAccount adds a flag to pass a service account ID principal

@rkpattnaik780
Copy link
Contributor

A rare scenario: If user creates a ACL rule using CLI with pricipal="all", <Allow | Read> and topic starts with "all". It would be impossible to delete using CLI, is that an edge case we should handle?

@craicoverflow
Copy link
Contributor Author

A rare scenario: If user creates a ACL rule using CLI with pricipal="all", <Allow | Read> and topic starts with "all". It would be impossible to delete using CLI, is that an edge case we should handle?

As agreed, I have now removed wildcard aliases. Could you review again?

@rkpattnaik780
Copy link
Contributor

Seems like an issue with api maybe
cpmmand: ./rhoas kafka acl delete --user kakashi --permission any --cluster --operation alter -v
preview:

PRINCIPAL      PERMISSION   OPERATION   DESCRIPTION                 
 -------------- ------------ ----------- ---------------------------- 
  kakashi        ALLOW        ALTER       CLUSTER is "kafka-cluster"  
  All accounts   ALLOW        ALTER       CLUSTER is "kafka-cluster"

Copy link
Contributor

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

LGTM!

@craicoverflow craicoverflow merged commit 8edfc44 into redhat-developer:main Oct 19, 2021
@craicoverflow craicoverflow deleted the delete-acls branch October 19, 2021 12:46
@craicoverflow craicoverflow linked an issue Oct 20, 2021 that may be closed by this pull request
@rkpattnaik780
Copy link
Contributor

./rhoas kafka acl delete --user kakashi --permission any --cluster --operation alter -v

Seems like an issue with api maybe cpmmand: ./rhoas kafka acl delete --user kakashi --permission any --cluster --operation alter -v preview:

PRINCIPAL      PERMISSION   OPERATION   DESCRIPTION                 
 -------------- ------------ ----------- ---------------------------- 
  kakashi        ALLOW        ALTER       CLUSTER is "kafka-cluster"  
  All accounts   ALLOW        ALTER       CLUSTER is "kafka-cluster"

Playing around the api a little, I found out that list operation lists out all the permissions with those specifics for the given user, including the ones it gets from All accounts. The above command makes a get request with cluster=kafka-cluster, allow:alter for user kakashi, which also checks if the user gets the access through rules defined for "All accounts".

Similarly, searching for a rule that isn't defined for the user but defined for "All accounts" is returned.

./rhoas kafka acl delete --user wtrocki --permission allow --operation read --group sasuke --prefix
⚠ The following ACLs will be deleted from Kafka instance "rama-test-2":

  PRINCIPAL      PERMISSION   OPERATION   DESCRIPTION                 
 -------------- ------------ ----------- ---------------------------- 
  All accounts   ALLOW        READ        GROUP starts with "sasuke"

I think list operation is working as it should be, some changes should be made to delete to prevent deleting the all account rules. Wdyt?

@craicoverflow
Copy link
Contributor Author

Can you move this to a new issue where can we can discuss this? It will be lost/hidden here.

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.

Implement rhoas kafka acl delete command

2 participants