Merge pull request #598 from azdagron/fetch-bundle-success-if-not-found

don't fail FetchBundle if bundle does not exist
spiffe · Oct 17, 2018 · a127a5b · a127a5b
2 parents 0878c9f + e0be6a3
commit a127a5b
Show file tree

Hide file tree

Showing 9 changed files with 27 additions and 14 deletions.
diff --git a/cmd/spire-server/cli/bundle/bundle_test.go b/cmd/spire-server/cli/bundle/bundle_test.go
@@ -303,10 +303,12 @@ func (s *BundleSuite) TestDelete() {
 	s.Require().Equal(0, s.deleteCmd.Run([]string{"-id", "spiffe://domain1.test"}))
 	s.Require().Equal("bundle deleted.\n", s.stdout.String())
 
-	_, err := s.ds.FetchBundle(context.Background(), &datastore.FetchBundleRequest{
+	resp, err := s.ds.FetchBundle(context.Background(), &datastore.FetchBundleRequest{
 		TrustDomain: "spiffe://domain1.test",
 	})
-	s.Require().EqualError(err, "no such bundle")
+	s.Require().NoError(err)
+	s.Require().NotNil(resp)
+	s.Require().Nil(resp.Bundle)
 }
 
 func (s *BundleSuite) TestDeleteWithRestrictMode() {

diff --git a/pkg/server/ca/manager.go b/pkg/server/ca/manager.go
@@ -215,7 +215,7 @@ func (m *manager) pruneBundle(ctx context.Context) error {
 		return fmt.Errorf("fetch bundle: %v", err)
 	}
 	if resp.Bundle == nil {
-		return errors.New("no bundle in response")
+		return errors.New("bundle not found")
 	}
 	oldBundle := resp.Bundle
 

diff --git a/pkg/server/endpoints/endpoints.go b/pkg/server/endpoints/endpoints.go
@@ -237,7 +237,7 @@ func (e *endpoints) getCerts(ctx context.Context) ([]tls.Certificate, *x509.Cert
 		return nil, nil, fmt.Errorf("get bundle from datastore: %v", err)
 	}
 	if resp.Bundle == nil {
-		return nil, nil, errors.New("response missing bundle")
+		return nil, nil, errors.New("bundle not found")
 	}
 
 	caCerts, err := x509.ParseCertificates(resp.Bundle.CaCerts)

diff --git a/pkg/server/endpoints/node/handler.go b/pkg/server/endpoints/node/handler.go
@@ -685,7 +685,7 @@ func (h *Handler) getBundle(ctx context.Context, trustDomain string) (*node.Bund
 		return nil, fmt.Errorf("get bundle from datastore: %v", err)
 	}
 	if resp.Bundle == nil {
-		return nil, errors.New("response missing bundle")
+		return nil, errors.New("bundle not found")
 	}
 
 	return &node.Bundle{

diff --git a/pkg/server/endpoints/registration/handler.go b/pkg/server/endpoints/registration/handler.go
@@ -246,7 +246,7 @@ func (h *Handler) FetchFederatedBundle(
 		return nil, err
 	}
 	if resp.Bundle == nil {
-		return nil, errors.New("no bundle in response")
+		return nil, errors.New("bundle not found")
 	}
 
 	return &registration.FederatedBundle{
@@ -374,7 +374,7 @@ func (h *Handler) FetchBundle(
 		return nil, fmt.Errorf("get bundle from datastore: %v", err)
 	}
 	if resp.Bundle == nil {
-		return nil, errors.New("response has no bundle")
+		return nil, errors.New("bundle not found")
 	}
 
 	return &registration.Bundle{CaCerts: resp.Bundle.CaCerts}, nil

diff --git a/pkg/server/endpoints/registration/handler_test.go b/pkg/server/endpoints/registration/handler_test.go
@@ -125,7 +125,7 @@ func (s *HandlerSuite) TestFetchFederatedBundle() {
 		{Id: "spiffe://example.org", CaCerts: "", Err: "federated bundle id cannot match server trust domain"},
 		{Id: "spiffe://otherdomain.org/spire/agent", CaCerts: "", Err: `"spiffe://otherdomain.org/spire/agent" is not a valid trust domain SPIFFE ID: path is not empty`},
 		{Id: "spiffe://otherdomain.org", CaCerts: "OTHERDOMAIN", Err: ""},
-		{Id: "spiffe://yetotherdomain.org", CaCerts: "", Err: "no such bundle"},
+		{Id: "spiffe://yetotherdomain.org", CaCerts: "", Err: "bundle not found"},
 	}
 
 	for _, testCase := range testCases {
@@ -248,8 +248,9 @@ func (s *HandlerSuite) TestDeleteFederatedBundle() {
 		resp, err := s.ds.FetchBundle(context.Background(), &datastore.FetchBundleRequest{
 			TrustDomain: testCase.Id,
 		})
-		s.Require().EqualError(err, "no such bundle")
-		s.Require().Nil(resp)
+		s.Require().NoError(err)
+		s.Require().NotNil(resp)
+		s.Require().Nil(resp.Bundle)
 	}
 }
 

diff --git a/pkg/server/plugin/datastore/sql/sql.go b/pkg/server/plugin/datastore/sql/sql.go
@@ -594,7 +594,11 @@ func deleteBundle(tx *gorm.DB, req *datastore.DeleteBundleRequest) (*datastore.D
 // FetchBundle returns the bundle matching the specified Trust Domain.
 func fetchBundle(tx *gorm.DB, req *datastore.FetchBundleRequest) (*datastore.FetchBundleResponse, error) {
 	model := new(Bundle)
-	if err := tx.Find(model, "trust_domain = ?", req.TrustDomain).Error; err != nil {
+	err := tx.Find(model, "trust_domain = ?", req.TrustDomain).Error
+	switch {
+	case err == gorm.ErrRecordNotFound:
+		return &datastore.FetchBundleResponse{}, nil
+	case err != nil:
 		return nil, sqlError.Wrap(err)
 	}
 

diff --git a/pkg/server/plugin/datastore/sql/sql_test.go b/pkg/server/plugin/datastore/sql/sql_test.go
@@ -107,14 +107,20 @@ func (s *PluginSuite) TestBundleCRUD() {
 		CaCerts:     s.cert.Raw,
 	}
 
+	// fetch non-existant
+	fresp, err := s.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{TrustDomain: "spiffe://foo"})
+	s.Require().NoError(err)
+	s.Require().NotNil(fresp)
+	s.Require().Nil(fresp.Bundle)
+
 	// create
-	_, err := s.ds.CreateBundle(ctx, &datastore.CreateBundleRequest{
+	_, err = s.ds.CreateBundle(ctx, &datastore.CreateBundleRequest{
 		Bundle: bundle,
 	})
 	s.Require().NoError(err)
 
 	// fetch
-	fresp, err := s.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{TrustDomain: "spiffe://foo"})
+	fresp, err = s.ds.FetchBundle(ctx, &datastore.FetchBundleRequest{TrustDomain: "spiffe://foo"})
 	s.Require().NoError(err)
 	s.Equal(bundle, fresp.Bundle)
 

diff --git a/test/fakes/fakedatastore/fakedatastore.go b/test/fakes/fakedatastore/fakedatastore.go
@@ -181,7 +181,7 @@ func (s *DataStore) FetchBundle(ctx context.Context, req *datastore.FetchBundleR
 
 	bundle, ok := s.bundles[req.TrustDomain]
 	if !ok {
-		return nil, ErrNoSuchBundle
+		return &datastore.FetchBundleResponse{}, nil
 	}
 
 	return &datastore.FetchBundleResponse{