Implementing Galadriel Server Management Handlers API #150

Victorblsilveira · 2023-05-08T18:55:47Z

Pull request check list

Proper tests/regressions included?
Documentation updated?

Affected functionality

All endpoints of the Server Management API.

Description of change

Implementing some endpoints from de UDS OpenAPI specifications.
Unit Tests included.

Which issue this pull requests fixes

… unit tests

mchurichi · 2023-05-08T19:38:17Z

pkg/server/endpoints/management.go

can we rename this to admin.go?

I'm modifying the same file, can we postpone the renaming for now?

pkg/server/datastore/fakedatabase.go

…ving get relationship uni tests

mgbcaio

Looks pretty good for me, lets see what others think. Thanks for putting this together, Victor! amazing work.

mgbcaio · 2023-05-11T15:49:58Z

pkg/server/endpoints/management_test.go

+		tdUUID1 := NewNullableID()
+		tdUUID2 := NewNullableID()
+		tdUUID3 := NewNullableID()


can we declare this to be reused in the other tests ? something like:

var ( // Relationships ID's r1ID = NewNullableID() r2ID = NewNullableID() r3ID = NewNullableID() // Trust Domains ID's tdUUID1 = NewNullableID() tdUUID2 = NewNullableID() tdUUID3 = NewNullableID() )

pkg/server/endpoints/management.go

maxlambrecht · 2023-05-11T19:51:30Z

pkg/server/endpoints/management.go

 	}

 	return nil
 }

 // GetRelationshipsRelationshipID retrieve a specific relationship based on its id - (GET /relationships/{relationshipID})
-func (h AdminAPIHandlers) GetRelationshipsRelationshipID(ctx echo.Context, relationshipID api.UUID) error {
+func (h AdminAPIHandlers) GetRelationshipsRelationshipID(echoCtx echo.Context, relationshipID api.UUID) error {


Change the spec adding the operation ID=GetRelationshipByID to avoid having this awful name.

That makes sense

maxlambrecht · 2023-05-11T19:52:22Z

pkg/server/endpoints/management.go

 	}

 	return nil
 }

 // GetTrustDomainTrustDomainName retrieve a specific trust domain by its name - (GET /trust-domain/{trustDomainName})
-func (h AdminAPIHandlers) GetTrustDomainTrustDomainName(ctx echo.Context, trustDomainName api.TrustDomainName) error {
+func (h AdminAPIHandlers) GetTrustDomainTrustDomainName(echoCtx echo.Context, trustDomainName api.TrustDomainName) error {


Same here, change the open api spec to generate a better name: GetTrustDomainByName

maxlambrecht · 2023-05-11T19:53:10Z

pkg/server/endpoints/management.go

 	return nil
 }

 // PutTrustDomainTrustDomainName updates the trust domain - (PUT /trust-domain/{trustDomainName})
-func (h AdminAPIHandlers) PutTrustDomainTrustDomainName(ctx echo.Context, trustDomainName api.UUID) error {
+func (h AdminAPIHandlers) PutTrustDomainTrustDomainName(echoCtx echo.Context, trustDomainID api.UUID) error {


pkg/server/endpoints/management.go

maxlambrecht · 2023-05-11T19:58:44Z

pkg/server/endpoints/management.go

+	return td, nil
+}
+
+func (h *AdminAPIHandlers) filterRelationshipsByStatus(


This function can be a lot simpler, like: https://github.com/HewlettPackard/galadriel/pull/154/files/f5baf1a3c0237b0cddf0743f5a1164809e0fd8e7#diff-6b385016307cc76128617e3a0d824b16eaf8d45a64b1a3edeb1565006452bc7cR253

I will need your changes to simplify that.

you can copy my function and then once one of the PRs is merged refactor to remove the duplication.

Here is a new version more readable:

func filterRelationshipsByConsentStatus(trustDomain uuid.UUID, relationships []*entity.Relationship, status api.ConsentStatus) []*entity.Relationship { filtered := make([]*entity.Relationship, 0) for _, relationship := range relationships { trustDomainA := relationship.TrustDomainAID trustDomainB := relationship.TrustDomainBID trustDomainAConsent := api.ConsentStatus(relationship.TrustDomainAConsent) trustDomainBConsent := api.ConsentStatus(relationship.TrustDomainBConsent) isConsentStatusMatch := (trustDomainA == trustDomain && trustDomainAConsent == status) || (trustDomainB == trustDomain && trustDomainBConsent == status) if isConsentStatusMatch { filtered = append(filtered, relationship) } } return filtered }

If I am not wrong, I think you changed the database models to support that. So functions like api.ConsenStatus does not exists.

I think that we can push your PR first. Since is already on review

maxlambrecht · 2023-05-11T20:02:03Z

pkg/server/endpoints/management.go

 	eRelationship := reqBody.ToEntity()
-	rel, err := h.Datastore.CreateOrUpdateRelationship(gctx, eRelationship)
+
+	if err := h.checkTrustDomains(ctx, http.StatusBadRequest, eRelationship.TrustDomainAID, eRelationship.TrustDomainBID); err != nil {


I think this can be removed and it would be better handled at the datastore layer, the CreateOrUpdateRelationship will fail if one of the trust domain doesn't exist because a DB constraint will fail.

Yeah, agreed that the database can handle that, the point of checking before is to properly identify the problem. I mean, it was a bad request from the user rather than an internal server error. We could do that by checking the error message instead but this will also make us couple with the specific database. Let me know what you think. Can we handle that in a different way?

You can do it this way:

tdA, err := h.lookupTrustDomain(ctx, eRelationship.TrustDomainAID) if err != nil { return err } tdB, err := h.lookupTrustDomain(ctx, eRelationship.TrustDomainBID) if err != nil { return err } ... func (h *HarvesterAPIHandlers) lookupTrustDomain(ctx context.Context, trustDomainID uuid.UUID) (*entity.TrustDomain, error) { td, err := h.Datastore.FindBundleByTrustDomainID(ctx, trustDomainID) if err != nil { msg := "error looking up trust domain" errMsg := fmt.Errorf("%s: %w", errorMsg, err) return nil, h.handleErrorAndLog(errMsg, msg, http.StatusInternalServerError) } if td == nil { errMsg := fmt.Sprintf("trust domain not found: %q", trustDomainID) return nil, h.handleErrorAndLog(errors.New(errMsg), errMsg, http.StatusBadRequest) } return td, nil }

Updated my comment here ☝️

Co-authored-by: Max Lambrecht <[email protected]> Signed-off-by: Victor Vieira Barros Leal da Silveira <[email protected]>

maxlambrecht · 2023-05-11T20:40:19Z

pkg/server/endpoints/const_test.go

@@ -1,5 +1,7 @@
 package endpoints

 const (
-	testTrustDomain = "test.com"


I think we don't need to create a separate file for constants used in tests.

maxlambrecht · 2023-05-11T20:41:41Z

pkg/server/endpoints/auth_test.go

@@ -46,8 +46,8 @@ func SetupMiddleware() *AuthNTestSetup {
 	}
 }

-func SetupToken(t *testing.T, ds datastore.Datastore, token string, tdID uuid.UUID) *entity.JoinToken {
-	td, err := spiffeid.TrustDomainFromString(testTrustDomain)
+func SetupToken(t *testing.T, ds datastore.Datastore, tdID uuid.UUID, token, tdName string) *entity.JoinToken {


This function doesn't exist anymore.

maxlambrecht · 2023-05-11T20:42:20Z

It would be a good idea to rebase your branch before continuing making changes.

maxlambrecht · 2023-05-15T14:20:56Z

pkg/common/http/http.go

-// BodylessResponse wraps error echo body-less responses.
-func BodylessResponse(ctx echo.Context) error {
+// BodilessResponse wraps error echo body-less responses.
+func BodilessResponse(ctx echo.Context, code int) error {


why add this parameter that is not used?

Name suggestions for this function: NoContentResponse or EmptyResponse

maxlambrecht · 2023-05-15T14:28:02Z

pkg/common/http/http_test.go

@@ -34,21 +34,22 @@ func Setup() *HTTPSetup {
 func TestWriteResponse(t *testing.T) {
 	t.Run("Error when nil body is passed", func(t *testing.T) {
 		setup := Setup()
-		err := WriteResponse(setup.EchoContext, nil)
+		err := WriteResponse(setup.EchoContext, 0, nil)


is 0 a valid http status code?

maxlambrecht · 2023-05-15T14:32:58Z

pkg/server/endpoints/harvester_test.go

@@ -25,6 +26,23 @@ import (
 const (
 	jwtPath     = "/jwt"
 	onboardPath = "/onboard"
+


These changes shouldn't be part of this PR, it should be focused on the management and management_test. I'm working on the Harvester API and making changes to this file, so it will be very hard to merge.

I just add const, but I will move to another file.

sonarqubecloud · 2023-05-16T16:51:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

maxlambrecht

Thanks @Victorblsilveira!

Implementing Galadriel Server Management Handlers API (#150) Signed-off-by: Max Lambrecht <[email protected]>

Victorblsilveira added 2 commits May 5, 2023 11:40

feat: Implementing Admin Handlers

bfdfcc4

feat: Adding unit testes helpers and implemenenting get relationships…

cbdaf26

… unit tests

Victorblsilveira self-assigned this May 8, 2023

Merge branch 'main' into victor/admin-handlers

c55c71b

mchurichi reviewed May 8, 2023

View reviewed changes

mchurichi reviewed May 9, 2023

View reviewed changes

pkg/server/datastore/fakedatabase.go Outdated Show resolved Hide resolved

pkg/server/datastore/fakedatabase.go Outdated Show resolved Hide resolved

Victorblsilveira added 8 commits May 9, 2023 15:59

feat: Creating unit tests for relationshio request creation and impro…

664cb58

…ving get relationship uni tests

Merge remote-tracking branch 'origin/main' into victor/admin-handlers

d02e836

fix: Unmashaling assertion

a55e8fa

fix: Removin code duplication

a7edd5d

fix: Organization

c621801

feat: Adding more unit tests

4b49fcc

feat: Changin return code for relationship request creation

5f3dcbd

feat: Finishing unit tests

9bc7b3a

Victorblsilveira marked this pull request as ready for review May 11, 2023 14:46

Victorblsilveira requested review from wibarre, jufantozzi and mgbcaio as code owners May 11, 2023 14:46

mgbcaio reviewed May 11, 2023

View reviewed changes

maxlambrecht reviewed May 11, 2023

View reviewed changes

Victorblsilveira and others added 2 commits May 11, 2023 17:08

Update pkg/server/endpoints/management.go

46ca412

Co-authored-by: Max Lambrecht <[email protected]> Signed-off-by: Victor Vieira Barros Leal da Silveira <[email protected]>

Update pkg/server/endpoints/management.go

02de6e6

Co-authored-by: Max Lambrecht <[email protected]> Signed-off-by: Victor Vieira Barros Leal da Silveira <[email protected]>

maxlambrecht reviewed May 11, 2023

View reviewed changes

Victorblsilveira added 4 commits May 15, 2023 09:27

Merge remote-tracking branch 'origin/main' into victor/admin-handlers

990cd61

fix: Function names

85ce235

fix: Centralizing common stuff

63306ec

fix: Removing const_test file and unit test changes

2b5d958

maxlambrecht reviewed May 15, 2023

View reviewed changes

Victorblsilveira added 8 commits May 15, 2023 11:36

fix: Misstypes and const places

38449f0

fix: Refactoring trust domain lookup functions

21b679b

Merge branch 'HewlettPackard:main' into victor/admin-handlers

d9b7d39

fix: Unit tests failing

7eafbe5

Merge remote-tracking branch 'origin/main' into victor/admin-handlers

f085b5d

fix: Removing unused values

b60cec3

fix: Unit test failing

4da954f

fix: Unit test checking

b92ffcb

Victorblsilveira requested review from maxlambrecht and mgbcaio May 16, 2023 16:53

maxlambrecht approved these changes May 16, 2023

View reviewed changes

maxlambrecht merged commit 5ad8753 into HewlettPackard:main May 16, 2023

Victorblsilveira deleted the victor/admin-handlers branch May 16, 2023 17:56

maxlambrecht pushed a commit that referenced this pull request May 16, 2023

Add SQLite3 support (#157)

909bb28

Implementing Galadriel Server Management Handlers API (#150) Signed-off-by: Max Lambrecht <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing Galadriel Server Management Handlers API #150

Implementing Galadriel Server Management Handlers API #150

Victorblsilveira commented May 8, 2023

mchurichi May 8, 2023

maxlambrecht May 8, 2023

mgbcaio left a comment

mgbcaio May 11, 2023

maxlambrecht May 11, 2023

Victorblsilveira May 11, 2023

maxlambrecht May 11, 2023

maxlambrecht May 11, 2023

maxlambrecht May 11, 2023

Victorblsilveira May 15, 2023

maxlambrecht May 15, 2023 •

edited

Loading

Victorblsilveira May 15, 2023

Victorblsilveira May 15, 2023

maxlambrecht May 11, 2023

Victorblsilveira May 15, 2023

maxlambrecht May 15, 2023 •

edited

Loading

maxlambrecht May 15, 2023

maxlambrecht May 11, 2023

maxlambrecht May 11, 2023

maxlambrecht commented May 11, 2023

maxlambrecht May 15, 2023 •

edited

Loading

Victorblsilveira May 15, 2023

maxlambrecht May 15, 2023

maxlambrecht May 15, 2023

Victorblsilveira May 15, 2023 •

edited

Loading

sonarqubecloud bot commented May 16, 2023

maxlambrecht left a comment

Implementing Galadriel Server Management Handlers API #150

Implementing Galadriel Server Management Handlers API #150

Conversation

Victorblsilveira commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgbcaio left a 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

maxlambrecht May 15, 2023 • 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

maxlambrecht May 15, 2023 • 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

maxlambrecht commented May 11, 2023

maxlambrecht May 15, 2023 • 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

Victorblsilveira May 15, 2023 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented May 16, 2023

maxlambrecht left a comment

Choose a reason for hiding this comment

maxlambrecht May 15, 2023 •

edited

Loading

maxlambrecht May 15, 2023 •

edited

Loading

maxlambrecht May 15, 2023 •

edited

Loading

Victorblsilveira May 15, 2023 •

edited

Loading