Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to jimmctl auth relation list #1220

Merged
merged 16 commits into from
Jun 13, 2024
Merged

Fixes to jimmctl auth relation list #1220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
91a690b
Remove outdated docker compose check, rename target to match how it's…
pkulik0 May 28, 2024
8240162
Fix incorrect paging in `fetchRelations`, adjust `TestListRelations` …
pkulik0 May 28, 2024
5a90332
Add comment about tabular output padding
pkulik0 May 28, 2024
d9e2b53
Add serviceaccount tag to parsing logic, add relations to table funct…
pkulik0 May 29, 2024
3f0c0b8
Update Makefile help message
pkulik0 May 29, 2024
09ba675
Remove helper fn, export formatRelationsTabular for tests
pkulik0 Jun 11, 2024
cfdd6d5
Merge branch 'v3' into CSS-8387-Fix-`jimmctl-auth-relation-list`
pkulik0 Jun 11, 2024
9d3e4ed
Merge v3 into CSS 8387
pkulik0 Jun 11, 2024
bae9a7f
Change fetchRelations to a loop based approach
pkulik0 Jun 11, 2024
45602ea
Fix type of expectedData & order of paginated relations
pkulik0 Jun 13, 2024
73ed66d
Merge branch 'v3' into CSS-8387-Fix-`jimmctl-auth-relation-list`
pkulik0 Jun 13, 2024
deb676a
Add test cases for ToJAASTag, make the way fetchRelations returns tup…
pkulik0 Jun 13, 2024
97b0cfb
Rename remaining instances of `sysdeps` to `sys-deps`
pkulik0 Jun 13, 2024
bf4fa43
Add error message after a failed fetch of relationship tuples
pkulik0 Jun 13, 2024
fd4638d
Simplify export of `FormatRelationsTabular` for tests
pkulik0 Jun 13, 2024
0e588b3
Merge branch 'v3' into CSS-8387-Fix-`jimmctl-auth-relation-list`
pkulik0 Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ifeq ($(APT_BASED),0)
@$(call check_dep,go,Missing Go - install from https://go.dev/doc/install or 'sudo snap install go')
@$(call check_dep,git,Missing Git - install with 'sudo apt install git')
@$(call check_dep,gcc,Missing gcc - install with 'sudo apt install build-essentials')
@$(call check_dep,docker,Missing Docker - install from https://docs.docker.com/engine/install/')
@$(call check_dep,docker-compose,Missing Docker Compose - install from https://docs.docker.com/engine/install/')
@$(call check_dep,juju-db.mongo,Missing juju-db - install with 'sudo snap install juju-db --channel=4.4/stable')
else
@echo sysdeps runs only on systems with apt-get
Expand Down
1 change: 1 addition & 0 deletions cmd/jimmctl/cmd/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var (
AccessMessage = accessMessageFormat
AccessResultAllowed = accessResultAllowed
AccessResultDenied = accessResultDenied
DefaultPageSize = defaultPageSize
)

type AccessResult = accessResult
Expand Down
9 changes: 5 additions & 4 deletions cmd/jimmctl/cmd/relation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ Examples:
returns the list of relations, where target object
matches the specified one

jimmctl auth relation list --target <target_object> --relation <relation
jimmctl auth relation list --target <target_object> --relation <relation>
returns the list of relations, where target object
and relation match the specified ones
`
Expand Down Expand Up @@ -544,7 +544,7 @@ func (c *listRelationsCommand) Run(ctxt *cmd.Context) error {
Tuple: c.tuple,
PageSize: defaultPageSize,
}
result, err := fetchRelations(client, params, "")
result, err := fetchRelations(client, params)
if err != nil {
return errors.E(err)
}
Expand All @@ -557,13 +557,14 @@ func (c *listRelationsCommand) Run(ctxt *cmd.Context) error {
return nil
}

func fetchRelations(client *api.Client, params apiparams.ListRelationshipTuplesRequest, continuationToken string) (*apiparams.ListRelationshipTuplesResponse, error) {
func fetchRelations(client *api.Client, params apiparams.ListRelationshipTuplesRequest) (*apiparams.ListRelationshipTuplesResponse, error) {
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)
Copy link
Member

@babakks babakks May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

if err != nil {
return nil, errors.E(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Expand Down
62 changes: 31 additions & 31 deletions cmd/jimmctl/cmd/relation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,20 +347,10 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
// alice is superuser
bClient := jimmtest.NewUserSessionLogin(c, "alice")

groups := []string{"group-1", "group-2", "group-3"}
for _, group := range groups {
_, err := cmdtesting.RunCommand(c, cmd.NewAddGroupCommandForTesting(s.ClientStore(), bClient), group)
c.Assert(err, gc.IsNil)
}

relations := []apiparams.RelationshipTuple{{
Object: "user-" + env.users[0].Name,
Relation: "member",
TargetObject: "group-group-1",
}, {
Object: "user-" + env.users[1].Name,
Relation: "member",
TargetObject: "group-group-2",
}, {
Object: "group-group-2#member",
Relation: "member",
Expand All @@ -378,11 +368,38 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
Relation: "administrator",
TargetObject: "applicationoffer-" + env.controllers[0].Name + ":" + env.applicationOffers[0].Model.OwnerIdentityName + "/" + env.applicationOffers[0].Model.Name + "." + env.applicationOffers[0].Name,
}}

tabularOutput := `Object Relation Target Object
user-admin administrator controller-jimm
[email protected] administrator controller-jimm
[email protected] member group-group-1
group-group-2#member member group-group-3
group-group-3#member administrator controller-test-controller-1
group-group-1#member administrator model-test-controller-1:[email protected]/test-model-1
[email protected] administrator applicationoffer-test-controller-1:[email protected]/test-model-1.testoffer1`

for i := 0; i < cmd.DefaultPageSize+1; i++ {
kian99 marked this conversation as resolved.
Show resolved Hide resolved
groupName := fmt.Sprintf("group-%d", i)
_, err := cmdtesting.RunCommand(c, cmd.NewAddGroupCommandForTesting(s.ClientStore(), bClient), groupName)
c.Assert(err, gc.IsNil)

tabularOutput += fmt.Sprintf("\[email protected] member group-%-72s", groupName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

relations = append(relations, apiparams.RelationshipTuple{
Object: "user-" + env.users[1].Name,
Relation: "member",
TargetObject: "group-" + groupName,
})
}

for _, relation := range relations {
_, err := cmdtesting.RunCommand(c, cmd.NewAddRelationCommandForTesting(s.ClientStore(), bClient), relation.Object, relation.Relation, relation.TargetObject)
c.Assert(err, gc.IsNil)
}

context, err := cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "tabular")
c.Assert(err, gc.IsNil)
c.Assert(cmdtesting.Stdout(context), gc.Equals, tabularOutput)

expectedJSONData, err := json.Marshal(append(
[]apiparams.RelationshipTuple{{
Object: "user-admin",
Expand All @@ -396,6 +413,10 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
relations...,
))
c.Assert(err, gc.IsNil)
context, err = cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "json")
c.Assert(err, gc.IsNil)
c.Assert(strings.TrimRight(cmdtesting.Stdout(context), "\n"), gc.Equals, string(expectedJSONData))

expectedYAMLData, err := yaml.Marshal(append(
[]apiparams.RelationshipTuple{{
Object: "user-admin",
Expand All @@ -409,30 +430,9 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
relations...,
))
c.Assert(err, gc.IsNil)

context, err := cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "json")
c.Assert(err, gc.IsNil)
c.Assert(strings.TrimRight(cmdtesting.Stdout(context), "\n"), gc.Equals, string(expectedJSONData))

context, err = cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient))
c.Assert(err, gc.IsNil)
c.Assert(cmdtesting.Stdout(context), gc.Equals, string(expectedYAMLData))

context, err = cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "tabular")
c.Assert(err, gc.IsNil)
c.Assert(
cmdtesting.Stdout(context),
gc.Equals,
`Object Relation Target Object
user-admin administrator controller-jimm
[email protected] administrator controller-jimm
[email protected] member group-group-1
[email protected] member group-group-2
group-group-2#member member group-group-3
group-group-3#member administrator controller-test-controller-1
group-group-1#member administrator model-test-controller-1:[email protected]/test-model-1
[email protected] administrator applicationoffer-test-controller-1:[email protected]/test-model-1.testoffer1`,
)
}

// TODO: remove boilerplate of env setup and use initialiseEnvironment
Expand Down
Loading