diff --git a/NEXT_RELEASE_NOTES.md b/NEXT_RELEASE_NOTES.md index dd3e12514..be7391eee 100644 --- a/NEXT_RELEASE_NOTES.md +++ b/NEXT_RELEASE_NOTES.md @@ -62,6 +62,7 @@ The release notes should contain at least the following sections: ## Important information +* Searching RID identification service areas now fails with a 400 error when more than 10000 areas match, instead of silently returning a truncated list (see issue [#1120](https://github.com/interuss/dss/issues/1120)). Clients that hit this must reduce the size of the requested area or time range. Creating or updating a RID subscription in such an area now fails the same way, because the response lists the identification service areas it covers; note the subscription is still written. * Fixed a bug where the `evict` command ignored entries without a locality. If your DSS instance does not have a locality set, the next `evict` run may be slow while it processes the backlog of old entries. * Fixed a bug where Helm charts and Tanka files didn't actually perform any actions via the `evict` command when run via cron jobs (because no locality was set and no delete flag was specified). If you have a large number of entries, the next run may be slow while it processes the backlog of old entries. * AWS load balancer names are no longer enforced by Helm charts or Tanka files. Existing clusters will retain their current names, while new ones will use names automatically generated by AWS. diff --git a/pkg/rid/store/memstore/identification_service_area.go b/pkg/rid/store/memstore/identification_service_area.go index bf7bb042b..550d0923e 100644 --- a/pkg/rid/store/memstore/identification_service_area.go +++ b/pkg/rid/store/memstore/identification_service_area.go @@ -120,8 +120,11 @@ func (r *repo) SearchISAs(_ context.Context, cells s2.CellUnion, earliest *time. } out = append(out, rec.toModel()) - if len(out) > dssmodels.MaxResultLimit { // This mimics sqlstore behaviour, but it's not very good. - break + // One match beyond the limit is enough to know the response cannot be + // exhaustive, see #1120. + if len(out) > dssmodels.MaxResultLimit { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, + "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) } } return out, nil diff --git a/pkg/rid/store/memstore/identification_service_area_test.go b/pkg/rid/store/memstore/identification_service_area_test.go index f6baa8dbf..e9080ce6d 100644 --- a/pkg/rid/store/memstore/identification_service_area_test.go +++ b/pkg/rid/store/memstore/identification_service_area_test.go @@ -7,10 +7,12 @@ import ( "github.com/golang/geo/s2" "github.com/google/uuid" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" ridmodels "github.com/interuss/dss/pkg/rid/models" "github.com/interuss/dss/pkg/rid/repos" "github.com/interuss/dss/pkg/timestamp" + "github.com/interuss/stacktrace" "github.com/stretchr/testify/require" ) @@ -322,3 +324,32 @@ func TestStoreCountISAs(t *testing.T) { require.NoError(t, err) require.Equal(t, int64(0), count) } + +func TestStoreSearchISAsResultLimit(t *testing.T) { + ctx := context.Background() + ctx = timestamp.WithRequestTimestamp(ctx, fakeClock.Now()) + repo := setUpStore(t) + + insertISA := func() { + isa := *serviceArea + isa.ID = dssmodels.ID(uuid.New().String()) + _, err := repo.InsertISA(ctx, &isa) + require.NoError(t, err) + } + + for i := 0; i < dssmodels.MaxResultLimit; i++ { + insertISA() + } + + // Exactly MaxResultLimit matches still fits in an exhaustive response. + serviceAreas, err := repo.SearchISAs(ctx, serviceArea.Cells, &startTime, nil) + require.NoError(t, err) + require.Len(t, serviceAreas, dssmodels.MaxResultLimit) + + insertISA() + + serviceAreas, err = repo.SearchISAs(ctx, serviceArea.Cells, &startTime, nil) + require.Error(t, err) + require.Equal(t, dsserr.BadRequest, stacktrace.GetCode(err)) + require.Nil(t, serviceAreas) +} diff --git a/pkg/rid/store/sqlstore/identification_service_area.go b/pkg/rid/store/sqlstore/identification_service_area.go index c361344eb..266940e01 100644 --- a/pkg/rid/store/sqlstore/identification_service_area.go +++ b/pkg/rid/store/sqlstore/identification_service_area.go @@ -21,7 +21,16 @@ const ( updateISAFields = "id, url, cells, starts_at, ends_at, writer, updated_at" ) -func (r *repo) fetchISAs(ctx context.Context, query string, args ...interface{}) ([]*ridmodels.IdentificationServiceArea, error) { +// fetchISAs runs query and returns the ISAs it matches. When limitRows is set, one row beyond +// dssmodels.MaxResultLimit is selected so that a result set overflowing the limit is rejected +// rather than returned silently truncated, see #1120. Only set it for a plain SELECT: the LIMIT +// is appended to the end of the query, which the RETURNING statements reaching this helper +// through fetchISA do not accept. +func (r *repo) fetchISAs(ctx context.Context, limitRows bool, query string, args ...interface{}) ([]*ridmodels.IdentificationServiceArea, error) { + if limitRows { + query = fmt.Sprintf("%s LIMIT %d", query, dssmodels.MaxResultLimit+1) + } + rows, err := r.Query(ctx, query, args...) if err != nil { return nil, stacktrace.Propagate(err, "Error in query: %s", query) @@ -59,11 +68,16 @@ func (r *repo) fetchISAs(ctx context.Context, query string, args ...interface{}) return nil, stacktrace.Propagate(err, "Error in rows query result") } + if limitRows && len(payload) > dssmodels.MaxResultLimit { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, + "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) + } + return payload, nil } func (r *repo) fetchISA(ctx context.Context, query string, args ...interface{}) (*ridmodels.IdentificationServiceArea, error) { - isas, err := r.fetchISAs(ctx, query, args...) + isas, err := r.fetchISAs(ctx, false, query, args...) if err != nil { return nil, err // No need to Propagate this error as this stack layer does not add useful information } @@ -195,8 +209,7 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim AND COALESCE(starts_at <= $2, true) AND - cells && $3 - LIMIT $4`, isaFields) + cells && $3`, isaFields) ) if len(cells) == 0 { @@ -207,11 +220,12 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim return nil, stacktrace.NewError("Earliest start time is missing") } - return r.fetchISAs(ctx, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells), dssmodels.MaxResultLimit) + return r.fetchISAs(ctx, true, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells)) } // ListExpiredISAs lists all expired ISAs based on writer. // The function queries both empty writer and null writer when passing empty string as a writer. +// Truncation is wanted for this operator-run evict sweep: the remainder is picked up next run. func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) { if len(writer) == 0 { isasInCellsQuery := fmt.Sprintf(` @@ -224,7 +238,7 @@ func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold tim AND (writer = '' OR writer IS NULL) LIMIT $2`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, dssmodels.MaxResultLimit) + return r.fetchISAs(ctx, false, isasInCellsQuery, threshold, dssmodels.MaxResultLimit) } isasInCellsQuery := fmt.Sprintf(` @@ -237,7 +251,7 @@ func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold tim AND writer = $2 LIMIT $3`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, writer, dssmodels.MaxResultLimit) + return r.fetchISAs(ctx, false, isasInCellsQuery, threshold, writer, dssmodels.MaxResultLimit) } func (r *repo) CountISAs(ctx context.Context) (int64, error) { diff --git a/pkg/rid/store/sqlstore/identification_service_area_test.go b/pkg/rid/store/sqlstore/identification_service_area_test.go index 715a30155..7e485f94d 100644 --- a/pkg/rid/store/sqlstore/identification_service_area_test.go +++ b/pkg/rid/store/sqlstore/identification_service_area_test.go @@ -2,6 +2,8 @@ package sqlstore import ( "context" + "errors" + "fmt" "testing" "time" @@ -10,6 +12,9 @@ import ( dssmodels "github.com/interuss/dss/pkg/models" ridmodels "github.com/interuss/dss/pkg/rid/models" "github.com/interuss/dss/pkg/rid/repos" + "github.com/interuss/stacktrace" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jonboulle/clockwork" "github.com/stretchr/testify/require" ) @@ -357,3 +362,45 @@ func TestStoreCountISAs(t *testing.T) { require.NoError(t, err) require.Equal(t, int64(0), count) } + +// recordingQueryable captures the query it is handed and then fails the call, which is enough to +// assert on query construction without a datastore: fetchISAs returns as soon as Query does. +type recordingQueryable struct { + query string +} + +var errRecorded = errors.New("query recorded") + +func (q *recordingQueryable) Query(_ context.Context, query string, _ ...interface{}) (pgx.Rows, error) { + q.query = query + return nil, errRecorded +} + +func (q *recordingQueryable) QueryRow(_ context.Context, _ string, _ ...interface{}) pgx.Row { + panic("not needed by these tests") +} + +func (q *recordingQueryable) Exec(_ context.Context, _ string, _ ...interface{}) (pgconn.CommandTag, error) { + panic("not needed by these tests") +} + +func TestFetchISAsAppliesRowLimit(t *testing.T) { + var ( + ctx = context.Background() + q = &recordingQueryable{} + r = &repo{Queryable: q} + pastLimit = fmt.Sprintf("LIMIT %d", dssmodels.MaxResultLimit+1) + ) + + // A search asks for one row past the limit, so that an over-limit result set can be told + // apart from one that exactly fills it. + _, err := r.SearchISAs(ctx, serviceArea.Cells, &startTime, &endTime) + require.Equal(t, errRecorded, stacktrace.RootCause(err)) + require.Contains(t, q.query, pastLimit) + + // The evict sweep keeps its own truncating limit rather than the search one. + _, err = r.ListExpiredISAs(ctx, writer, endTime) + require.Equal(t, errRecorded, stacktrace.RootCause(err)) + require.NotContains(t, q.query, pastLimit) + require.Contains(t, q.query, "LIMIT $3") +}