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

don't fail FetchBundle if bundle does not exist #598

Merged
merged 2 commits into from
Oct 17, 2018
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
6 changes: 4 additions & 2 deletions cmd/spire-server/cli/bundle/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/ca/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion pkg/server/endpoints/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/endpoints/node/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions pkg/server/endpoints/registration/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions pkg/server/endpoints/registration/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/server/plugin/datastore/sql/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
10 changes: 8 additions & 2 deletions pkg/server/plugin/datastore/sql/sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion test/fakes/fakedatastore/fakedatastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down