feat(kafka acl grant-access): add --all-accounts flag by rkpattnaik780 · Pull Request #1222 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-10-14T11:40:48Z

--all-accounts flag to grant permissions to all users and service accounts (substitute for --user "*" and --service-account "*").

Closes #1197

Verification Steps

The following command should grant access to all users for consuming messages from topic "my-topic"

rhoas kafka acl grant-permissions --consumer --all-accounts --topic my-topic --group my-group

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

wtrocki · 2021-10-14T12:09:05Z

See dicussion in #1197

craicoverflow · 2021-10-14T12:29:26Z

pkg/cmd/kafka/acl/grant/grant.go

+		userArg = buildPrincipal(acl.Wildcard)
+	}
+
 	if opts.svcAccount != "" {


We need to throw an error if user or service-account value is all. I have not tested whether it does this but I cannot see this.

Working on it. I am not sure if we should throw an error.

I think passing all should be left as it is?

If no error, what do you think should happen?

it should be processed as a simple identifier?
user is literal "all"

The user of the CLI will think running --user=all will limit the wildcard to users, not service accounts, but in fact it will apply it to all. That IMO is dangerous as it will give a false sense of security.

craicoverflow · 2021-10-18T08:26:38Z

I'm wondering if we should call it --all-accounts instead? This is how we describe it in the ACL list table, and it feel more natural to me.

craicoverflow · 2021-10-18T08:27:52Z

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

 [kafka.acl.common.flag.groupPrefix.description]
 one = 'Prefix name for groups to be selected'

+[kafka.acl.common.flag.allPrincipals]


Suggested change

[kafka.acl.common.flag.allPrincipals]

[kafka.acl.common.flag.allPrincipals.description]

Pattern has been {flagName}.description

rkpattnaik780 · 2021-10-18T12:25:21Z

I'm wondering if we should call it --all-accounts instead? This is how we describe it in the ACL list table, and it feel more natural to me.

Sticking to --all-accounts, UI uses accounts as well.

craicoverflow · 2021-10-18T14:23:51Z

pkg/cmd/kafka/acl/grant/grant.go

+	allAccounts bool
 }

 // NewGrantPermissionsACLCommand creates a series of ACL rules


Should be NewGrantAccess now.

craicoverflow · 2021-10-18T14:28:09Z

pkg/cmd/kafka/acl/grant/grant.go

 	if opts.user != "" {
-		user := getArgumentFromAlias(opts.user)
-		userArg = buildPrincipal(user)
+		if opts.user == acl.All {
+			return opts.localizer.MustLocalizeError("kafka.acl.common.error.allNotAllowed", localize.NewEntry("Flag", "user"))
+		}
+		userArg = buildPrincipal(opts.user)
+	}
+
+	if opts.allAccounts {
+		userArg = buildPrincipal(acl.Wildcard)
 	}


nit: I like the pattern of where opts only has principal value and you do not need to carry out this logic to determine which one to select (as you must then repeat these checks every time you want to reference the principal to use.

A cleaner approach might be to figure out the principal from the flags and assign it to opts in the input validation stage, like this:

app-services-cli/pkg/cmd/kafka/acl/delete/delete.go

Lines 294 to 298 in 3e66214

if userID != "" {

opts.principal = userID

} else if serviceAccount != "" {

opts.principal = serviceAccount

}

craicoverflow · 2021-10-19T11:09:35Z

pkg/cmd/kafka/acl/grant/grant.go

+	if userID == "" && serviceAccount == "" && !allAccounts {
 		return opts.localizer.MustLocalizeError("kafka.acl.grantPermissions.error.noPrincipalsSelected")
 	}

 	// user and service account should not be provided together
-	if opts.user != "" && opts.svcAccount != "" {
+	if userID != "" && serviceAccount != "" {
 		return opts.localizer.MustLocalizeError("kafka.acl.grantPermissions.error.bothPrincipalsSelected")
 	}


nit: these checks could go before the logic of assigning the correct principal to opts.principal

these checks are executed before assignment, it is the function definition here.

rkpattnaik780 requested review from craicoverflow and wtrocki October 14, 2021 11:40

craicoverflow reviewed Oct 14, 2021

View reviewed changes

rkpattnaik780 force-pushed the acl_convenience_all_principals branch from bbad6e1 to 610ec81 Compare October 18, 2021 07:05

rkpattnaik780 requested a review from craicoverflow October 18, 2021 07:07

craicoverflow reviewed Oct 18, 2021

View reviewed changes

rkpattnaik780 requested a review from craicoverflow October 18, 2021 13:31

craicoverflow changed the title ~~feat(kafka acl grant-permissions): add --all-principals flag~~ feat(kafka acl grant-permissions): add --all-accounts flag Oct 18, 2021

craicoverflow reviewed Oct 18, 2021

View reviewed changes

craicoverflow approved these changes Oct 18, 2021

View reviewed changes

rkpattnaik780 requested a review from craicoverflow October 19, 2021 10:21

rkpattnaik780 changed the title ~~feat(kafka acl grant-permissions): add --all-accounts flag~~ feat(kafka acl grant-access): add --all-accounts flag Oct 19, 2021

craicoverflow approved these changes Oct 19, 2021

View reviewed changes

feat(kafka acl grant-access): add --all-accounts flag

5fa07be

rkpattnaik780 force-pushed the acl_convenience_all_principals branch from 3f5a596 to 5fa07be Compare October 19, 2021 14:43

rkpattnaik780 merged commit d9fcb54 into main Oct 19, 2021

rkpattnaik780 deleted the acl_convenience_all_principals branch October 19, 2021 15:20

	[kafka.acl.common.flag.allPrincipals]
	[kafka.acl.common.flag.allPrincipals.description]

	if userID != "" {
	opts.principal = userID
	} else if serviceAccount != "" {
	opts.principal = serviceAccount
	}

Conversation

rkpattnaik780 commented Oct 14, 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!

wtrocki commented Oct 14, 2021

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkpattnaik780 commented Oct 18, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rkpattnaik780 commented Oct 14, 2021 •

edited

Loading

rkpattnaik780 Oct 14, 2021 •

edited

Loading