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

CLI-259: Add support for centralized ACLs #295

Merged
merged 3 commits into from
Oct 3, 2019

Conversation

arvindth
Copy link
Member

@arvindth arvindth commented Sep 17, 2019

What

Add support for centralized ACLs for the confluent cli tool.

Depends on confluentinc/mds-sdk-go#2

Differences between confluent and ccloud apis:

  • confluent acl commands have been placed under confluent iam acl ... whereas existing ccloud acls commands are under ccloud kafka acl ...
    • ccloud placed this under kafka to underscore the point that only kafka acls could be manipulated, and not say schema registry acls.
    • The intention behind moving this under iam is to eventually specify the resource using CRN notation (not implemented yet except in Audit logs). At that point, both schema registry and kafka resources would be specified the same way, so the cli usage would be generic and would make more sense to consolidate with other security related commands under iam.
  • confluent iam acl delete prints out a list of matching acls that got deleted. ccloud does not.

Other design decisions:

  • Although the backend mds acl delete api allows specifying a filter for matching acls to delete, this is flexible enough to delete all acls by running something like:
    • confluent iam acl delete --kafka-cluster-id
    • To prevent users from accidentally deleting everything without meaning to, the cli deliberately requires users to specify principal/operation/host to narrow to individual acls to delete.
  • Backward compatibility:
    • These acl commands will only work against a 5.4+ mds server. Running against a 5.3 server will throw a 404, which is caught by the cli and changed to a more informative error message about the backend version.
    • Until 5.4 is released, the new acl subcommand is wrapped in a feature flag that needs to be set to enable this feature.

References

Related PRs: Depends on confluentinc/mds-sdk-go#2
Related JIRA: CLI-259

Test & Review

Added unit tests. Build using make deps && make build. Test using make test

Run using the following sample commands:

# export XX_FLAG_CENTRALIZED_ACL_ENABLE=true
# confluent login --url http://localhost:8090
# confluent iam acl create --allow --principal User:1522 --operation READ --consumer-group java_example_group_1 --kafka-cluster-id my-cluster
# confluent iam acl create --allow --principal Group:testgroup --operation READ --kafka-cluster-id my-cluster --transactional-id xyz --prefix
# confluent iam acl list --kafka-cluster-id my-cluster
# confluent iam acl delete --allow --principal User:1522 --operation READ --consumer-group java_example_group_1 --kafka-cluster-id my-cluster --host '*'

Out of scope

  • Backward compatibility: Have the help and completion dynamically hide/show the acl subcommand based on the version of the backend server. Out of scope because it requires:
    • implementation of a current cli context (in progress but not completed).
    • support from the mds server to return the current version for querying.
    • updates to the bash completion to potentially include a custom completion function to handle these cases
    • an upstream merge of zsh: added custom completions spf13/cobra#884 to enable custom zsh completions before they can be used for this task.

go.mod Outdated Show resolved Hide resolved
@arvindth arvindth force-pushed the ath-centralized-acls branch 2 times, most recently from 2c570f3 to 124d692 Compare September 17, 2019 19:14
@arvindth arvindth changed the title CLI-259: Add support for centralized ACLs WIP CLI-259: Add support for centralized ACLs Sep 18, 2019
Copy link
Member

@codyaray codyaray left a comment

Choose a reason for hiding this comment

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

lgtm

Man I can't wait til we have declarative policies for this stuff. These are some long gnarly commands.

internal/cmd/iam/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/iam/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/iam/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/iam/command_acl.go Show resolved Hide resolved
internal/cmd/iam/command_acl_test.go Show resolved Hide resolved
internal/cmd/iam/command_acl_test.go Show resolved Hide resolved
internal/cmd/iam/command_acl_test.go Show resolved Hide resolved
internal/cmd/iam/command_acl_utils.go Show resolved Hide resolved
internal/cmd/iam/command_acl_utils.go Show resolved Hide resolved
internal/cmd/iam/command_acl_test.go Show resolved Hide resolved
Copy link
Member

@codyaray codyaray left a comment

Choose a reason for hiding this comment

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

thanks!

@arvindth
Copy link
Member Author

arvindth commented Oct 3, 2019

retest this please

  - Also, zsh completion doesn't like braces in the description
  - Also wrap centralized acls with temporary feature flag to
    allow merge to master without breaking 5.3 compatibility.
  - Label acls command as 5.4+ only
  - Print out the cluster id on list/delete output
  - Make new acl subcommand backward compatible to 5.3.
    - Replace generic 404 error with specific error message
@arvindth arvindth merged commit 3f8b74a into confluentinc:master Oct 3, 2019
@arvindth arvindth deleted the ath-centralized-acls branch October 4, 2019 16:41
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