Skip to content

fix: allow to delete all ACLs for principal #1441

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
wtrocki merged 10 commits into from
Feb 17, 2022
Merged

fix: allow to delete all ACLs for principal #1441

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
72f616f
fix: allow to delete all ACLs for principal
wtrocki Feb 15, 2022
8de1f79
feat: allow to delete more than one ACL at the time
wtrocki Feb 16, 2022
0f9aceb
fix: add new flags to support new flow for delete command
wtrocki Feb 16, 2022
d2baa6e
fix: update documentation
wtrocki Feb 16, 2022
741bb5b
fix: documentation for pattern type
wtrocki Feb 16, 2022
9fa50ff
fix: remove depreciation from prefix
wtrocki Feb 16, 2022
cddb206
fix: move prefix flag to be more important
wtrocki Feb 17, 2022
9ced870
fix: remove app services tools
wtrocki Feb 17, 2022
8295746
fix: clarify message for pattern-type
wtrocki Feb 17, 2022
d44f176
fix: documentation
wtrocki Feb 17, 2022
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 app-services-tools
Open in desktop
Submodule app-services-tools added at 3f5c1a
4 changes: 4 additions & 0 deletions docs/commands/rhoas_kafka_acl_delete.md
View file
Open in desktop

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

2 changes: 2 additions & 0 deletions pkg/cmd/kafka/acl/aclcmdutil/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,5 @@ const (
PatternTypePREFIX = "prefix"
PatternTypeANY = "any"
)

var PatternTypes = []string{PatternTypeANY, PatternTypeLITERAL, PatternTypePREFIX}
10 changes: 3 additions & 7 deletions pkg/cmd/kafka/acl/aclcmdutil/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ func IsValidResourceOperation(resourceType string, operation string, resourceOpe
return false, resourceOperations
}

// ValidateAndSetResources validates and sets resources options
func ValidateAndSetResources(opts *CrudOptions, resourceTypeFlagEntries []*localize.TemplateEntry) error {
// SetACLResources sets resources options and returns number of changed resources
func SetACLResources(opts *CrudOptions) int {
var selectedResourceTypeCount int

if opts.Topic != "" {
Expand All @@ -118,11 +118,7 @@ func ValidateAndSetResources(opts *CrudOptions, resourceTypeFlagEntries []*local
opts.ResourceName = KafkaCluster
}

if selectedResourceTypeCount != 1 {
return opts.Localizer.MustLocalizeError("kafka.acl.common.error.oneResourceTypeAllowed", resourceTypeFlagEntries...)
}

return nil
return selectedResourceTypeCount
}

// ValidateAPIError checks for a HTTP error and maps it to a user friendly error
Expand Down
10 changes: 6 additions & 4 deletions pkg/cmd/kafka/acl/create/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package create
import (
"github.com/AlecAivazis/survey/v2"
"github.com/redhat-developer/app-services-cli/pkg/cmd/kafka/acl/aclcmdutil"
aclFlagutil "github.com/redhat-developer/app-services-cli/pkg/cmd/kafka/acl/flagutil"
aclFlagUtil "github.com/redhat-developer/app-services-cli/pkg/cmd/kafka/acl/flagutil"
"github.com/redhat-developer/app-services-cli/pkg/core/cmdutil"
"github.com/redhat-developer/app-services-cli/pkg/core/cmdutil/flagutil"
"github.com/redhat-developer/app-services-cli/pkg/core/ioutil/dump"
Expand Down Expand Up @@ -63,8 +63,10 @@ func NewCreateCommand(f *factory.Factory) *cobra.Command {
errorCollection = append(errorCollection, opts.Localizer.MustLocalizeError("kafka.acl.common.flag.operation.required"))
}

if resourceErrors := aclcmdutil.ValidateAndSetResources(opts, aclFlagutil.ResourceTypeFlagEntries); resourceErrors != nil {
errorCollection = append(errorCollection, resourceErrors)
selectedResourceTypeCount := aclcmdutil.SetACLResources(opts)

if selectedResourceTypeCount != 1 {
errorCollection = append(errorCollection, opts.Localizer.MustLocalizeError("kafka.acl.common.error.oneResourceTypeAllowed", aclFlagUtil.ResourceTypeFlagEntries...))
}

if principalErrors := validateAndSetOpts(opts); principalErrors != nil {
Expand All @@ -79,7 +81,7 @@ func NewCreateCommand(f *factory.Factory) *cobra.Command {
},
}

flags := aclFlagutil.NewFlagSet(cmd, f)
flags := aclFlagUtil.NewFlagSet(cmd, f)

flags.AddPermissionCreate(&opts.Permission)
flags.AddOperationCreate(&opts.Operation)
Expand Down
91 changes: 65 additions & 26 deletions pkg/cmd/kafka/acl/delete/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ import (
)

var (
serviceAccount string
userID string
allAccounts bool
prefix bool
serviceAccount string
userID string
allAccounts bool
prefix bool
patternTypeFlag string
)

type requestParams struct {
Expand Down Expand Up @@ -56,12 +57,10 @@ func NewDeleteCommand(f *factory.Factory) *cobra.Command {

var errorCollection []error

if opts.Operation == "" {
errorCollection = append(errorCollection, opts.Localizer.MustLocalizeError("kafka.acl.common.flag.operation.required"))
}

if resourceErrors := aclcmdutil.ValidateAndSetResources(opts, aclFlagUtil.ResourceTypeFlagEntries); resourceErrors != nil {
errorCollection = append(errorCollection, resourceErrors)
selectedResourceTypeCount := aclcmdutil.SetACLResources(opts)
if selectedResourceTypeCount > 1 {
errorCollection = append(errorCollection,
opts.Localizer.MustLocalizeError("kafka.acl.common.error.oneResourceTypeAllowed", aclFlagUtil.ResourceTypeFlagEntries...))
}

if principalErrors := validateAndSetOpts(opts); principalErrors != nil {
Expand All @@ -82,7 +81,7 @@ func NewDeleteCommand(f *factory.Factory) *cobra.Command {
flags.AddOperationFilter(&opts.Operation)

flags.AddCluster(&opts.Cluster)
flags.AddPrefix(&prefix)

flags.AddTopic(&opts.Topic)
flags.AddConsumerGroup(&opts.Group)
flags.AddTransactionalID(&opts.TransactionalID)
Expand All @@ -92,6 +91,19 @@ func NewDeleteCommand(f *factory.Factory) *cobra.Command {
flags.AddServiceAccount(&serviceAccount)
flags.AddAllAccounts(&allAccounts)
flags.AddYes(&opts.SkipConfirm)
flags.AddPrefix(&prefix)

cmd.Flags().StringVar(
&patternTypeFlag,
"pattern-type",
aclcmdutil.PatternTypeLITERAL,
opts.Localizer.MustLocalize("kafka.acl.common.flag.patterntypes.description",
localize.NewEntry("Types", aclcmdutil.PatternTypes)),
)

_ = cmd.RegisterFlagCompletionFunc("pattern-type", func(cmd *cobra.Command, _ []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return aclcmdutil.PatternTypes, cobra.ShellCompDirectiveNoSpace
})
Comment on lines +96 to +106
Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

It'd be nice to add this into the flag package we have created? I know it is not used in other commands but it does abstract it away and make the command setup much easier to read.

Copy link
Collaborator Author

@wtrocki wtrocki Feb 17, 2022

Choose a reason for hiding this comment

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

I think flag package is overkill

Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

The package already exists though, so don't see how (there is no extra code involved).


return cmd
}
Expand All @@ -118,12 +130,15 @@ func runDelete(instanceID string, opts *aclcmdutil.CrudOptions) error {
return err
}

if isValidOp, validResourceOperations := aclcmdutil.IsValidResourceOperation(opts.ResourceType, opts.Operation, resourceOperations); !isValidOp {
return opts.Localizer.MustLocalizeError("kafka.acl.common.error.invalidResourceOperation",
localize.NewEntry("ResourceType", opts.ResourceType),
localize.NewEntry("Operation", opts.Operation),
localize.NewEntry("ValidOperationList", cmdutil.StringSliceToListStringWithQuotes(validResourceOperations)),
)
// Validate only when both are present
if opts.ResourceType != "" && opts.Operation != "" {
if isValidOp, validResourceOperations := aclcmdutil.IsValidResourceOperation(opts.ResourceType, opts.Operation, resourceOperations); !isValidOp {
return opts.Localizer.MustLocalizeError("kafka.acl.common.error.invalidResourceOperation",
localize.NewEntry("ResourceType", opts.ResourceType),
localize.NewEntry("Operation", opts.Operation),
localize.NewEntry("ValidOperationList", cmdutil.StringSliceToListStringWithQuotes(validResourceOperations)),
)
}
}

kafkaNameTmplEntry := localize.NewEntry("Name", kafkaInstance.GetName())
Expand All @@ -149,14 +164,29 @@ func runDelete(instanceID string, opts *aclcmdutil.CrudOptions) error {

requestParams := getRequestParams(opts)

deletedACLs, httpRes, err := adminAPI.AclsApi.DeleteAcls(ctx).
ResourceType(requestParams.resourceType).
Principal(requestParams.principal).
PatternType(requestParams.patternType).
ResourceName(requestParams.resourceName).
Operation(requestParams.operation).
Permission(requestParams.permission).
Execute()
requestDeleteAcls := adminAPI.AclsApi.DeleteAcls(ctx)
if requestParams.resourceType != "" {
requestDeleteAcls = requestDeleteAcls.ResourceType(requestParams.resourceType)
}

if requestParams.principal != "" {
requestDeleteAcls = requestDeleteAcls.Principal(requestParams.principal)
}

if requestParams.resourceName != "" {
requestDeleteAcls = requestDeleteAcls.ResourceName(requestParams.resourceName)
}
if requestParams.patternType != "" {
requestDeleteAcls = requestDeleteAcls.PatternType(requestParams.patternType)
}
if requestParams.operation != "" {
requestDeleteAcls = requestDeleteAcls.Operation(requestParams.operation)
}
if requestParams.permission != "" {
requestDeleteAcls = requestDeleteAcls.Permission(requestParams.permission)
}

deletedACLs, httpRes, err := requestDeleteAcls.Execute()

if httpRes != nil {
defer httpRes.Body.Close()
Expand Down Expand Up @@ -226,11 +256,20 @@ func validateAndSetOpts(opts *aclcmdutil.CrudOptions) error {
return opts.Localizer.MustLocalizeError("kafka.acl.common.error.noPrincipalsSelected")
}

opts.PatternType = aclcmdutil.PatternTypeLITERAL
// Backwards compatibility:
Copy link
Contributor

@rkpattnaik780 rkpattnaik780 Feb 17, 2022

Choose a reason for hiding this comment

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

This should be moved after the switch in 264.
Current behavior keeps pattern type as literal if we pass --prefix flag.

./rhoas kafka acl delete --user enda --topic "rhoas-" --prefix -v

/rest/acls?patternType=LITERAL&permission=ANY&principal=User%3Aenda&resourceName=rhoas-&resourceType=TOPIC

Copy link
Collaborator Author

@wtrocki wtrocki Feb 17, 2022

Choose a reason for hiding this comment

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

Oh.. I did refactor if into switch and introduced that bug. Really hate that golanglint forces us to use switch statements even when they do not make sense

Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

You could always look to remove that rule from the lint config file? It is not a rule coming from the Go stdlib or anything.

if prefix {
opts.PatternType = aclcmdutil.PatternTypePREFIX
}

switch patternTypeFlag {
case aclcmdutil.PatternTypeANY:
opts.PatternType = aclcmdutil.PatternTypeANY
case aclcmdutil.PatternTypePREFIX:
opts.PatternType = aclcmdutil.PatternTypePREFIX
case aclcmdutil.PatternTypeLITERAL:
opts.PatternType = aclcmdutil.PatternTypeLITERAL
}

if userID != "" {
opts.Principal = userID
} else if serviceAccount != "" {
Expand Down
6 changes: 6 additions & 0 deletions pkg/core/cmdutil/flagutil/deprecation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package flagutil

// DeprecateFlag provides a way to deprecate a flag by appending standard prefixes to the flag description.
func DeprecateFlag(flagDescription string) string {
return "DEPRECATED: " + flagDescription
}
Comment on lines +3 to +6
Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

I also think that when a deprecated flag is used, the command should print a message like:

$ rhoas kafka acl delete --prefix

The "--prefix" flag has been deprecated, in the future please use "--pattern-type"

... delete output

The reason being that existing users of the CLI will not be checking the flag descriptions any more as they already understand the purpose of the prefix flag, so will likely never see that it is deprecated.

Copy link
Collaborator Author

@wtrocki wtrocki Feb 17, 2022

Choose a reason for hiding this comment

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

Fully agree. I will remove that method as it is not used right now and it is not properly implemented.

6 changes: 6 additions & 0 deletions pkg/core/localize/locales/en/cmd/acl.en.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ one = 'Set the resource type to cluster'
[kafka.acl.common.flag.prefix.description]
one = 'Determine if the resource should be exact match or prefix'

[kafka.acl.common.flag.patterntypes.description]
one = 'Determine if the resource should be exact match, prefix or any {{.Types}}'
Copy link
Contributor

@rkpattnaik780 rkpattnaik780 Feb 17, 2022

Choose a reason for hiding this comment

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

The flag description looks odd

Determine if the resource should be exact match, prefix or any [any literal prefix] (default "literal")

Should we keep options in description only?

Copy link
Collaborator Author

@wtrocki wtrocki Feb 17, 2022

Choose a reason for hiding this comment

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

It is good to give list of values in canonical form. Will remove them from description

Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

Yes you are right @wtrocki it is good to, but currently they are not. The output that @rkpattnaik780 shows is the exact output from the command.


[kafka.acl.common.flag.topic.description]
one = 'Set the topic resource. When the --prefix option is also passed, this is used as the topic prefix'

Expand Down Expand Up @@ -291,6 +294,9 @@ $ rhoas kafka acl delete --operation write --permission allow --topic all --user
# Delete an ACL for a service account
$ rhoas kafka acl delete --operation all --permission any --topic "rhoas" --prefix --service-account "srvc-acct-11924479-43fe-42b4-9676-cf0c9aca81"

# Delete all ACLs for a service account
$ rhoas kafka acl delete --service-account "srvc-acct-11924479-43fe-42b4-9676-cf0c9aca81 --pattern-type=all"
Copy link
Contributor

@craicoverflow craicoverflow Feb 17, 2022

Choose a reason for hiding this comment

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

Is pattern type needed in order to do this?

Copy link
Collaborator Author

@wtrocki wtrocki Feb 17, 2022

Choose a reason for hiding this comment

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

Yes. While it is counter intuitive we need backwards compatibility.

So:

  • nothing provided - exact match
  • all - take all
  • prefix (use prefix)

etc.


# Delete an ACL for all users on the consumer group resource
$ rhoas kafka acl delete --operation all --permission any --group "group-1" --all-accounts
'''
Expand Down