feat(kafka acl): add grant-admin command by rkpattnaik780 · Pull Request #1230 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-10-18T16:43:09Z

Add rhoas kafka acl admin command.

Verification Steps

Run admin command with --user flag set to abc

rhoas kafka acl grant-admin --user abc

It should create a permission for Kafka Instance with principal=abc and operationType=ALTER

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

craicoverflow

Command did not work for me:

❯ ./rhoas kafka acl admin
panic: message "kafka.acl.grantPermissions.error.noPrincipalsSelected" not found in language "en"

goroutine 1 [running]:
github.com/nicksnyder/go-i18n/v2/i18n.(*Localizer).MustLocalize(0xc0001af540, 0xc000133c28, 0x0, 0x0)
        /home/ephelan/.gvm/pkgsets/go1.16.4/global/pkg/mod/github.com/nicksnyder/go-i18n/v2@v2.1.2/i18n/localizer.go:211 +0x7c
github.com/redhat-developer/app-services-cli/pkg/localize/goi18n.(*Goi18n).MustLocalize(0xc00070ccd0, 0x1e4d942, 0x35, 0x0, 0x0, 0x0, 0xc0000ea100, 0xc000133cf8)
        /home/ephelan/code/github.com/redhat-developer/app-services-cli/pkg/localize/goi18n/go_i18n.go:85 +0x165
github.com/redhat-developer/app-services-cli/pkg/localize/goi18n.(*Goi18n).MustLocalizeError(0xc00070ccd0, 0x1e4d942, 0x35, 0x0, 0x0, 0x0, 0xc000133ce0, 0x0)
        /home/ephelan/code/github.com/redhat-developer/app-services-cli/pkg/localize/goi18n/go_i18n.go:111 +0x67
github.com/redhat-developer/app-services-cli/pkg/cmd/kafka/acl/admin.NewAdminACLCommand.func1(0xc0001d3680, 0x2db8c80, 0x0, 0x0, 0x0, 0x0)
        /home/ephelan/code/github.com/redhat-developer/app-services-cli/pkg/cmd/kafka/acl/admin/admin.go:71 +0x18a
github.com/spf13/cobra.(*Command).execute(0xc0001d3680, 0x2db8c80, 0x0, 0x0, 0xc0001d3680, 0x2db8c80)
        /home/ephelan/.gvm/pkgsets/go1.16.4/global/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:856 +0x472
github.com/spf13/cobra.(*Command).ExecuteC(0xc00032e780, 0x2057538, 0x3, 0xc00032e780)
        /home/ephelan/.gvm/pkgsets/go1.16.4/global/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:974 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
        /home/ephelan/.gvm/pkgsets/go1.16.4/global/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:902
main.main()
        /home/ephelan/code/github.com/redhat-developer/app-services-cli/cmd/rhoas/main.go:43 +0x29d

pkg/cmd/kafka/acl/admin/admin.go

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

craicoverflow

The ACLs granted in this command do now allow me to create topics:

$ kafka-topics.sh --create --topic rhoas_2hello --bootstrap-server 
Error while executing topic command : Authorization failed.
[2021-10-20 15:14:35,800] ERROR org.apache.kafka.common.errors.TopicAuthorizationException: Authorization failed.
 (kafka.admin.TopicCommand$)

However once I create an ACL with ALLOW | ALL | TOPIC is "*"I could do it:

$ kafka-topics.sh --create --topic rhoas_2hello --bootstrap-server $(rhoas kafka describe --bootstrap-server) --command-config ./kafka-config.prod.properties
Created topic rhoas_2hello.

I recommend investagint the Kafka bin scripts, to see exactly what they would apply, we may need additonal ACLs other than CLUSTER ALTER

rkpattnaik780 · 2021-10-21T08:29:54Z

The ACLs granted in this command do now allow me to create topics:
$ kafka-topics.sh --create --topic rhoas_2hello --bootstrap-server 
Error while executing topic command : Authorization failed.
[2021-10-20 15:14:35,800] ERROR org.apache.kafka.common.errors.TopicAuthorizationException: Authorization failed.
 (kafka.admin.TopicCommand$)
However once I create an ACL with ALLOW | ALL | TOPIC is "*"I could do it:
$ kafka-topics.sh --create --topic rhoas_2hello --bootstrap-server $(rhoas kafka describe --bootstrap-server) --command-config ./kafka-config.prod.properties
Created topic rhoas_2hello.
I recommend investagint the Kafka bin scripts, to see exactly what they would apply, we may need additonal ACLs other than CLUSTER ALTER

bin scripts don't seem to have an explicit option to grant admin access, it is a common use case. The design documents mention an extra ACL rule for cluster describe, the requirements point that admin should grant only ability to add/delete ACLs, user must create an acl before doing operations.

craicoverflow · 2021-10-21T09:04:02Z

Great work so far!

I think we should also be displaying to the user the exact ACL(s) like we do in the grant-access commands, as we are hiding the actual ACL that is being created. This makes it difficult for the user to:

a) get a clear understanding of what is being applied
b) reverse the decision by deleting it in case of a mistake

$ ./rhoas kafka acl grant-access --service-account hello --producer --topic rhoas
The following ACL rules are to be created:

  PRINCIPAL (5)    PERMISSION   OPERATION   DESCRIPTION              
 ---------------- ------------ ----------- ------------------------- 
  hello            ALLOW        DESCRIBE    TOPIC is "rhoas"         
  hello            ALLOW        WRITE       TOPIC is "rhoas"         
  hello            ALLOW        CREATE      TOPIC is "rhoas"         
  hello            ALLOW        WRITE       TRANSACTIONAL_ID is "*"  
  hello            ALLOW        DESCRIBE    TRANSACTIONAL_ID is "*"  

? Are you sure you want to create the listed ACL rules (y/N)

craicoverflow · 2021-10-26T15:41:08Z

pkg/cmd/kafka/acl/admin/admin.go

+			// check if priincipal is provided
+			if userID == "" && serviceAccount == "" && !allAccounts {
+				return opts.localizer.MustLocalizeError("kafka.acl.common.error.noPrincipalsSelected")
+			}
+
+			// user and service account can't be along with "--all-accounts" flag
+			if allAccounts && (serviceAccount != "" || userID != "") {
+				return opts.localizer.MustLocalizeError("kafka.acl.common.error.allAccountsCannotBeUsedWithUserFlag")
+			}


From what I can see we are not preventing the user from passing a wilcard to the user or service-account flags. Shouldn't we be?

craicoverflow · 2021-10-26T15:47:27Z

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

 one = '''
 # Grant access to principal for consuming messages from all topics
-$ rhoas kafka acl grant-access --consumer --user user_name --topic all --group all
+$ rhoas kafka acl grant-access --consumer --user user_name --topic "*" --group "*"


I believe we are sticking with aliases now?

craicoverflow

A couple of questions left inline.

craicoverflow

Looks great! I think we should have a second opinion on the cmd long and short descriptions though.

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

craicoverflow · 2021-10-27T11:54:57Z

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

+[kafka.acl.grantAdmin]
+
+[kafka.acl.grantAdmin.cmd.shortDescription]
+one = 'Give admin rights to the account'


Suggested change

one = 'Give admin rights to the account'

one = 'Grant an account permissions to create and delete ACLs in the Kafka instance'

The existing short description seemed a little too short and informal.

craicoverflow · 2021-10-27T11:56:31Z

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

+[kafka.acl.grantAdmin.cmd.shortDescription]
+one = 'Give admin rights to the account'
+
+[kafka.acl.grantAdmin.cmd.longDescription]
+one = 'This command will give specified account permission to create and delete ACLs in a Kafka instance.'


cc @jbyrne-redhat could review the CLI command documentation highlighted?

Suggest just a couple of minor tweaks:

'This command grants a specified account permission to create and delete ACLs in a Kafka instance.'

craicoverflow · 2021-10-27T12:03:15Z

pkg/cmd/kafka/acl/admin/admin.go

+		kafkainstanceclient.ACLPERMISSIONTYPE_ALLOW,
+	)
+
+	rows := aclutil.MapACLsToTableRows([]kafkainstanceclient.AclBinding{*aclBindClusterAlter}, opts.localizer)


We could probably skip that when -y is passed.

I would prefer to keep -y for the confirmation action only and display the ACLs to be generated in both scenario.

That's fair. I think it needs an explanation to give context to the table though because right now it prints without info about it.

❯ ./rhoas kafka acl grant-admin --all-accounts -y PRINCIPAL PERMISSION OPERATION DESCRIPTION -------------- ------------ ----------- ---------------------------- All accounts ALLOW ALTER CLUSTER is "kafka-cluster" ✔️ Account "*" is now allowed to create and delete ACLs for Kafka instance "enda-dev"

It might be nice to add something like:

The following ACLs will be created: ...

above it.

wtrocki · 2021-10-27T20:24:08Z

pkg/cmd/kafka/acl/admin/admin.go

+
+			opts.kafkaID = instanceID
+
+			// check if priincipal is provided


Suggested change

// check if priincipal is provided

// check if principal is provided

wtrocki

Reviewed and verified. Great job!

rkpattnaik780 requested a review from craicoverflow October 18, 2021 16:43

rkpattnaik780 force-pushed the acl_convenience_admin branch from b510c2d to b6a52d9 Compare October 20, 2021 06:59

rkpattnaik780 changed the title ~~Acl convenience admin~~ feat(kafka acl): add admin command Oct 20, 2021

craicoverflow suggested changes Oct 20, 2021

View reviewed changes

rkpattnaik780 force-pushed the acl_convenience_admin branch 2 times, most recently from 3797b73 to 43d2c9c Compare October 20, 2021 11:16

rkpattnaik780 requested a review from craicoverflow October 20, 2021 11:36

craicoverflow suggested changes Oct 20, 2021

View reviewed changes

craicoverflow self-requested a review October 21, 2021 09:01

rkpattnaik780 changed the title ~~feat(kafka acl): add admin command~~ feat(kafka acl): add grant-admin command Oct 21, 2021

rkpattnaik780 force-pushed the acl_convenience_admin branch from 90f0e1c to 63cd1f8 Compare October 21, 2021 13:13

rkpattnaik780 requested review from craicoverflow and removed request for craicoverflow October 26, 2021 14:55

craicoverflow reviewed Oct 26, 2021

View reviewed changes

rkpattnaik780 added 8 commits October 27, 2021 15:15

feat(kafka acl): add admin command

b9a8bd2

fix: address review comments

cdb7aee

fix: outdates examples

38b300a

fix: failing build

1597031

fix: add review and confirm prompt

f7c1357

fix: failing build for docs

02b3a35

fix: build failing for rebase

0cce846

fix: address review comments

e3da262

rkpattnaik780 force-pushed the acl_convenience_admin branch from 63cd1f8 to e3da262 Compare October 27, 2021 10:05

rkpattnaik780 requested a review from craicoverflow October 27, 2021 10:30

craicoverflow approved these changes Oct 27, 2021

View reviewed changes

fix: address review comments

778cfb6

wtrocki reviewed Oct 27, 2021

View reviewed changes

wtrocki approved these changes Oct 27, 2021

View reviewed changes

fix: typos and punctuations

9fef977

rkpattnaik780 force-pushed the acl_convenience_admin branch from a817214 to 9fef977 Compare October 28, 2021 06:47

rkpattnaik780 merged commit e77ed09 into main Oct 28, 2021

rkpattnaik780 deleted the acl_convenience_admin branch October 28, 2021 06:49

	one = 'Give admin rights to the account'
	one = 'Grant an account permissions to create and delete ACLs in the Kafka instance'

	// check if priincipal is provided
	// check if principal is provided

Conversation

rkpattnaik780 commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification Steps

Type of change

Checklist

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

rkpattnaik780 commented Oct 21, 2021

Uh oh!

craicoverflow commented Oct 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rkpattnaik780 commented Oct 18, 2021 •

edited

Loading