Fixes to `jimmctl auth relation list` #1220

pkulik0 · 2024-05-28T11:02:49Z

Description

This PR fixes a bugs in jimmctl auth relation list which cause:

an infinite loop of requesting the first page if the number of results go over defaultPageSize of 50.
invalid parsing of the serviceaccount tag

&

Small adjustments to the Makefile bring it up-to-date and unify naming of targets across files.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

github-actions · 2024-05-28T11:03:04Z

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

… referred to in other files

…to include a multipage result

kian99 · 2024-05-28T15:12:16Z

cmd/jimmctl/cmd/relation_test.go

+		_, err := cmdtesting.RunCommand(c, cmd.NewAddGroupCommandForTesting(s.ClientStore(), bClient), groupName)
+		c.Assert(err, gc.IsNil)
+
+		tabularOutput += fmt.Sprintf("\[email protected]  	member       	group-%-72s", groupName)


How come the use of -72s? I had to google as I don't see this often, for others, it left-justifies the text and prints a minimum of 72 chars.

The output has whitespace after each line to match the length of the longest row. 72 is a hard-coded diff between the length of this string without groupName and the required line length.

Maybe leave a comment to that effect, if someone were to add a new relation that increases/remove a relation and decreases the longest row, that will be annoying to debug.

cmd/jimmctl/cmd/relation_test.go

…ion for testing

babakks

Thanks for this. Just a few comments.

babakks · 2024-05-29T11:08:13Z

Makefile

@@ -111,13 +111,12 @@ endef

 # Install packages required to develop JIMM and run tests.
 APT_BASED := $(shell command -v apt-get >/dev/null; echo $$?)
-sysdeps:
+sys-deps:


As I checked there are still a few more instances that point to sysdeps. Can you please update them as well?

babakks · 2024-05-29T11:19:22Z

cmd/jimmctl/cmd/relation.go

 	response, err := client.ListRelationshipTuples(&params)
 	if err != nil {
 		return nil, errors.E(err)
 	}
 	if response.ContinuationToken != "" {
-		response1, err := fetchRelations(client, params, response.ContinuationToken)
+		params.ContinuationToken = response.ContinuationToken
+		response1, err := fetchRelations(client, params)


(suggestion) Now that you're fixing this, can you please also refactor it and get rid of the recursive calls to fetchRelations? This should just be a simple loop that runs until it gets the same continuation token back.

We have done this kind of loop before. You can just replicate it.

https://github.com/babakks/jimm/blob/97e203f43c51969887aa52d524c28b3f05bf13a4/internal/openfga/user.go#L375-L393

babakks · 2024-05-29T12:30:51Z

cmd/jimmctl/cmd/relation_test.go

 	c.Assert(err, gc.IsNil)
+	c.Assert(cmdtesting.Stdout(context), gc.Equals, relationsToTabularStr(expectedRelations))


I understand what you did here, but I think this actually is a bit against the purpose of the test.

The thing is, with this change, we'll have two implementations for tabular printing; one in the code (i.e., the formatRelationsTabular function that uses utiable under-the-hood), and the new relationsToTabularStr that you've made for testing. What we want to do is to make sure the former does its job as expected. So, we need to compare the output with something that we are absolutely sure that it's accurate. But, if we use the relationsToTabularStr helper method, we can never be sure because the helper method is not tested. Now, if we attempt to write a test for it, we again have to compare the output with a literal multiline string, like what we originally had in the test.

I'm not sure if I've conveyed my point clearly. But the gist is, if possible, we shouldn't assert against something that is not totally trusted to work as expected.

I fully agree with this point, but in this situation I considered it trival enough to prefer a helper over an easily breakable hard-coded multiline string that requires perfectly matching whitespace on each line which gives you a headache on every change. The perspective of changing a few strings like this after possibly more tests get added seems dreading compared to, in my opinion, the minimal stink of this helper given its simple logic.

let's get rid of the helper function, use export_test to export the formatting function used by the command and use that to write the expected output to a string, then compare.. i don't think there's any value in having a separate helper function here.. if we ever change tabular format, we'll need to change the helper function, too..

babakks · 2024-05-29T12:42:50Z

internal/jimm/access.go

+	case jimmnames.ServiceAccountTagKind:
+		return jimmnames.ServiceAccountTagKind + "-" + tag.ID, nil


Thanks for adding this. We need to update the tests to cover this as well. You can find the test function here:

https://github.com/babakks/jimm/blob/2750ee5242c18cbdb0ebb4c548ab6c2b367ae7c9/internal/jujuapi/access_control_test.go#L791

Please, in addition to a test case for the service account tag, add one more for the group tags (which we've missed adding before).

I think this is still missing, a test in access_control.go for the changes to ToJAASTag.

babakks · 2024-05-29T12:49:48Z

internal/jimm/access.go

+	case jimmnames.ServiceAccountTagKind:
+		zapctx.Debug(
+			ctx,
+			"Resolving JIMM tags to Juju tags for tag kind: serviceaccount",
+			zap.String("serviceaccount-name", trailer),
+		)
+		return ofganames.ConvertTagWithRelation(jimmnames.NewServiceAccountTag(trailer), relation), nil


(question) @alesstimec @ale8k @kian99 Just want to make sure; we cannot check for the service account existence at this stage? Because the tag might exist but the the service account has not yet logged in (and hence to Identity entry is associated to it), right?

i don't think we need a DB lookup at this point, no.. it's better to show "stale" tuples that might be left-overs and give admins the ability to delete them

@babakks I believe that is correct:

the tag might exist but the the service account has not yet logged

Users work the same way

alesstimec · 2024-05-30T06:33:03Z

cmd/jimmctl/cmd/relation_test.go

 	c.Assert(err, gc.IsNil)
+	c.Assert(cmdtesting.Stdout(context), gc.Equals, relationsToTabularStr(expectedRelations))


let's get rid of the helper function, use export_test to export the formatting function used by the command and use that to write the expected output to a string, then compare.. i don't think there's any value in having a separate helper function here.. if we ever change tabular format, we'll need to change the helper function, too..

alesstimec · 2024-05-30T06:35:10Z

internal/jimm/access.go

+	case jimmnames.ServiceAccountTagKind:
+		zapctx.Debug(
+			ctx,
+			"Resolving JIMM tags to Juju tags for tag kind: serviceaccount",
+			zap.String("serviceaccount-name", trailer),
+		)
+		return ofganames.ConvertTagWithRelation(jimmnames.NewServiceAccountTag(trailer), relation), nil


i don't think we need a DB lookup at this point, no.. it's better to show "stale" tuples that might be left-overs and give admins the ability to delete them

kian99 · 2024-06-13T06:16:09Z

cmd/jimmctl/cmd/relation.go

+			response.Tuples = tuples
+			return response, nil


This is a bit confusing to read because we are creating a new response object in each loop and then at the end using the last response. I'd prefer creating a response object above the for loop, and then renaming the current response variable to page, then append tuples to response.Tuples and finally return the response.

I agree with @kian99. We can just create a new apiparams.ListRelationshipTuplesResponse instance after the end of the loop and assign the tuples.

kian99 · 2024-06-13T06:18:20Z

internal/jimm/access.go

+	case jimmnames.ServiceAccountTagKind:
+		return jimmnames.ServiceAccountTagKind + "-" + tag.ID, nil


I think this is still missing, a test in access_control.go for the changes to ToJAASTag.

babakks · 2024-06-13T09:45:14Z

cmd/jimmctl/cmd/export_test.go

@@ -321,3 +323,7 @@ func NewMigrateModelCommandForTesting(store jujuclient.ClientStore, lp jujuapi.L

 	return modelcmd.WrapBase(cmd)
 }
+
+func FormatRelationsTabularForTesting(writer io.Writer, value interface{}) error {


Since it's just for the sake of exposing formatRelationsTabular you can do it like this (which makes it more consistent with other same usages like this one):

var FormatRelationsTabular = formatRelationsTabular

babakks · 2024-06-13T09:48:06Z

cmd/jimmctl/cmd/relation.go

-		response1, err := fetchRelations(client, params)
+	tuples := make([]apiparams.RelationshipTuple, 0)
+	for {
+		response, err := client.ListRelationshipTuples(&params)
 		if err != nil {
 			return nil, errors.E(err)


(suggestion) Can you please add an error message to this, like:

return nil, errors.E(err, "failed to fetch list of relationship tuples")

Actually I'd caution against this because it would hide the underlying error. JIMM's error package is very poor in that it doesn't provide a clean way of masking or surfacing the underlying error. So here I think the best thing to do would be

errors.E(fmt.Sprintf("message: %s",err.Error()))

We'll need to improve it in the future but for now consider where you want to mask errors and where you want to present them to the user.

babakks · 2024-06-13T09:50:55Z

cmd/jimmctl/cmd/relation.go

+			response.Tuples = tuples
+			return response, nil


I agree with @kian99. We can just create a new apiparams.ListRelationshipTuplesResponse instance after the end of the loop and assign the tuples.

…les clearer

alesstimec

LGTM

pkulik0 requested review from alesstimec, kian99, ale8k and babakks as code owners May 28, 2024 11:02

pkulik0 added 2 commits May 28, 2024 11:28

Remove outdated docker compose check, rename target to match how it's…

91a690b

… referred to in other files

Fix incorrect paging in fetchRelations, adjust TestListRelations …

8240162

…to include a multipage result

pkulik0 force-pushed the CSS-8387-Fix-`jimmctl-auth-relation-list` branch from f3003f3 to 8240162 Compare May 28, 2024 11:28

kian99 approved these changes May 28, 2024

View reviewed changes

Add comment about tabular output padding

5a90332

pkulik0 force-pushed the CSS-8387-Fix-`jimmctl-auth-relation-list` branch from d050539 to 5a90332 Compare May 28, 2024 16:31

Add serviceaccount tag to parsing logic, add relations to table funct…

d9e2b53

…ion for testing

pkulik0 changed the title ~~Fix relations pagination in jimmctl auth relation list~~ Fixes to jimmctl auth relation list May 29, 2024

pkulik0 requested a review from kian99 May 29, 2024 08:53

Update Makefile help message

3f0c0b8

babakks suggested changes May 29, 2024

View reviewed changes

alesstimec requested changes May 30, 2024

View reviewed changes

pkulik0 and others added 6 commits June 11, 2024 13:19

Remove helper fn, export formatRelationsTabular for tests

09ba675

Merge branch 'v3' into CSS-8387-Fix-jimmctl-auth-relation-list

cfdd6d5

Merge v3 into CSS 8387

9d3e4ed

Change fetchRelations to a loop based approach

bae9a7f

Fix type of expectedData & order of paginated relations

45602ea

Merge branch 'v3' into CSS-8387-Fix-jimmctl-auth-relation-list

73ed66d

kian99 reviewed Jun 13, 2024

View reviewed changes

babakks approved these changes Jun 13, 2024

View reviewed changes

Add test cases for ToJAASTag, make the way fetchRelations returns tup…

deb676a

…les clearer

pkulik0 requested a review from a team as a code owner June 13, 2024 13:14

pkulik0 added 3 commits June 13, 2024 13:15

Rename remaining instances of sysdeps to sys-deps

97b0cfb

Add error message after a failed fetch of relationship tuples

bf4fa43

Simplify export of FormatRelationsTabular for tests

fd4638d

alesstimec approved these changes Jun 13, 2024

View reviewed changes

Merge branch 'v3' into CSS-8387-Fix-jimmctl-auth-relation-list

0e588b3

pkulik0 merged commit 0bceaa9 into v3 Jun 13, 2024
4 checks passed

pkulik0 deleted the CSS-8387-Fix-`jimmctl-auth-relation-list` branch June 13, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to `jimmctl auth relation list` #1220

Fixes to `jimmctl auth relation list` #1220

pkulik0 commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024

kian99 May 28, 2024

pkulik0 May 28, 2024

kian99 May 28, 2024

babakks left a comment

babakks May 29, 2024

babakks May 29, 2024 •

edited

Loading

babakks May 29, 2024

pkulik0 May 29, 2024

alesstimec May 30, 2024

babakks May 29, 2024

kian99 Jun 13, 2024

babakks May 29, 2024

alesstimec May 30, 2024

kian99 May 30, 2024

alesstimec May 30, 2024

alesstimec May 30, 2024

kian99 Jun 13, 2024

babakks Jun 13, 2024

kian99 Jun 13, 2024

babakks Jun 13, 2024

babakks Jun 13, 2024

kian99 Jun 13, 2024

babakks Jun 13, 2024

alesstimec left a comment

		c.Assert(err, gc.IsNil)
		c.Assert(cmdtesting.Stdout(context), gc.Equals, relationsToTabularStr(expectedRelations))

		case jimmnames.ServiceAccountTagKind:
		return jimmnames.ServiceAccountTagKind + "-" + tag.ID, nil

Fixes to jimmctl auth relation list #1220

Fixes to jimmctl auth relation list #1220

Conversation

pkulik0 commented May 28, 2024 • edited Loading

Description

Engineering checklist

github-actions bot commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babakks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babakks May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alesstimec left a comment

Choose a reason for hiding this comment

Fixes to `jimmctl auth relation list` #1220

Fixes to `jimmctl auth relation list` #1220

pkulik0 commented May 28, 2024 •

edited

Loading

babakks May 29, 2024 •

edited

Loading