From d57428aefe0b117d4aa4c26153fea54a59f773e6 Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Tue, 22 Aug 2023 12:33:06 -0600 Subject: [PATCH] Remove startup entry scan (#4449) Quite some time ago we added a scan to first warn and then eventually delete entries with invalid SPIFFE IDs. This scan is no longer needed, since entries will have already been removed by previous upgrades and can be removed. Signed-off-by: Andrew Harding Co-authored-by: Marcos Yacob --- pkg/server/scanentries.go | 72 --------------- pkg/server/scanentries_test.go | 154 --------------------------------- pkg/server/server.go | 1 - 3 files changed, 227 deletions(-) delete mode 100644 pkg/server/scanentries.go delete mode 100644 pkg/server/scanentries_test.go diff --git a/pkg/server/scanentries.go b/pkg/server/scanentries.go deleted file mode 100644 index bf07f8a2c1..0000000000 --- a/pkg/server/scanentries.go +++ /dev/null @@ -1,72 +0,0 @@ -package server - -import ( - "context" - - "github.com/sirupsen/logrus" - "github.com/spiffe/go-spiffe/v2/spiffeid" - "github.com/spiffe/spire/pkg/common/telemetry" - serverTelemetry "github.com/spiffe/spire/pkg/common/telemetry/server" - "github.com/spiffe/spire/pkg/server/datastore" -) - -var ( - // entryScanPageSize defaults to 200 but can be mutated by unit tests to - // easier test page handling - entryScanPageSize int32 = 200 -) - -// scanForBadEntries scans the entry list for entries that we will ignore -// because they contain problematic spiffe IDs. It will never return an -// error since it is advisory only. -func scanForBadEntries(log logrus.FieldLogger, metrics telemetry.Metrics, ds datastore.DataStore) func(ctx context.Context) error { - return func(ctx context.Context) error { - pagination := &datastore.Pagination{PageSize: entryScanPageSize} - deleted := 0 - for { - resp, err := ds.ListRegistrationEntries(ctx, &datastore.ListRegistrationEntriesRequest{ - Pagination: pagination, - }) - if err != nil { - log.WithError(err).Warn("Failed to scan for bad entries") - return nil - } - - for _, entry := range resp.Entries { - if _, err := spiffeid.FromString(entry.ParentId); err != nil { - log.WithFields(logrus.Fields{ - telemetry.ParentID: entry.ParentId, - telemetry.SPIFFEID: entry.SpiffeId, - telemetry.RegistrationID: entry.EntryId, - logrus.ErrorKey: err, - }).Error("Deleting entry with invalid parentID") - deleted++ - if _, err := ds.DeleteRegistrationEntry(ctx, entry.EntryId); err != nil { - log.WithError(err).Error("Failed to delete entry with an invalid parentID") - } - continue - } - if _, err := spiffeid.FromString(entry.SpiffeId); err != nil { - log.WithFields(logrus.Fields{ - telemetry.ParentID: entry.ParentId, - telemetry.SPIFFEID: entry.SpiffeId, - telemetry.RegistrationID: entry.EntryId, - logrus.ErrorKey: err, - }).Error("Deleting entry with invalid spiffeID") - deleted++ - if _, err := ds.DeleteRegistrationEntry(ctx, entry.EntryId); err != nil { - log.WithError(err).Error("Failed to delete entry with an invalid spiffeID") - } - continue - } - } - - switch { - case resp.Pagination == nil, resp.Pagination.Token == "": - serverTelemetry.SetEntryDeletedGauge(metrics, deleted) - return nil - } - pagination.Token = resp.Pagination.Token - } - } -} diff --git a/pkg/server/scanentries_test.go b/pkg/server/scanentries_test.go deleted file mode 100644 index 7beab53a30..0000000000 --- a/pkg/server/scanentries_test.go +++ /dev/null @@ -1,154 +0,0 @@ -package server - -import ( - "context" - "sort" - "testing" - - "github.com/sirupsen/logrus" - "github.com/sirupsen/logrus/hooks/test" - "github.com/spiffe/spire/pkg/server/datastore" - "github.com/spiffe/spire/proto/spire/common" - "github.com/spiffe/spire/test/fakes/fakedatastore" - "github.com/spiffe/spire/test/fakes/fakemetrics" - "github.com/spiffe/spire/test/spiretest" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func init() { - // Use a page size of two for unit tests - entryScanPageSize = 2 -} - -func TestScanForBadEntries(t *testing.T) { - good1 := &common.RegistrationEntry{ - ParentId: "spiffe://example.org/node", - SpiffeId: "spiffe://example.org/workload", - Selectors: []*common.Selector{{Type: "one", Value: "1"}}, - } - good2 := &common.RegistrationEntry{ - ParentId: "spiffe://example.org/node", - SpiffeId: "spiffe://example.org/workload", - Selectors: []*common.Selector{{Type: "two", Value: "2"}}, - } - good3 := &common.RegistrationEntry{ - ParentId: "spiffe://example.org/node", - SpiffeId: "spiffe://example.org/workload", - Selectors: []*common.Selector{{Type: "three", Value: "3"}}, - } - badParentID := &common.RegistrationEntry{ - ParentId: "spiffe://example.org//node", - SpiffeId: "spiffe://example.org/workload", - Selectors: []*common.Selector{{Type: "bad", Value: "parent-id"}}, - } - badSpiffeID := &common.RegistrationEntry{ - ParentId: "spiffe://example.org/node", - SpiffeId: "spiffe://example.org//workload", - Selectors: []*common.Selector{{Type: "bad", Value: "spiffe-id"}}, - } - - for _, tt := range []struct { - name string - entries []*common.RegistrationEntry - expectLogs []spiretest.LogEntry - expectEntries []*common.RegistrationEntry - expectDeleted int - }{ - { - name: "no entries", - expectDeleted: 0, - }, - { - name: "no bad entries", - entries: []*common.RegistrationEntry{good1, good2, good3}, - expectEntries: []*common.RegistrationEntry{good1, good2, good3}, - expectDeleted: 0, - }, - { - name: "bad spiffe id", - entries: []*common.RegistrationEntry{good1, good2, badSpiffeID, good3}, - expectLogs: []spiretest.LogEntry{ - { - Level: logrus.ErrorLevel, - Message: "Deleting entry with invalid spiffeID", - Data: logrus.Fields{ - "entry_id": "SOME-ID", - "error": "path cannot contain empty segments", - "parent_id": "spiffe://example.org/node", - "spiffe_id": "spiffe://example.org//workload", - }, - }, - }, - expectEntries: []*common.RegistrationEntry{good1, good2, good3}, - expectDeleted: 1, - }, - { - name: "bad parent id", - entries: []*common.RegistrationEntry{good1, good2, badParentID, good3}, - expectLogs: []spiretest.LogEntry{ - { - Level: logrus.ErrorLevel, - Message: "Deleting entry with invalid parentID", - Data: logrus.Fields{ - "entry_id": "SOME-ID", - "error": "path cannot contain empty segments", - "parent_id": "spiffe://example.org//node", - "spiffe_id": "spiffe://example.org/workload", - }, - }, - }, - expectEntries: []*common.RegistrationEntry{good1, good2, good3}, - expectDeleted: 1, - }, - } { - tt := tt - t.Run(tt.name, func(t *testing.T) { - log, logHook := test.NewNullLogger() - metrics := fakemetrics.New() - ds := fakedatastore.New(t) - - for i, entry := range tt.entries { - entry, err := ds.CreateRegistrationEntry(context.Background(), entry) - require.NoError(t, err) - tt.entries[i].EntryId = entry.EntryId - if i < len(tt.expectEntries) { - tt.expectEntries[i].CreatedAt = entry.CreatedAt - } - } - - require.NoError(t, scanForBadEntries(log, metrics, ds)(context.Background())) - - resp, err := ds.ListRegistrationEntries(context.Background(), &datastore.ListRegistrationEntriesRequest{}) - require.NoError(t, err) - - sortEntries(tt.expectEntries) - sortEntries(resp.Entries) - spiretest.AssertProtoListEqual(t, tt.expectEntries, resp.Entries) - - // Assert the logs are as expected. The Entry IDs need to be - // patched up in the log entries since they are non-deterministic. - logEntries := logHook.AllEntries() - for _, logEntry := range logEntries { - if _, ok := logEntry.Data["entry_id"]; ok { - logEntry.Data["entry_id"] = "SOME-ID" - } - } - spiretest.AssertLogs(t, logEntries, tt.expectLogs) - - assert.Equal(t, []fakemetrics.MetricItem{ - { - Type: fakemetrics.SetGaugeType, - Key: []string{"entry", "deleted"}, - Val: float32(tt.expectDeleted), - }, - }, metrics.AllMetrics()) - }) - } -} - -func sortEntries(es []*common.RegistrationEntry) { - sort.Slice(es, func(a, b int) bool { - return es[a].EntryId < es[b].EntryId - }) -} diff --git a/pkg/server/server.go b/pkg/server/server.go index 7dbf4e622e..7dc76690c7 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -208,7 +208,6 @@ func (s *Server) run(ctx context.Context) (err error) { registrationManager.Run, bundlePublishingManager.Run, util.SerialRun(s.waitForTestDial, healthChecker.ListenAndServe), - scanForBadEntries(s.config.Log, metrics, cat.GetDataStore()), } if s.config.LogReopener != nil {