fix: allow to delete all ACLs for principal by wtrocki · Pull Request #1441 · redhat-developer/app-services-cli

wtrocki · 2022-02-15T15:32:44Z

Creating for early feedback for #1430

Validation for delete command were redundant preventing users from deleting multiple acls.
For example it would not be possible to remove all ACLs for Service account.

After this change it is possible:

✔️  Deleted 5 ACLs from Kafka instance "wtrocki"
The following ACL rules were deleted:

  PRINCIPAL (5)    PERMISSION   OPERATION   DESCRIPTION               
 ---------------- ------------ ----------- -------------------------- 
  test             allow        write       transactional-id is "*"   
  test             allow        describe    topic starts with "test"  
  test             allow        write       topic starts with "test"  
  test             allow        create      topic starts with "test"  
  test             allow        describe    transactional-id is "*"

However we need to still spend some time cleaning up messages/validation and arguments to API.
Posted for early feedback

Verification

[wtrocki@graphapi app-services-cli (delete-acl)]$ rhoas kafka acl delete --pattern-type=any --service-account=test
? All ACLs matching the criteria provided will be deleted from the Kafka instance "wtrocki". Are you sure you want to proceed? Yes


✔️  Deleted 5 ACLs from Kafka instance "wtrocki"
The following ACL rules were deleted:

  PRINCIPAL (5)    PERMISSION   OPERATION   DESCRIPTION               
 ---------------- ------------ ----------- -------------------------- 
  test             allow        describe    topic starts with "test"  
  test             allow        write       transactional-id is "*"   
  test             allow        describe    transactional-id is "*"   
  test             allow        write       topic starts with "test"  
  test             allow        create      topic starts with "test"

Delete all write operations for sa

rhoas kafka acl delete --service-account=test1 --operation=write -v --pattern-type=any

other variations

rkpattnaik780 · 2022-02-16T07:01:23Z

Deleting all acls for a principal.
Passing other arguments needs to be handled for deleting specific ACL. Ignore if it is on todo.

wtrocki · 2022-02-16T12:39:57Z

Not sure if we should allow all accounts as well?


[wtrocki@graphapi app-services-cli (delete-acl ✗)]$ rhoas kafka acl delete --all-accounts         
? All ACLs matching the criteria provided will be deleted from the Kafka instance "wtrocki". Are you sure you want to proceed? Yes


✔️  Deleted 4 ACLs from Kafka instance "wtrocki"
The following ACL rules were deleted:

  PRINCIPAL (4)    PERMISSION   OPERATION          DESCRIPTION     
 ---------------- ------------ ------------------ ---------------- 
  All Accounts     allow        describe-configs   topic is "*"    
  All Accounts     allow        describe           group is "*"    
  All Accounts     allow        describe           cluster is "*"  
  All Accounts     allow        describe           topic is "*"

wtrocki · 2022-02-16T12:48:38Z

Seeing some strange issue/inconsitencies:

[wtrocki@graphapi app-services-cli (delete-acl)]$ rhoas kafka acl list                    

  PRINCIPAL (5)    PERMISSION   OPERATION   DESCRIPTION               
 ---------------- ------------ ----------- -------------------------- 
  wtrocki          allow        create      topic starts with "test"  
  wtrocki          allow        describe    topic starts with "test"  
  wtrocki          allow        write       topic starts with "test"  
  wtrocki          allow        describe    transactional-id is "*"   
  wtrocki          allow        write       transactional-id is "*"   
[wtrocki@graphapi app-services-cli (delete-acl)]$ rhoas kafka acl delete --service-account=wtrocki
? All ACLs matching the criteria provided will be deleted from the Kafka instance "wtrocki". Are you sure you want to proceed? Yes


✔️  Deleted 2 ACLs from Kafka instance "wtrocki"
The following ACL rules were deleted:

  PRINCIPAL   PERMISSION   OPERATION   DESCRIPTION              
 ----------- ------------ ----------- ------------------------- 
  wtrocki     allow        describe    transactional-id is "*"  
  wtrocki     allow        write       transactional-id is "*"

wtrocki · 2022-02-16T12:59:12Z

Ok. So problem is with Pattern Type. because we assume Literal pattern type is provided by default there is no easy way for us to fix that without breaking CLI. I will need to add explicit pattern-type=none and deprecated --prefix to get around this

wtrocki · 2022-02-16T16:12:36Z

@craicoverflow @rkpattnaik780 I have managed to workaround problem by introducing new flag called pattern type and deprecated prefix.

Not sure if we should be deprecating prefix or still keep it?
I'm testing all gazillion test use cases that this change introduced

craicoverflow · 2022-02-16T16:15:00Z

Pattern type is fine to use. I used "prefix" because this is what confluent CLI does.

wtrocki · 2022-02-16T16:17:17Z

I still removed depreciation from prefix as it is much simpler/neater flag.

We have pattern-type=all only because there is no other way to reflect it now. When none is specified it should be literal and prefix stays as it is. Obviously pattern type supports all other flags but that is too verbose IMHO for regular use

rkpattnaik780 · 2022-02-17T07:08:28Z

Not sure if we should allow all accounts as well?

UI allows deleting permissions for all accounts. We should keep it too.

rkpattnaik780 · 2022-02-17T07:16:01Z

pkg/cmd/kafka/acl/delete/delete.go

 	}

-	opts.PatternType = aclcmdutil.PatternTypeLITERAL
+	// Backwards compatibility:


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

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

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.

rkpattnaik780 · 2022-02-17T07:21:21Z

pkg/core/localize/locales/en/cmd/acl.en.toml

 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}}'


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?

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

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.

craicoverflow · 2022-02-17T09:13:47Z

What is this submodule for?

wtrocki · 2022-02-17T12:05:45Z

What is this submodule for?

That was different POC. Removing it

craicoverflow

Verified. I left one suggestion regarding printing deprecation message from within the command output aswell as in the description.

craicoverflow · 2022-02-17T09:19:58Z

pkg/cmd/kafka/acl/delete/delete.go

+	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
+	})


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.

I think flag package is overkill

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

craicoverflow · 2022-02-17T09:23:04Z

pkg/cmd/kafka/acl/delete/delete.go

 	}

-	opts.PatternType = aclcmdutil.PatternTypeLITERAL
+	// Backwards compatibility:


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.

craicoverflow · 2022-02-17T09:26:58Z

pkg/core/cmdutil/flagutil/deprecation.go

+// DeprecateFlag provides a way to deprecate a flag by appending standard prefixes to the flag description.
+func DeprecateFlag(flagDescription string) string {
+	return "DEPRECATED: " + flagDescription
+}


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.

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

craicoverflow · 2022-02-17T09:29:05Z

pkg/core/localize/locales/en/cmd/acl.en.toml

 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}}'


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.

craicoverflow · 2022-02-17T09:34:45Z

pkg/core/localize/locales/en/cmd/acl.en.toml

 $ 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"


Is pattern type needed in order to do this?

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

So:

nothing provided - exact match

all - take all

prefix (use prefix)

etc.

wtrocki · 2022-02-17T12:36:17Z

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.

Rule itself is good, but it does have lots of false positives - where doing switch is not possible.
I will need to start disabling linting for this instead.

fix: allow to delete all ACLs for principal

72f616f

wtrocki requested review from craicoverflow and rkpattnaik780 February 15, 2022 15:32

openshift-ci bot added the do-not-merge/work-in-progress label Feb 15, 2022

feat: allow to delete more than one ACL at the time

8de1f79

wtrocki added 2 commits February 16, 2022 15:47

fix: add new flags to support new flow for delete command

0f9aceb

fix: update documentation

d2baa6e

wtrocki force-pushed the delete-acl branch from 94d311c to d2baa6e Compare February 16, 2022 16:08

fix: documentation for pattern type

741bb5b

fix: remove depreciation from prefix

9fa50ff

wtrocki marked this pull request as ready for review February 16, 2022 16:17

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 16, 2022

rkpattnaik780 requested changes Feb 17, 2022

View reviewed changes

fix: move prefix flag to be more important

cddb206

wtrocki added 2 commits February 17, 2022 12:14

fix: remove app services tools

9ced870

fix: clarify message for pattern-type

8295746

craicoverflow approved these changes Feb 17, 2022

View reviewed changes

fix: documentation

d44f176

rkpattnaik780 approved these changes Feb 17, 2022

View reviewed changes

wtrocki merged commit 424d92a into main Feb 17, 2022

wtrocki deleted the delete-acl branch February 17, 2022 12:57

Conversation

wtrocki commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification

Uh oh!

rkpattnaik780 commented Feb 16, 2022

Uh oh!

wtrocki commented Feb 16, 2022

Uh oh!

wtrocki commented Feb 16, 2022

Uh oh!

wtrocki commented Feb 16, 2022

Uh oh!

wtrocki commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craicoverflow commented Feb 16, 2022

Uh oh!

wtrocki commented Feb 16, 2022

Uh oh!

rkpattnaik780 commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow commented Feb 17, 2022

Uh oh!

wtrocki commented Feb 17, 2022

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wtrocki commented Feb 15, 2022 •

edited

Loading

wtrocki commented Feb 16, 2022 •

edited

Loading