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 1 commit
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
1 change: 1 addition & 0 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.

7 changes: 4 additions & 3 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
3 changes: 3 additions & 0 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 @@ -93,6 +93,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 Down