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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 6 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ clean:
certs:
@cd local/traefik/certs; ./certs.sh; cd -

test-env: sysdeps certs
test-env: sys-deps certs
@touch ./local/vault/approle.json && touch ./local/vault/roleid.txt && touch ./local/vault/vault.env
@docker compose up --force-recreate -d --wait

test-env-cleanup:
@docker compose down -v --remove-orphans

dev-env-setup: sysdeps certs
dev-env-setup: sys-deps certs
@touch ./local/vault/approle.json && touch ./local/vault/roleid.txt && touch ./local/vault/vault.env
@make version/commit.txt && make version/version.txt

Expand Down Expand Up @@ -115,18 +115,17 @@ 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,yq,Missing yq - install with 'sudo snap install yq')
@$(call check_dep,gcc,Missing microk8s - install latest none-classic from snapstore')
@$(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
@echo sys-deps runs only on systems with apt-get
@echo on OS X with homebrew try: brew install bazaar mongodb
endif

Expand All @@ -137,13 +136,13 @@ help:
@echo 'make install - Install the package.'
@echo 'make server - Start the JIMM server.'
@echo 'make clean - Remove object files from package source directories.'
@echo 'make sysdeps - Install the development environment system packages.'
@echo 'make sys-deps - Install the development environment system packages.'
@echo 'make format - Format the source files.'
@echo 'make simplify - Format and simplify the source files.'
@echo 'make get-local-auth - Get local auth to the API WSS endpoint locally.'
@echo 'make rock - Build the JIMM rock.'
@echo 'make load-rock - Load the most recently built rock into your local docker daemon.'

.PHONY: build check install release clean format server simplify sysdeps help FORCE
.PHONY: build check install release clean format server simplify sys-deps help FORCE

FORCE:
8 changes: 5 additions & 3 deletions cmd/jimmctl/cmd/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ import (
)

var (
AccessMessage = accessMessageFormat
AccessResultAllowed = accessResultAllowed
AccessResultDenied = accessResultDenied
AccessMessage = accessMessageFormat
AccessResultAllowed = accessResultAllowed
AccessResultDenied = accessResultDenied
DefaultPageSize = defaultPageSize
FormatRelationsTabular = formatRelationsTabular
)

type AccessResult = accessResult
Expand Down
25 changes: 13 additions & 12 deletions cmd/jimmctl/cmd/relation.go
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 @@ -559,19 +559,20 @@ func (c *listRelationsCommand) Run(ctxt *cmd.Context) error {
return nil
}

func fetchRelations(client *api.Client, params apiparams.ListRelationshipTuplesRequest, continuationToken string) (*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)
func fetchRelations(client *api.Client, params apiparams.ListRelationshipTuplesRequest) (*apiparams.ListRelationshipTuplesResponse, error) {
tuples := make([]apiparams.RelationshipTuple, 0)
for {
response, err := client.ListRelationshipTuples(&params)
if err != nil {
return nil, errors.E(err)
return nil, errors.E(fmt.Sprintf("failed to fetch list of relationship tuples: %s", err.Error()))
}
tuples = append(tuples, response.Tuples...)

if response.ContinuationToken == "" {
return &apiparams.ListRelationshipTuplesResponse{Tuples: tuples, Errors: response.Errors}, nil
}
response.Tuples = append(response.Tuples, response1.Tuples...)
params.ContinuationToken = response.ContinuationToken
}
return response, nil
}

func formatRelationsTabular(writer io.Writer, value interface{}) error {
Expand Down
75 changes: 33 additions & 42 deletions cmd/jimmctl/cmd/relation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,23 +343,12 @@ func initializeEnvironment(c *gc.C, ctx context.Context, db *db.Database, u dbmo

func (s *relationSuite) TestListRelations(c *gc.C) {
env := initializeEnvironment(c, context.Background(), &s.JIMM.Database, *s.AdminUser)
// 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)
}
bClient := jimmtest.NewUserSessionLogin(c, "alice") // alice is superuser

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 @@ -376,7 +365,24 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
Object: "user-" + env.users[1].Name,
Relation: "administrator",
TargetObject: "applicationoffer-" + env.controllers[0].Name + ":" + env.applicationOffers[0].Model.OwnerIdentityName + "/" + env.applicationOffers[0].Model.Name + "." + env.applicationOffers[0].Name,
}, {
Object: "user-" + env.users[0].Name,
Relation: "administrator",
TargetObject: "serviceaccount-test@serviceaccount",
}}

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)

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)
Expand All @@ -394,35 +400,26 @@ func (s *relationSuite) TestListRelations(c *gc.C) {
}},
relations...,
)}
expectedJSONData, err := json.Marshal(expectedData)

context, err := cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "tabular")
c.Assert(err, gc.IsNil)
// Necessary to use yamlv2 to match what Juju does.
expectedYAMLData, err := yamlv2.Marshal(expectedData)
var builder strings.Builder
err = cmd.FormatRelationsTabular(&builder, &expectedData)
c.Assert(err, gc.IsNil)
c.Assert(cmdtesting.Stdout(context), gc.Equals, builder.String())

context, err := cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "json")
expectedJSONData, err := json.Marshal(expectedData)
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))

// Necessary to use yamlv2 to match what Juju does.
expectedYAMLData, err := yamlv2.Marshal(expectedData)
c.Assert(err, gc.IsNil)
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`,
)
}

func (s *relationSuite) TestListRelationsWithError(c *gc.C) {
Expand Down Expand Up @@ -482,16 +479,10 @@ func (s *relationSuite) TestListRelationsWithError(c *gc.C) {

context, err = cmdtesting.RunCommand(c, cmd.NewListRelationsCommandForTesting(s.ClientStore(), bClient), "--format", "tabular")
c.Assert(err, gc.IsNil)
expectedOutput := fmt.Sprintf(
`Object Relation Target Object
user-admin administrator controller-jimm
[email protected] administrator controller-jimm
[email protected] member group:%s

Errors
failed to parse target: failed to fetch group information: %s
`, group.UUID, group.UUID)
c.Assert(cmdtesting.Stdout(context), gc.Equals, expectedOutput)
var builder strings.Builder
err = cmd.FormatRelationsTabular(&builder, &expectedData)
c.Assert(err, gc.IsNil)
c.Assert(cmdtesting.Stdout(context), gc.Equals, builder.String())
}

// TODO: remove boilerplate of env setup and use initialiseEnvironment
Expand Down
9 changes: 9 additions & 0 deletions internal/jimm/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ func (j *JIMM) ToJAASTag(ctx context.Context, tag *ofganames.Tag) (string, error
switch tag.Kind {
case names.UserTagKind:
return names.UserTagKind + "-" + tag.ID, nil
case jimmnames.ServiceAccountTagKind:
return jimmnames.ServiceAccountTagKind + "-" + tag.ID, nil
Comment on lines +397 to +398
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

case names.ControllerTagKind:
if tag.ID == j.ResourceTag().Id() {
return "controller-jimm", nil
Expand Down Expand Up @@ -603,6 +605,13 @@ func resolveTag(jimmUUID string, db *db.Database, tag string) (*ofganames.Tag, e
}

return ofganames.ConvertTagWithRelation(names.NewApplicationOfferTag(offer.UUID), relation), nil
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
Comment on lines +608 to +614
Copy link
Member

Choose a reason for hiding this comment

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

(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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@babakks I believe that is correct:

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

Users work the same way

}
return nil, errors.E("failed to map tag " + matches[1])
}
Expand Down
10 changes: 9 additions & 1 deletion internal/jujuapi/access_control_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/canonical/jimm/internal/jujuapi"
"github.com/canonical/jimm/internal/openfga"
ofganames "github.com/canonical/jimm/internal/openfga/names"
"github.com/canonical/jimm/pkg/names"
)

type accessControlSuite struct {
Expand Down Expand Up @@ -792,7 +793,8 @@ func (s *accessControlSuite) TestJAASTag(c *gc.C) {
ctx := context.Background()
db := s.JIMM.Database

user, _, controller, model, applicationOffer, cloud, _, _, closeClient := createTestControllerEnvironment(ctx, c, s)
user, group, controller, model, applicationOffer, cloud, _, _, closeClient := createTestControllerEnvironment(ctx, c, s)
serviceAccountId := petname.Generate(2, "-") + "@serviceaccount"
closeClient()

tests := []struct {
Expand All @@ -802,6 +804,12 @@ func (s *accessControlSuite) TestJAASTag(c *gc.C) {
}{{
tag: ofganames.ConvertTag(user.ResourceTag()),
expectedJAASTag: "user-" + user.Name,
}, {
tag: ofganames.ConvertTag(names.NewServiceAccountTag(serviceAccountId)),
expectedJAASTag: "serviceaccount-" + serviceAccountId,
}, {
tag: ofganames.ConvertTag(group.ResourceTag()),
expectedJAASTag: "group-" + group.Name,
}, {
tag: ofganames.ConvertTag(controller.ResourceTag()),
expectedJAASTag: "controller-" + controller.Name,
Expand Down
Loading