From 36a5d48e004f8fc6cec5f447ac2e4232ee3a124b Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 7 Aug 2024 10:04:23 +0200 Subject: [PATCH] feat: address PR changes - Don't leak api types outside of the jaas client. - Cleanup tests --- internal/juju/common_test.go | 4 +--- internal/juju/jaas.go | 42 ++++++++++++++++++++++++++++------- internal/juju/jaas_test.go | 38 +++++++++++++++---------------- internal/juju/secrets_test.go | 32 +++++++++++++------------- 4 files changed, 68 insertions(+), 48 deletions(-) diff --git a/internal/juju/common_test.go b/internal/juju/common_test.go index c392ddb8..78a4e766 100644 --- a/internal/juju/common_test.go +++ b/internal/juju/common_test.go @@ -19,9 +19,7 @@ type JujuSuite struct { mockSharedClient *MockSharedClient } -func (s *JujuSuite) setupMocks(t *testing.T, modelName *string) *gomock.Controller { - s.testModelName = modelName - +func (s *JujuSuite) setupMocks(t *testing.T) *gomock.Controller { ctlr := gomock.NewController(t) s.mockConnection = NewMockConnection(ctlr) diff --git a/internal/juju/jaas.go b/internal/juju/jaas.go index 457ab759..0997bd8a 100644 --- a/internal/juju/jaas.go +++ b/internal/juju/jaas.go @@ -6,9 +6,8 @@ package juju import ( "github.com/canonical/jimm-go-sdk/v3/api" "github.com/canonical/jimm-go-sdk/v3/api/params" - jujuapi "github.com/juju/juju/api" - "github.com/juju/errors" + jujuapi "github.com/juju/juju/api" ) type jaasClient struct { @@ -25,8 +24,35 @@ func newJaasClient(sc SharedClient) *jaasClient { } } +// JaasTuple represents a tuple object of used by JAAS for permissions management. +type JaasTuple struct { + // Object represents the source side of the relation. + Object string + // Relation represents the level of access + Relation string + // Target represents the resource that you want `object` to have access to. + Target string +} + +func toAPITuples(tuples []JaasTuple) []params.RelationshipTuple { + out := make([]params.RelationshipTuple, 0, len(tuples)) + for _, tuple := range tuples { + out = append(out, toAPITuple(tuple)) + } + return out +} + +func toAPITuple(tuple JaasTuple) params.RelationshipTuple { + return params.RelationshipTuple{ + Object: tuple.Object, + Relation: tuple.Relation, + TargetObject: tuple.Target, + } +} + // AddRelations attempts to create the provided slice of relationship tuples. -func (jc *jaasClient) AddRelations(tuples []params.RelationshipTuple) error { +// The caller is expected to populate the slice so that `len(tuples) > 0`. +func (jc *jaasClient) AddRelations(tuples []JaasTuple) error { conn, err := jc.GetConnection(nil) if err != nil { return err @@ -34,13 +60,13 @@ func (jc *jaasClient) AddRelations(tuples []params.RelationshipTuple) error { defer func() { _ = conn.Close() }() cl := jc.getJaasApiClient(conn) req := params.AddRelationRequest{ - Tuples: tuples, + Tuples: toAPITuples(tuples), } return cl.AddRelation(&req) } // DeleteRelations attempts to delete the provided slice of relationship tuples. -func (jc *jaasClient) DeleteRelations(tuples []params.RelationshipTuple) error { +func (jc *jaasClient) DeleteRelations(tuples []JaasTuple) error { conn, err := jc.GetConnection(nil) if err != nil { return err @@ -48,13 +74,13 @@ func (jc *jaasClient) DeleteRelations(tuples []params.RelationshipTuple) error { defer func() { _ = conn.Close() }() cl := jc.getJaasApiClient(conn) req := params.RemoveRelationRequest{ - Tuples: tuples, + Tuples: toAPITuples(tuples), } return cl.RemoveRelation(&req) } // ReadRelations attempts to read relations that match the criteria defined by `tuple`. -func (jc *jaasClient) ReadRelations(tuple *params.RelationshipTuple) ([]params.RelationshipTuple, error) { +func (jc *jaasClient) ReadRelations(tuple *JaasTuple) ([]params.RelationshipTuple, error) { if tuple == nil { return nil, errors.New("add relation request nil") } @@ -67,7 +93,7 @@ func (jc *jaasClient) ReadRelations(tuple *params.RelationshipTuple) ([]params.R client := jc.getJaasApiClient(conn) relations := make([]params.RelationshipTuple, 0) - req := ¶ms.ListRelationshipTuplesRequest{Tuple: *tuple} + req := ¶ms.ListRelationshipTuplesRequest{Tuple: toAPITuple(*tuple)} for { resp, err := client.ListRelationshipTuples(req) if err != nil { diff --git a/internal/juju/jaas_test.go b/internal/juju/jaas_test.go index 9a17915a..c9da2572 100644 --- a/internal/juju/jaas_test.go +++ b/internal/juju/jaas_test.go @@ -19,10 +19,8 @@ type JaasSuite struct { mockJaasClient *MockJaasAPIClient } -func (s *JaasSuite) SetupTest() {} - func (s *JaasSuite) setupMocks(t *testing.T) *gomock.Controller { - ctlr := s.JujuSuite.setupMocks(t, nil) + ctlr := s.JujuSuite.setupMocks(t) s.mockJaasClient = NewMockJaasAPIClient(ctlr) return ctlr @@ -41,17 +39,17 @@ func (s *JaasSuite) TestAddRelations() { ctlr := s.setupMocks(s.T()) defer ctlr.Finish() - tuples := []params.RelationshipTuple{ - {Object: "object-1", Relation: "relation", TargetObject: "target-1"}, - {Object: "object-2", Relation: "relation", TargetObject: "target-2"}, + tuples := []JaasTuple{ + {Object: "object-1", Relation: "relation", Target: "target-1"}, + {Object: "object-2", Relation: "relation", Target: "target-2"}, } req := params.AddRelationRequest{ - Tuples: tuples, + Tuples: toAPITuples(tuples), } s.mockJaasClient.EXPECT().AddRelation( &req, - ).Return(nil).Times(1) + ).Return(nil) client := s.getJaasClient() err := client.AddRelations(tuples) @@ -62,17 +60,17 @@ func (s *JaasSuite) TestDeleteRelations() { ctlr := s.setupMocks(s.T()) defer ctlr.Finish() - tuples := []params.RelationshipTuple{ - {Object: "object-1", Relation: "relation", TargetObject: "target-1"}, - {Object: "object-2", Relation: "relation", TargetObject: "target-2"}, + tuples := []JaasTuple{ + {Object: "object-1", Relation: "relation", Target: "target-1"}, + {Object: "object-2", Relation: "relation", Target: "target-2"}, } req := params.RemoveRelationRequest{ - Tuples: tuples, + Tuples: toAPITuples(tuples), } s.mockJaasClient.EXPECT().RemoveRelation( &req, - ).Return(nil).Times(1) + ).Return(nil) client := s.getJaasClient() err := client.DeleteRelations(tuples) @@ -83,25 +81,25 @@ func (s *JaasSuite) TestReadRelations() { ctlr := s.setupMocks(s.T()) defer ctlr.Finish() - tuple := params.RelationshipTuple{Object: "object-1", Relation: "relation", TargetObject: "target-1"} + tuple := JaasTuple{Object: "object-1", Relation: "relation", Target: "target-1"} // 1st request/response has no token in the request and a token in the response indicating another page is available. - req := ¶ms.ListRelationshipTuplesRequest{Tuple: tuple} + req := ¶ms.ListRelationshipTuplesRequest{Tuple: toAPITuple(tuple)} respWithToken := ¶ms.ListRelationshipTuplesResponse{ - Tuples: []params.RelationshipTuple{tuple}, + Tuples: []params.RelationshipTuple{toAPITuple(tuple)}, ContinuationToken: "token", } s.mockJaasClient.EXPECT().ListRelationshipTuples( req, - ).Return(respWithToken, nil).Times(1) + ).Return(respWithToken, nil) // 2nd request/response has the previous token in the request and no token in the response, indicating all pages have been consumed. - reqWithToken := ¶ms.ListRelationshipTuplesRequest{Tuple: tuple, ContinuationToken: "token"} + reqWithToken := ¶ms.ListRelationshipTuplesRequest{Tuple: toAPITuple(tuple), ContinuationToken: "token"} respWithoutToken := ¶ms.ListRelationshipTuplesResponse{ - Tuples: []params.RelationshipTuple{tuple}, + Tuples: []params.RelationshipTuple{toAPITuple(tuple)}, ContinuationToken: "", } s.mockJaasClient.EXPECT().ListRelationshipTuples( reqWithToken, - ).Return(respWithoutToken, nil).Times(1) + ).Return(respWithoutToken, nil) client := s.getJaasClient() relations, err := client.ReadRelations(&tuple) diff --git a/internal/juju/secrets_test.go b/internal/juju/secrets_test.go index e74be5b4..2a8a3782 100644 --- a/internal/juju/secrets_test.go +++ b/internal/juju/secrets_test.go @@ -19,17 +19,15 @@ type SecretSuite struct { suite.Suite JujuSuite - testModelName string - mockSecretClient *MockSecretAPIClient } -func (s *SecretSuite) SetupTest() {} +func (s *SecretSuite) SetupSuite() { + s.testModelName = strPtr("test-secret-model") +} func (s *SecretSuite) setupMocks(t *testing.T) *gomock.Controller { - s.testModelName = "test-secret-model" - - ctlr := s.JujuSuite.setupMocks(t, &s.testModelName) + ctlr := s.JujuSuite.setupMocks(t) s.mockSecretClient = NewMockSecretAPIClient(ctlr) return ctlr @@ -60,7 +58,7 @@ func (s *SecretSuite) TestCreateSecret() { client := s.getSecretsClient() output, err := client.CreateSecret(&CreateSecretInput{ - ModelName: s.testModelName, + ModelName: *s.testModelName, Name: "test-secret", Value: decodedValue, Info: "test info", @@ -86,7 +84,7 @@ func (s *SecretSuite) TestCreateSecretError() { client := s.getSecretsClient() output, err := client.CreateSecret(&CreateSecretInput{ - ModelName: s.testModelName, + ModelName: *s.testModelName, Name: "test-secret", Value: decodedValue, Info: "test info", @@ -133,7 +131,7 @@ func (s *SecretSuite) TestReadSecret() { client := s.getSecretsClient() output, err := client.ReadSecret(&ReadSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, Name: &secretName, Revision: &secretRevision, }) @@ -165,7 +163,7 @@ func (s *SecretSuite) TestReadSecretError() { client := s.getSecretsClient() output, err := client.ReadSecret(&ReadSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, }) s.Require().Error(err) @@ -195,7 +193,7 @@ func (s *SecretSuite) TestUpdateSecretWithRenaming() { client := s.getSecretsClient() err = client.UpdateSecret(&UpdateSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, Name: &newSecretName, Value: &decodedValue, AutoPrune: &autoPrune, @@ -224,7 +222,7 @@ func (s *SecretSuite) TestUpdateSecretWithRenaming() { // read secret and check if value is updated output, err := client.ReadSecret(&ReadSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, }) s.Require().NoError(err) @@ -252,7 +250,7 @@ func (s *SecretSuite) TestUpdateSecret() { client := s.getSecretsClient() err = client.UpdateSecret(&UpdateSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, Value: &decodedValue, AutoPrune: &autoPrune, Info: &secretInfo, @@ -281,7 +279,7 @@ func (s *SecretSuite) TestUpdateSecret() { // read secret and check if secret info is updated output, err := client.ReadSecret(&ReadSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, }) s.Require().NoError(err) @@ -302,7 +300,7 @@ func (s *SecretSuite) TestDeleteSecret() { client := s.getSecretsClient() err = client.DeleteSecret(&DeleteSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, }) s.Assert().NoError(err) } @@ -323,14 +321,14 @@ func (s *SecretSuite) TestUpdateAccessSecret() { client := s.getSecretsClient() err = client.UpdateAccessSecret(&GrantRevokeAccessSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, Applications: applications, }, GrantAccess) s.Require().NoError(err) err = client.UpdateAccessSecret(&GrantRevokeAccessSecretInput{ SecretId: secretId, - ModelName: s.testModelName, + ModelName: *s.testModelName, Applications: applications, }, RevokeAccess) s.Require().NoError(err)