CLI-979 Refactor `ccloud kafka acl` parameter names to match those of REST #943

MuweiHe · 2021-07-27T00:21:49Z

Checklist

[CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
- yes: ok
Did you add/update any commands that accept secrets as args/flags?
- no: ok

What

Change ccloud kafka acl flag names to match the REST Proxy params

Changes would be: {"ServiceAccountId", "Permission", "Operation", "Resource", "Name", "Type"} --> {"Principal", "Permission", "Operation", "ResourceType", "ResourceName", "PatternType"}

Note: this is a breaking change that would ship in ccloud v2.0

References

Test&Review

…ing REST Proxy API

internal/cmd/kafka/command_acl.go

DABH

Some questions about some of the fields, in particular Principal vs. ServiceAccountId, but otherwise this is mostly good

internal/cmd/ksql/command_test.go

DABH · 2021-07-28T04:31:42Z

internal/pkg/acl/acl.go

@@ -345,8 +345,8 @@ func PrintACLsFromKafkaRestResponseWithMap(cmd *cobra.Command, aclGetResp kafkar
 		cmd.Flags().StringP(output.FlagName, output.ShortHandFlag, output.DefaultValue, output.Usage)
 	}

-	aclListFields := []string{"UserId", "ServiceAccountId", "Permission", "Operation", "Resource", "Name", "Type"}
-	aclListStructuredRenames := []string{"user_id", "service_account_id", "permission", "operation", "resource", "name", "type"}
+	aclListFields := []string{"Principal", "ServiceAccountId", "Permission", "Operation", "ResourceType", "ResourceName", "PatternType"}


Oh, why do we have both Principal and ServiceAccountId here?

principal is like 'User: 12345' and service account is 'sa-12345'

So does the REST Proxy API also have a service account ID field / service account resource ID field? Or are you saying that this is just an extra field that we show in the CLI, even though the REST Proxy API doesn't use it?

No the acl data doesn't have a sa id field. we're doing a map lookup using the principal to get the corresponding sa id.

@MuweiHe I commented about this below, but we should check if ACLs can only be created on a per SA basis, or if they can also be made on a per User basis.

But either way, I think you're using the integer ID to do the map lookup to get the resource ID for the service account. Since we're trying to deprecate the integer ID, we should just replace the integer ID with the resource ID in the principal column. I.e. Something like this:

Principal:

User:sa-123

@mtodzo ACLs only make sense when created for a sa to my understanding 'cause saID is a required param, and admin users should already have access to resources thus don't need acls. Right now the backend service are using the numeric ID principal like 'User:123' to manage acls. So for now I can do a look up from 'User:123' to 'sa-123' and put that under the Principal column. Is that ok?

mtodzo

We should update the --service-account flag as well to be --principal. Also, we should figure out whether ACLs can be configured for users (see #929 (comment))

MuweiHe · 2021-07-30T21:47:55Z

We should update the --service-account flag as well to be --principal. Also, we should figure out whether ACLs can be configured for users (see #929 (comment))

like in {"Principal", "ServiceAccountId", "Permission", "Operation", "ResourceType", "ResourceName", "PatternType"} The first column is like 'User: 1234' the second 'sa-12345'. So do we remove the first column and rename the second to be principal?

mtodzo · 2021-07-30T22:00:55Z

We should update the --service-account flag as well to be --principal. Also, we should figure out whether ACLs can be configured for users (see #929 (comment))

like in {"Principal", "ServiceAccountId", "Permission", "Operation", "ResourceType", "ResourceName", "PatternType"} The first column is like 'User: 1234' the second 'sa-12345'. So do we remove the first column and rename the second to be principal?

Yeah, except it should probably include the principal prefix, so it would be User:sa-12345 ... maybe check with Ethan, but yeah the 1234 in User:1234 is the numeric ID that we don't want to show.

test/fixtures/output/kafka/kafka-acls-list.golden

internal/pkg/acl/acl.go

DABH

I left a question, but, this looks good to me overall. I would like @mtodzo to give the final +1 though since he is most familiar with the REST interface. Thank you!

internal/pkg/sso/auth_state.go

mtodzo

few comments, but after those looks good!

internal/cmd/kafka/command_acl.go

internal/pkg/acl/acl.go

mtodzo · 2021-09-07T16:39:14Z

test/fixtures/output/kafka/kafka-acls-create.golden

-  User:0           | ALLOW      | DESCRIBE  | TOPIC    | 'test-topic' | LITERAL  
+  Principal | Permission | Operation | ResourceType | ResourceName | PatternType  
+-----------+------------+-----------+--------------+--------------+-------------+
+  User:     | ALLOW      | READ      | TOPIC        | 'test-topic' | LITERAL      


is this expected to be empty? User:

This file is not used anywhere... I might have changed it to be safe.

Should we delete it if it's not used anywhere?

changes addressed

mtodzo

lgtm!

Refactor ccloud kafka acl parameter names to match those of underly…

d0cc857

…ing REST Proxy API

MuweiHe requested a review from a team as a code owner July 27, 2021 00:21

MuweiHe requested review from DABH and brianstrauch July 27, 2021 00:22

Update kafka-acls-list.golden

6ed9915

brianstrauch reviewed Jul 27, 2021

View reviewed changes

internal/cmd/kafka/command_acl.go Outdated Show resolved Hide resolved

rename IdMap and fix ksql tests

981d17d

DABH suggested changes Jul 28, 2021

View reviewed changes

rename saId to principal

9167a84

MuweiHe requested review from DABH and brianstrauch July 28, 2021 19:03

mtodzo previously requested changes Jul 30, 2021

View reviewed changes

MuweiHe force-pushed the CLI-979-2.0 branch 2 times, most recently from 8abee17 to 9167a84 Compare July 30, 2021 23:05

delete service account id column

a5f3d05

MuweiHe requested a review from mtodzo July 30, 2021 23:11

MuweiHe and others added 3 commits August 3, 2021 10:45

parse userID by splitting by colon

38e86c1

revert split principal by colon

cf5cbe5

Merge branch '2.0' into CLI-979-2.0

95e35ec

mtodzo reviewed Aug 13, 2021

View reviewed changes

test/fixtures/output/kafka/kafka-acls-list.golden Outdated Show resolved Hide resolved

internal/pkg/acl/acl.go Outdated Show resolved Hide resolved

MuweiHe added 2 commits August 16, 2021 23:06

modify acl tests

e01e152

Update auth_state.go

74c044c

DABH reviewed Sep 5, 2021

View reviewed changes

internal/pkg/sso/auth_state.go Outdated Show resolved Hide resolved

mtodzo reviewed Sep 7, 2021

View reviewed changes

MuweiHe added 3 commits September 8, 2021 13:33

Merge remote-tracking branch 'origin/2.0' into CLI-979-2.0

9bb2f5a

made changes according to comments

ea48059

fix kafka acl tests

f10474b

update TestConfluentKafkaACL

6800ee7

mtodzo self-requested a review September 9, 2021 04:06

mtodzo approved these changes Sep 9, 2021

View reviewed changes

MuweiHe merged commit 0d970d0 into 2.0 Sep 9, 2021

MuweiHe deleted the CLI-979-2.0 branch September 9, 2021 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI-979 Refactor `ccloud kafka acl` parameter names to match those of REST #943

CLI-979 Refactor `ccloud kafka acl` parameter names to match those of REST #943

MuweiHe commented Jul 27, 2021

DABH left a comment

DABH Jul 28, 2021

MuweiHe Jul 28, 2021

DABH Jul 28, 2021

MuweiHe Jul 28, 2021

mtodzo Jul 30, 2021 •

edited

Loading

MuweiHe Jul 30, 2021 •

edited

Loading

mtodzo left a comment

MuweiHe commented Jul 30, 2021

mtodzo commented Jul 30, 2021

DABH left a comment

mtodzo left a comment

mtodzo Sep 7, 2021

MuweiHe Sep 8, 2021

mtodzo Sep 9, 2021

mtodzo left a comment

CLI-979 Refactor ccloud kafka acl parameter names to match those of REST #943

CLI-979 Refactor ccloud kafka acl parameter names to match those of REST #943

Conversation

MuweiHe commented Jul 27, 2021

Checklist

What

References

Test&Review

DABH left a comment

Choose a reason for hiding this comment

DABH Jul 28, 2021

Choose a reason for hiding this comment

MuweiHe Jul 28, 2021

Choose a reason for hiding this comment

DABH Jul 28, 2021

Choose a reason for hiding this comment

MuweiHe Jul 28, 2021

Choose a reason for hiding this comment

mtodzo Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Principal:

MuweiHe Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

mtodzo left a comment

Choose a reason for hiding this comment

MuweiHe commented Jul 30, 2021

mtodzo commented Jul 30, 2021

DABH left a comment

Choose a reason for hiding this comment

mtodzo left a comment

Choose a reason for hiding this comment

mtodzo Sep 7, 2021

Choose a reason for hiding this comment

MuweiHe Sep 8, 2021

Choose a reason for hiding this comment

mtodzo Sep 9, 2021

Choose a reason for hiding this comment

mtodzo left a comment

Choose a reason for hiding this comment

CLI-979 Refactor `ccloud kafka acl` parameter names to match those of REST #943

CLI-979 Refactor `ccloud kafka acl` parameter names to match those of REST #943

mtodzo Jul 30, 2021 •

edited

Loading

MuweiHe Jul 30, 2021 •

edited

Loading