Skip to content

feat(kafka acl): add instance-id flag and use all alias #1217

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.

Sign up for GitHub

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

Merged
rkpattnaik780 merged 2 commits into from
Oct 13, 2021
Merged

feat(kafka acl): add instance-id flag and use all alias #1217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4aece75
feat(kafka acl): add instance-id flag and use all alias
rkpattnaik780 Oct 12, 2021
f157644
docs: add examples
rkpattnaik780 Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/commands/rhoas_kafka_acl.adoc
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 9 additions & 5 deletions docs/commands/rhoas_kafka_acl_grant-permissions.adoc
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions docs/commands/rhoas_kafka_acl_list.adoc
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 27 additions & 4 deletions pkg/cmd/kafka/acl/grant/grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ import (
kafkainstanceclient "github.com/redhat-developer/app-services-sdk-go/kafkainstance/apiv1internal/client"
)

// When the value of the `--topic`, `--group`, `user` or `service-account` option is one of
// the keys of this map, it will be replaced by the corresponding value.
var commonArgAliases = map[string]string{
"all": acl.Wildcard,
}

type options struct {
Config config.IConfig
Connection factory.ConnectionFunc
Expand Down Expand Up @@ -56,6 +62,10 @@ func NewGrantPermissionsACLCommand(f *factory.Factory) *cobra.Command {
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {

if opts.kafkaID != "" {
return runGrantPermissions(opts)
}

cfg, err := opts.Config.Load()
if err != nil {
return err
Expand Down Expand Up @@ -83,6 +93,7 @@ func NewGrantPermissionsACLCommand(f *factory.Factory) *cobra.Command {
cmd.Flags().BoolVar(&opts.producer, "producer", false, opts.localizer.MustLocalize("kafka.acl.grantPermissions.flag.producer.description"))
cmd.Flags().StringVar(&opts.topicPrefix, "topic-prefix", "", opts.localizer.MustLocalize("kafka.acl.common.flag.topicPrefix.description"))
cmd.Flags().StringVar(&opts.groupPrefix, "group-prefix", "", opts.localizer.MustLocalize("kafka.acl.common.flag.groupPrefix.description"))
cmd.Flags().StringVar(&opts.kafkaID, "instance-id", "", opts.localizer.MustLocalize("kafka.acl.common.flag.instance.id"))
Copy link
Contributor

@craicoverflow craicoverflow Oct 13, 2021

Choose a reason for hiding this comment

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

What would your thoughts be if we make this a persistent flag at the acl command level? This would save having to add it to every acl subcommand.

Copy link
Collaborator

@wtrocki wtrocki Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can we create persistent flag without redefiniing it to set opts.kafkaID in the cmd?
That will probably require some packaged scoped variable that all commands will read?

Not sure if there is clever way that is as clean as defining that flag in every command

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Oct 13, 2021

Choose a reason for hiding this comment

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

I agree, we could have something similar for registry commands as well?

Copy link
Contributor

@craicoverflow craicoverflow Oct 13, 2021

Choose a reason for hiding this comment

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

Actually @wtrocki is probably correct, we would need to have the value set in memory via a pointer like the debug flag for this work.

Feel free to explore the idea, but it may not be worth it.

Copy link
Collaborator

@wtrocki wtrocki Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally we can apply instance-id to numerous commands as in #685 so we can think about this clever way and then change it across CLI. WDYT?

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Oct 13, 2021

Choose a reason for hiding this comment

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

skipping, to be addressed along with 685

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Oct 13, 2021

Choose a reason for hiding this comment

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

@craicoverflow , filtering by id column seems to be unsupported in Admin API. Should we raise it to admin api team, this would enable completions at multiple places.

Copy link
Contributor

@craicoverflow craicoverflow Oct 13, 2021

Choose a reason for hiding this comment

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

I'm not sure what you mean, why does the admin API require ID filtering?

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Oct 13, 2021

Choose a reason for hiding this comment

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

I meant for auto completions of --instance-id

Copy link
Contributor

@craicoverflow craicoverflow Oct 13, 2021

Choose a reason for hiding this comment

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

You mean the control plane API?

You could fetch all IDs and cache them. I don't think we need to have filtering in the API for it for this one use case.


return cmd
}
Expand Down Expand Up @@ -110,7 +121,7 @@ func runGrantPermissions(opts *options) (err error) {
var userArg string

if opts.topic != "" {
topicNameArg = opts.topic
topicNameArg = getArgumentFromAlias(opts.topic)
}

if opts.topicPrefix != "" {
Expand All @@ -119,7 +130,7 @@ func runGrantPermissions(opts *options) (err error) {
}

if opts.group != "" {
groupIdArg = opts.group
groupIdArg = getArgumentFromAlias(opts.group)
}

if opts.groupPrefix != "" {
Expand All @@ -128,11 +139,13 @@ func runGrantPermissions(opts *options) (err error) {
}

if opts.user != "" {
userArg = buildPrincipal(opts.user)
user := getArgumentFromAlias(opts.user)
userArg = buildPrincipal(user)
}

if opts.svcAccount != "" {
userArg = buildPrincipal(opts.svcAccount)
serviceAccount := getArgumentFromAlias(opts.svcAccount)
userArg = buildPrincipal(serviceAccount)
}

req := api.AclsApi.CreateAcl(opts.Context)
Expand Down Expand Up @@ -309,3 +322,13 @@ func validateFlagInputCombination(opts *options) error {

return nil
}

func getArgumentFromAlias(argOrAlias string) string {

argument, ok := commonArgAliases[argOrAlias]
if !ok {
return argOrAlias
}

return argument
}
8 changes: 8 additions & 0 deletions pkg/cmd/kafka/acl/list/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/redhat-developer/app-services-cli/internal/config"
"github.com/redhat-developer/app-services-cli/pkg/cmd/factory"
"github.com/redhat-developer/app-services-cli/pkg/cmdutil"
flagutil "github.com/redhat-developer/app-services-cli/pkg/cmdutil/flags"
"github.com/redhat-developer/app-services-cli/pkg/connection"
"github.com/redhat-developer/app-services-cli/pkg/dump"
"github.com/redhat-developer/app-services-cli/pkg/iostreams"
Expand Down Expand Up @@ -61,6 +62,10 @@ func NewListACLCommand(f *factory.Factory) *cobra.Command {
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {

if opts.kafkaID != "" {
return runList(opts)
}

cfg, err := opts.Config.Load()
if err != nil {
return err
Expand All @@ -79,6 +84,9 @@ func NewListACLCommand(f *factory.Factory) *cobra.Command {
cmd.Flags().Int32Var(&opts.page, "page", cmdutil.ConvertPageValueToInt32(build.DefaultPageNumber), opts.localizer.MustLocalize("kafka.acl.list.flag.page.description"))
cmd.Flags().Int32Var(&opts.size, "size", cmdutil.ConvertSizeValueToInt32(build.DefaultPageSize), opts.localizer.MustLocalize("kafka.acl.list.flag.size.description"))
cmd.Flags().StringVarP(&opts.output, "output", "o", dump.EmptyFormat, opts.localizer.MustLocalize("kafka.acl.list.flag.output.description"))
cmd.Flags().StringVar(&opts.kafkaID, "instance-id", "", opts.localizer.MustLocalize("kafka.acl.common.flag.instance.id"))

flagutil.EnableOutputFlagCompletion(cmd)

return cmd
}
Expand Down
25 changes: 17 additions & 8 deletions pkg/localize/locales/en/cmd/acl.en.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ By default, every users and service account have limited access to their Kafka i
[kafka.acl.cmd.example]
one = '''
# Grant access to principal for consuming messages from all topics
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic "*" --group "*"
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic all --group all

# Grant access to principal for producing messages to all topics
$ rhoas kafka acl grant-permissions --producer --user user_name --topic "*"
$ rhoas kafka acl grant-permissions --producer --user user_name --topic all
Copy link
Contributor

@craicoverflow craicoverflow Oct 13, 2021

Choose a reason for hiding this comment

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

Should we have an example for both?

Copy link
Collaborator

@wtrocki wtrocki Oct 13, 2021

Choose a reason for hiding this comment

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

Not sure myself. @rkpattnaik780 feel free to make call

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Oct 13, 2021

Choose a reason for hiding this comment

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

Won't prefer having asterisk as an example.


# List ACL rules for a Kafka instance
$ rhoas kafka acl list
Expand Down Expand Up @@ -44,10 +44,13 @@ one = 'This command creates ACL rules that grant the specified user access to pr
[kafka.acl.grantPermissions.cmd.example]
one = '''
# Grant access to principal for consuming messages from all topics
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic "*" --group "*"
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic all --group all

# Grant access to principal for consuming messages from all topics in a specified instance
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic all --group all --instance-id c5hv7iru4an1g84pogp0

# Grant access to principal for producing messages to all topics
$ rhoas kafka acl grant-permissions --producer --user user_name --topic "*"
$ rhoas kafka acl grant-permissions --producer --user user_name --topic all

# Grant access to principal for consuming messages from topics starting with "abc"
$ rhoas kafka acl grant-permissions --consumer --user user_name --topic-prefix "abc" --group my-group
Expand All @@ -56,13 +59,13 @@ $ rhoas kafka acl grant-permissions --consumer --user user_name --topic-prefix "
$ rhoas kafka acl grant-permissions --producer --user user_name --topic-prefix "abc"

# Grant access to all users for consuming messages from topic "my-topic"
$ rhoas kafka acl grant-permissions --consumer --user "*" --topic my-topic --group my-group
$ rhoas kafka acl grant-permissions --consumer --user all --topic my-topic --group my-group

# Grant access to all users for producing messages to topic "my-topic"
$ rhoas kafka acl grant-permissions --producer --user "*" --topic my-topic
$ rhoas kafka acl grant-permissions --producer --user all --topic my-topic

# Grant access to principal for produce and consume messages from all topics
$ rhoas kafka acl grant-permissions --producer --consumer --user user_name --topic "*" --group "*"
$ rhoas kafka acl grant-permissions --producer --consumer --user user_name --topic all --group all
'''

[kafka.acl.grantPermissions.error.bothPrincipalsSelected]
Expand Down Expand Up @@ -93,6 +96,9 @@ one = 'Prefix name for topics to be selected'
[kafka.acl.common.flag.groupPrefix.description]
one = 'Prefix name for groups to be selected'

[kafka.acl.common.flag.instance.id]
one = 'ID of the Kafka instance to be used. By default, uses the currently selected instance'

[kafka.acl.grantPermissions.flag.producer.description]
one = 'Add ACL rules that grant the specified principal access to produce messages to topics'

Expand All @@ -112,8 +118,11 @@ The instances are displayed by default in a table, but can also be displayed as

[kafka.acl.list.cmd.example]
one = '''
# Display Kafka ACL rules for the instance
# Display Kafka ACL rules for the Kafka instance
$ rhoas kafka acl list

# Display Kafka ACL rules for a specific Kafka instance
$ rhoas kafka acl list --instance-id c5hv7iru4an1g84pogp0
'''


Expand Down