From da03e3fad256832009a94ca3e0a98bf7e3e4770e Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 17 Jan 2022 10:12:40 -0600 Subject: [PATCH 1/5] add race and shardwidth22 to Gitlab CI, cleanup our coverage reporting was a bit wonky and had files coming from both test and test-future... made everything come from future --- .gitlab/.gitlab-ci.yml | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 4164ba8da..6dbd26dc8 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -12,7 +12,7 @@ include: - .go/pkg/mod/ variables: - GOVERSION: "1.16.10" + GOVERSION: "1.16.13" stages: - lint @@ -76,15 +76,34 @@ run go tests: - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' script: - echo "Running featurebase unit tests..." - - PKG_LIST=$(go list ./... | grep -v internal/clustertests | paste -s -d, -) - - go test -coverprofile=coverage.out -covermode=atomic -coverpkg=${PKG_LIST} ./... - artifacts: - paths: - - coverage.out + - go test ./... +run go tests race: + stage: test + image: golang:$GOVERSION + extends: .go-cache + rules: + - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' + script: + - echo "Running featurebase race tests..." + - go test -race -timeout=30m ./... + +run go tests shardwidth22: + stage: test + image: golang:$GOVERSION + extends: .go-cache + rules: + - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' + script: + - echo "Running featurebase race tests..." + - go test -tags=shardwidth22 ./... + +# we do coverage reporting from the future tests because the json +# output is very difficult to human-read. The alternative would be to +# run the regular tests twice and also run the future tests. run go tests future: stage: test - image: golang:1.17.3 + image: golang:1.17.6 extends: .go-cache rules: - if: '$CI_PIPELINE_SOURCE == "push" || $CI_PIPELINE_SOURCE == "schedule" || $CI_PIPELINE_SOURCE == "web"' @@ -107,7 +126,7 @@ upload to sonarcloud: script: - sonar-scanner -Dsonar.projectKey=molecula_featurebase -Dsonar.organization=molecula -Dsonar.sources=. -Dsonar.host.url=https://sonarcloud.io -Dsonar.go.coverage.reportPaths=coverage.out -Dsonar.go.tests.reportPaths=test-report.out -Dsonar.javascript.lcov.reportPaths=lattice/coverage/lcov.info needs: - - job: run go tests + - job: run go tests future - job: run jest tests build for linux amd64: @@ -211,6 +230,7 @@ build container fb: # 3. make sure docker/docker-compose is installed # 4. make sure the git config is done `git config --global --add url."ssh://git@github.com/".insteadOf "https://github.com/"` # 5. Add deploy key github.com/molecula/featurebase/settings/keys and add public key in .ssh folder of gitlab-runner user +# TODO: (I think) get clustertests coverage added to coverage report clustertests: stage: integration tags: @@ -224,6 +244,7 @@ clustertests: external lookup tests: stage: integration image: golang:$GOVERSION + # TODO: no rules here, do we need to add the rules line? variables: POSTGRES_DB: $POSTGRES_DB POSTGRES_USER: $POSTGRES_USER From a16fee5f88efc2ca85e93b4c8d257b8b2ed2cd05 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 17 Jan 2022 10:57:01 -0600 Subject: [PATCH 2/5] set shardWidth properly in client the shardwidth22 tests were broken client side, but we didn't realize this because we weren't running the client side tests since moving the client code into the main FB repo until recently (woops), and more recently, we'd stopped running the shardwidth22 tests in the move to Gitlab, so when we re-enabled them we finally noticed that they were broken in the client. All this change does is takes the shardWidth value from the core featurebase package instead of using a hardcoded value in the client package. --- client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/client.go b/client/client.go index 211e7c3f7..082a56634 100644 --- a/client/client.go +++ b/client/client.go @@ -36,7 +36,7 @@ import ( const PQLVersion = "1.0" // DefaultShardWidth is used if an index doesn't have it defined. -const DefaultShardWidth = 1 << 20 +const DefaultShardWidth = pilosa.ShardWidth const maxHosts = 10 From fdf7b4107a16fe45e60639ad4d372219b372c0ac Mon Sep 17 00:00:00 2001 From: reesporte Date: Tue, 18 Jan 2022 12:14:54 -0600 Subject: [PATCH 3/5] actually be able to generate-statik these were the changes i had to make to be able to build lattice on my machine --- lattice/Dockerfile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lattice/Dockerfile b/lattice/Dockerfile index 2b30d6cfb..6aa669e54 100644 --- a/lattice/Dockerfile +++ b/lattice/Dockerfile @@ -1,10 +1,10 @@ FROM moleculacorp/nodejs:latest as build - +# make sure that your docker settings allow for at least like 4gb of ram, it +# takes a lot to build this WORKDIR /lattice - COPY package.json ./ -COPY yarn.lock ./ -RUN yarn install +RUN apk update && apk upgrade yarn +RUN yarn install --network-timeout 100000 COPY . ./ RUN yarn build From e7552a76a7aa32762e0ce1f6af4f6c70b3e45d06 Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 19 Jan 2022 12:08:05 -0600 Subject: [PATCH 4/5] fix bug where drop table wasn't being authorized also fixes bug in GetAuthorizedIndexList where perms weren't being properly compared --- authz/authorization.go | 4 ++-- server/grpc.go | 33 +++++++++++++++++++++++--- server/grpc_test.go | 53 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index a2127f33d..1cd0d3dc1 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -120,7 +120,7 @@ func (p *GroupPermissions) IsAdmin(groups []authn.Group) bool { func (p *GroupPermissions) GetAuthorizedIndexList(groups []authn.Group, desiredPermission Permission) (indexList []string) { // if user is admin, find all indexes in permissions file and return them - if admin := p.IsAdmin(groups); admin { + if p.IsAdmin(groups) { for groupId := range p.Permissions { for index := range p.Permissions[groupId] { indexList = append(indexList, index) @@ -132,7 +132,7 @@ func (p *GroupPermissions) GetAuthorizedIndexList(groups []authn.Group, desiredP for _, group := range groups { if _, ok := p.Permissions[group.GroupID]; ok { for index, permission := range p.Permissions[group.GroupID] { - if permission >= desiredPermission { + if permission.Satisfies(desiredPermission) { indexList = append(indexList, index) } } diff --git a/server/grpc.go b/server/grpc.go index f24823915..c57374020 100644 --- a/server/grpc.go +++ b/server/grpc.go @@ -30,6 +30,7 @@ import ( "google.golang.org/grpc/peer" "google.golang.org/grpc/reflection" "google.golang.org/grpc/status" + "vitess.io/vitess/go/vt/sqlparser" ) // GRPCHandler contains methods which handle the various gRPC requests. @@ -174,7 +175,13 @@ func (h *GRPCHandler) QuerySQL(req *pb.QuerySQLRequest, stream pb.Pilosa_QuerySQ return errors.Wrap(err, "parsing SQL") } - allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, authz.Read) + perm := authz.Read + switch parsed.Statement.(type) { + case *sqlparser.DDL: // currently only used for DropTable + perm = authz.Admin + } + + allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, perm) if !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { if !isAllowed(parsed.Tables, allowed) { return status.Error(codes.PermissionDenied, "insufficient permissions to access requested tables") @@ -219,9 +226,29 @@ func (h *GRPCHandler) QuerySQL(req *pb.QuerySQLRequest, stream pb.Pilosa_QuerySQ func (h *GRPCHandler) QuerySQLUnary(ctx context.Context, req *pb.QuerySQLRequest) (*pb.TableResponse, error) { start := time.Now() uinfo := ctx.Value("userinfo") - if uinfo != nil && !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { - ctx = context.WithValue(ctx, "indices", h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, authz.Read)) + if uinfo != nil { + // authz + m := sql.NewMapper() + parsed, err := m.MapSQL(req.Sql) + if err != nil { + return nil, errors.Wrap(err, "parsing SQL") + } + + perm := authz.Read + switch parsed.Statement.(type) { + case *sqlparser.DDL: // currently only used for DropTable + perm = authz.Admin + } + + allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, perm) + if !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { + if !isAllowed(parsed.Tables, allowed) { + return nil, status.Error(codes.PermissionDenied, "insufficient permissions to access requested tables") + } + ctx = context.WithValue(ctx, "indices", allowed) + } } + results, err := h.execSQL(ctx, req.Sql) if err != nil { return nil, err diff --git a/server/grpc_test.go b/server/grpc_test.go index c2b993a96..acfa48943 100644 --- a/server/grpc_test.go +++ b/server/grpc_test.go @@ -1162,6 +1162,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` t.Fatal(err) } }) + t.Run("test-show-tables-unary-admin", func(t *testing.T) { response, err := gh.QuerySQLUnary(adminCtx, &pb.QuerySQLRequest{ Sql: "show tables", @@ -1190,6 +1191,55 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` t.Fatal(err) } }) + + t.Run("test-drop-table-unary-read", func(t *testing.T) { + _, err := gh.QuerySQLUnary(readCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + + t.Run("test-drop-table-unary-write", func(t *testing.T) { + _, err := gh.QuerySQLUnary(writeCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + + t.Run("test-drop-table-unary-admin", func(t *testing.T) { + _, err := gh.QuerySQLUnary(adminCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err != nil { + t.Fatalf("expected nil error but got %v", err) + } + }) + + t.Run("test-drop-table-stream-read", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: readCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + t.Run("test-drop-table-stream-write", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: writeCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + t.Run("test-drop-table-stream-admin", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: adminCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err != nil { + t.Fatalf("expected nil error but got %v", err) + } + }) } func TestCRUDIndexes(t *testing.T) { @@ -1506,6 +1556,9 @@ func setUpTestQuerySQLUnary(ctx context.Context, t *testing.T) (gh *server.GRPCH // delete_me m.MustCreateIndex(t, "delete_me", pilosa.IndexOptions{TrackExistence: true}) + m.MustCreateIndex(t, "another_one", pilosa.IndexOptions{TrackExistence: true}) + m.MustCreateIndex(t, "deletable_index", pilosa.IndexOptions{TrackExistence: true}) + return gh, func() { if err := m.API.DeleteIndex(ctx, joiner.Name()); err != nil { panic(err) From 61ef1aee4e694316810238236c84d01e714ede20 Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 19 Jan 2022 12:55:07 -0600 Subject: [PATCH 5/5] fix older tests --- server/grpc_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/grpc_test.go b/server/grpc_test.go index acfa48943..3508d03ab 100644 --- a/server/grpc_test.go +++ b/server/grpc_test.go @@ -843,6 +843,8 @@ func TestQuerySQL(t *testing.T) { {"Table", "string"}, }, rows: []row{ + {[]columnResponse{"another_one"}}, + {[]columnResponse{"deletable_index"}}, {[]columnResponse{"delete_me"}}, {[]columnResponse{"grouper"}}, {[]columnResponse{"joiner"}}, @@ -881,6 +883,9 @@ func TestQuerySQL(t *testing.T) { {"Table", "string"}, }, rows: []row{ + {[]columnResponse{"another_one"}}, + {[]columnResponse{"deletable_index"}}, + {[]columnResponse{"grouper"}}, {[]columnResponse{"joiner"}}, },