API refactor: Add integration tests for admin RPCs #1730

MarcosDY · 2020-07-17T17:42:39Z

Add integration tests for admin RPCs

amartinezfayo

Looking good!
We may perform some validations of the responses?

test/integration/setup/adminclient/client.go

amartinezfayo

I have some suggestions, all minor things.

amartinezfayo · 2020-07-23T14:37:23Z

test/integration/setup/adminclient/client.go

+	case err != nil:
+		return err
+	case time.Unix(resp.Svid.ExpiresAt, 0).Before(time.Now()):
+		return errors.New("invalid expiredAt")


Suggested change

return errors.New("invalid expiredAt")

return errors.New("invalid ExpiresAt")

amartinezfayo · 2020-07-23T14:43:35Z

test/integration/setup/adminclient/client.go

+	claimsMap := make(map[string]interface{})
+	err = token.UnsafeClaimsWithoutVerification(&claimsMap)
+	if err != nil {
+		return fmt.Errorf("failed to claims: %v", err)


Suggested change

return fmt.Errorf("failed to claims: %v", err)

return fmt.Errorf("claims verification failed: %v", err)

amartinezfayo · 2020-07-23T14:47:43Z

test/integration/setup/adminclient/client.go

+	case len(r.Bundle.X509Authorities) == 0:
+		return errors.New("missing X509 authorities")
+	case !containsX509Certificate(r.Bundle.X509Authorities, blk.Bytes):
+		return errors.New("no x509 authority")


Suggested change

return errors.New("no x509 authority")

return errors.New("no X509 authority")

amartinezfayo · 2020-07-23T14:47:55Z

test/integration/setup/adminclient/client.go

+	case !containsX509Certificate(r.Bundle.X509Authorities, blk.Bytes):
+		return errors.New("no x509 authority")
+	case !containsJWTKey(r.Bundle.JwtAuthorities, jwtKey):
+		return errors.New("no jwt key")


Suggested change

return errors.New("no jwt key")

return errors.New("no JWT key")

amartinezfayo · 2020-07-23T14:48:28Z

test/integration/setup/adminclient/client.go

+	case len(r.Bundle.JwtAuthorities) == 0:
+		return errors.New("missing JWT authorities")
+	case len(r.Bundle.X509Authorities) != 0:
+		return errors.New("unexpected x509 authorities")


Suggested change

return errors.New("unexpected x509 authorities")

return errors.New("unexpected X509 authorities")

amartinezfayo · 2020-07-23T14:53:42Z

test/integration/setup/adminclient/client.go

+
+	for _, e := range resp.Entries {
+		if !containsFunc(e.SpiffeId) {
+			return fmt.Errorf("unexpected entry: %v", e.Id)


Suggested change

return fmt.Errorf("unexpected entry: %v", e.Id)

return fmt.Errorf("unexpected entry: %v", e)

amartinezfayo · 2020-07-23T15:10:47Z

test/integration/setup/adminclient/client.go

+	case r.Status.Code != int32(codes.OK):
+		return fmt.Errorf("unexpected status: %v", r.Status)
+	case r.Id != entryID:
+		return fmt.Errorf("unexpected entryID: %v", r.Id)


Suggested change

return fmt.Errorf("unexpected entryID: %v", r.Id)

return fmt.Errorf("unexpected entry: %v", r)

amartinezfayo · 2020-07-23T15:11:13Z

test/integration/setup/adminclient/client.go

+		return errors.New("missing token")
+	}
+
+	// Set agentID that will be used in another tests


Suggested change

// Set agentID that will be used in another tests

// Set agentID that will be used in other tests

amartinezfayo · 2020-07-23T15:11:30Z

test/integration/setup/adminclient/client.go

+	// Create CSR
+	csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{}, key)
+	if err != nil {
+		return fmt.Errorf("fails to create CSR: %v", err)


Suggested change

return fmt.Errorf("fails to create CSR: %v", err)

return fmt.Errorf("failed to create CSR: %v", err)

amartinezfayo · 2020-07-23T16:26:33Z

test/integration/suites/admin-endpoints/README.md

+
+## Description
+
+This suite validates access to experimental admin endpoints


Since we are not validating only access, this does't seem to be completely accurate

Suggested change

This suite validates access to experimental admin endpoints

This suite validates the experimental endpoints that require an admin X509-SVID

dfeldman · 2020-07-23T17:58:48Z

test/integration/setup/adminclient/build.sh

+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+
+cd "${DIR}" && GOOS=linux go build -o $1


Can you please add CGO_ENABLED=0 to this? It makes it easier to do development since you can compile outside Docker and run inside Docker (alpine)

marcosy · 2020-07-23T18:53:34Z

test/integration/suites/admin-endpoints/04-create-registration-entries

+    -ttl 0
+check-synced-entry "spire-agent" "spiffe://domain.test/admin"
+
+log-debug "creating admin registration entry..."


The message seems duplicated from the previous log line. Maybe:

Suggested change

log-debug "creating admin registration entry..."

log-debug "creating non admin registration entry..."

or

Suggested change

log-debug "creating admin registration entry..."

log-debug "creating regular registration entry..."

marcosy · 2020-07-23T19:22:42Z

test/integration/suites/admin-endpoints/conf/server/server.conf

+	ca_ttl = "1h"
+	default_svid_ttl = "10m"
+	experimental {
+		enable_api = true


Just for clarifying... why does the server configuration file has the experimental API enabled and the agent configuration file does not have it? Not sure if it was intended

we dont use agent to run this test we use it only to fetch svids on client, and using regular workload api that is compatible with go-spiffe

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

amartinezfayo

Thanks @MarcosDY!

MarcosDY requested review from amartinezfayo, APTy, azdagron, evan2645 and mcpherrinm as code owners July 17, 2020 17:42

MarcosDY changed the title ~~API refactor: Add integration tests for access RPCs~~ API refactor: Add integration tests for admin RPCs Jul 20, 2020

amartinezfayo reviewed Jul 21, 2020

View reviewed changes

MarcosDY force-pushed the api-refactor-admin-its branch 2 times, most recently from 760d68b to 23e93b4 Compare July 23, 2020 15:50

amartinezfayo reviewed Jul 23, 2020

View reviewed changes

dfeldman reviewed Jul 23, 2020

View reviewed changes

marcosy reviewed Jul 23, 2020

View reviewed changes

MarcosDY added 3 commits July 24, 2020 12:03

API refactor: Add integration tests for admin RPCs

b9ce74a

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

use CGO_ENABLED and minor updates

0bddd44

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

solve typo

c0479af

Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>

MarcosDY force-pushed the api-refactor-admin-its branch from 0a0ddd4 to c0479af Compare July 24, 2020 15:32

Merge branch 'master' into api-refactor-admin-its

9c516ba

amartinezfayo approved these changes Jul 24, 2020

View reviewed changes

amartinezfayo merged commit 066ab54 into spiffe:master Jul 24, 2020

amartinezfayo mentioned this pull request Sep 1, 2020

API Refactor: integration tests for common flows #1718

Closed

MarcosDY deleted the api-refactor-admin-its branch August 2, 2022 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API refactor: Add integration tests for admin RPCs #1730

API refactor: Add integration tests for admin RPCs #1730

MarcosDY commented Jul 17, 2020

amartinezfayo left a comment

amartinezfayo left a comment

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

amartinezfayo Jul 23, 2020

dfeldman Jul 23, 2020

marcosy Jul 23, 2020

marcosy Jul 23, 2020

MarcosDY Jul 23, 2020

amartinezfayo left a comment

	return errors.New("invalid expiredAt")
	return errors.New("invalid ExpiresAt")

	return fmt.Errorf("failed to claims: %v", err)
	return fmt.Errorf("claims verification failed: %v", err)

	return errors.New("no x509 authority")
	return errors.New("no X509 authority")

	return errors.New("no jwt key")
	return errors.New("no JWT key")

	return errors.New("unexpected x509 authorities")
	return errors.New("unexpected X509 authorities")

	return fmt.Errorf("unexpected entry: %v", e.Id)
	return fmt.Errorf("unexpected entry: %v", e)

	return fmt.Errorf("unexpected entryID: %v", r.Id)
	return fmt.Errorf("unexpected entry: %v", r)

	// Set agentID that will be used in another tests
	// Set agentID that will be used in other tests

	return fmt.Errorf("fails to create CSR: %v", err)
	return fmt.Errorf("failed to create CSR: %v", err)


		## Description

		This suite validates access to experimental admin endpoints

	This suite validates access to experimental admin endpoints
	This suite validates the experimental endpoints that require an admin X509-SVID


		DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

		cd "${DIR}" && GOOS=linux go build -o $1

	log-debug "creating admin registration entry..."
	log-debug "creating non admin registration entry..."

	log-debug "creating admin registration entry..."
	log-debug "creating regular registration entry..."

API refactor: Add integration tests for admin RPCs #1730

API refactor: Add integration tests for admin RPCs #1730

Conversation

MarcosDY commented Jul 17, 2020

amartinezfayo left a comment

Choose a reason for hiding this comment

amartinezfayo 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

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

amartinezfayo left a comment

Choose a reason for hiding this comment