From 66c7d59707a994a01e24af4626a80f7cbee46b99 Mon Sep 17 00:00:00 2001 From: reesporte Date: Fri, 5 Nov 2021 15:39:50 -0500 Subject: [PATCH 01/23] remove dot imports from catcher.go --- catcher.go | 74 +++++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/catcher.go b/catcher.go index 92ce5b2b9..b4f23fa61 100644 --- a/catcher.go +++ b/catcher.go @@ -17,7 +17,7 @@ package pilosa import ( "github.com/molecula/featurebase/v2/roaring" txkey "github.com/molecula/featurebase/v2/short_txkey" - . "github.com/molecula/featurebase/v2/vprint" + "github.com/molecula/featurebase/v2/vprint" ) // catcher is useful to report error locations with a @@ -46,8 +46,8 @@ func (c *catcherTx) NewTxIterator(index, field, view string, shard uint64) *roar func (c *catcherTx) ImportRoaringBits(index, field, view string, shard uint64, rit roaring.RoaringIterator, clear bool, log bool, rowSize uint64) (changed int, rowSet map[uint64]int, err error) { defer func() { if r := recover(); r != nil { - AlwaysPrintf("see ImportRoaringBits() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see ImportRoaringBits() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.ImportRoaringBits(index, field, view, shard, rit, clear, log, rowSize) @@ -56,8 +56,8 @@ func (c *catcherTx) ImportRoaringBits(index, field, view string, shard uint64, r func (c *catcherTx) Rollback() { defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Rollback() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Rollback() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() c.b.Rollback() @@ -67,8 +67,8 @@ func (c *catcherTx) Commit() error { defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Commit() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Commit() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Commit() @@ -78,8 +78,8 @@ func (c *catcherTx) RoaringBitmap(index, field, view string, shard uint64) (*roa defer func() { if r := recover(); r != nil { - AlwaysPrintf("see RoaringBitmap() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see RoaringBitmap() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.RoaringBitmap(index, field, view, shard) @@ -89,8 +89,8 @@ func (c *catcherTx) Container(index, field, view string, shard uint64, key uint6 defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Container() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Container() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Container(index, field, view, shard, key) @@ -100,8 +100,8 @@ func (c *catcherTx) PutContainer(index, field, view string, shard uint64, key ui defer func() { if r := recover(); r != nil { - AlwaysPrintf("see PutContainer() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see PutContainer() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.PutContainer(index, field, view, shard, key, rc) @@ -111,8 +111,8 @@ func (c *catcherTx) RemoveContainer(index, field, view string, shard uint64, key defer func() { if r := recover(); r != nil { - AlwaysPrintf("see RemoveContainer() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see RemoveContainer() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.RemoveContainer(index, field, view, shard, key) @@ -122,8 +122,8 @@ func (c *catcherTx) Add(index, field, view string, shard uint64, a ...uint64) (c defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Add() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Add() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Add(index, field, view, shard, a...) @@ -133,8 +133,8 @@ func (c *catcherTx) Remove(index, field, view string, shard uint64, a ...uint64) defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Remove() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Remove() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Remove(index, field, view, shard, a...) @@ -144,8 +144,8 @@ func (c *catcherTx) Contains(index, field, view string, shard uint64, key uint64 defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Contains() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Contains() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Contains(index, field, view, shard, key) @@ -155,8 +155,8 @@ func (c *catcherTx) ContainerIterator(index, field, view string, shard uint64, f defer func() { if r := recover(); r != nil { - AlwaysPrintf("see ContainerIterator() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see ContainerIterator() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.ContainerIterator(index, field, view, shard, firstRoaringContainerKey) @@ -166,8 +166,8 @@ func (c *catcherTx) ForEach(index, field, view string, shard uint64, fn func(i u defer func() { if r := recover(); r != nil { - AlwaysPrintf("see ForEach() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see ForEach() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.ForEach(index, field, view, shard, fn) @@ -177,8 +177,8 @@ func (c *catcherTx) ForEachRange(index, field, view string, shard uint64, start, defer func() { if r := recover(); r != nil { - AlwaysPrintf("see ForEachRange() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see ForEachRange() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.ForEachRange(index, field, view, shard, start, end, fn) @@ -188,8 +188,8 @@ func (c *catcherTx) Count(index, field, view string, shard uint64) (uint64, erro defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Count() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Count() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Count(index, field, view, shard) @@ -199,8 +199,8 @@ func (c *catcherTx) Max(index, field, view string, shard uint64) (uint64, error) defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Max() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Max() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Max(index, field, view, shard) @@ -210,8 +210,8 @@ func (c *catcherTx) Min(index, field, view string, shard uint64) (uint64, bool, defer func() { if r := recover(); r != nil { - AlwaysPrintf("see Min() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see Min() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.Min(index, field, view, shard) @@ -221,8 +221,8 @@ func (c *catcherTx) CountRange(index, field, view string, shard uint64, start, e defer func() { if r := recover(); r != nil { - AlwaysPrintf("see CountRange() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see CountRange() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.CountRange(index, field, view, shard, start, end) @@ -232,8 +232,8 @@ func (c *catcherTx) OffsetRange(index, field, view string, shard, offset, start, defer func() { if r := recover(); r != nil { - AlwaysPrintf("see OffsetRange() PanicOn '%v' at '%v'", r, Stack()) - PanicOn(r) + vprint.AlwaysPrintf("see OffsetRange() PanicOn '%v' at '%v'", r, vprint.Stack()) + vprint.PanicOn(r) } }() return c.b.OffsetRange(index, field, view, shard, offset, start, end) From 28e1be754032072f787a731c1e6cdc34a1ba5b98 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 29 Nov 2021 09:40:05 -0600 Subject: [PATCH 02/23] added AuthN/AuthZ parameters to featurebase server configuration --- ctl/server.go | 7 +++++++ server/config.go | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/ctl/server.go b/ctl/server.go index d53bf81af..ac1dcd204 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -121,4 +121,11 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // Toggle /schema/details endpoint. flags.BoolVar(&srv.Config.SchemaDetailsOn, "schema-details-on", true, "Disable /schema/details endpoint") + + // OAuth2.0 identity provider configuration + flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") + flags.StringVar(&srv.Config.Auth.IdentityProviderURL, "auth.identity-provider-url", srv.Config.Auth.IdentityProviderURL, "Base URL for identity provider.") + flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Base URL for authorize.") + flags.StringVar(&srv.Config.Auth.UserInfoURL, "auth.user-info-url", srv.Config.Auth.UserInfoURL, "Base URL for user info.") + flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Application/Client ID") } diff --git a/server/config.go b/server/config.go index a42eb7917..da9c9e43a 100644 --- a/server/config.go +++ b/server/config.go @@ -240,6 +240,24 @@ type Config struct { // Toggles /schema/details endpoint. If off, it returns empty. SchemaDetailsOn bool `toml:"schema-details-on"` + + // Enable AuthZ/AuthN + Auth struct { + // Enable AuthZ/AuthN for featurebase server + Enable bool `toml:"enable"` + + // Base URL for identity provider + IdentityProviderURL string `toml:"identity-provider-url"` + + // Authorize URL + AuthorizeURL string `toml:"authorize-url"` + + // User info URL + UserInfoURL string `toml:"user-info-url"` + + // Application/Client ID + ClientId string `toml:"client-id"` + } `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -392,6 +410,13 @@ func NewConfig() *Config { // Schema Details Toggle c.SchemaDetailsOn = true + // AuthZ/AuthN disabled by default + c.Auth.Enable = false + c.Auth.IdentityProviderURL = "" + c.Auth.AuthorizeURL = "" + c.Auth.UserInfoURL = "" + c.Auth.ClientId = "" + return c } From dc1c39fd21a76155110e9074820dfe3434cc24f8 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 29 Nov 2021 16:46:55 -0600 Subject: [PATCH 03/23] updated featurebase.conf --- install/featurebase.conf | 8 ++++++++ server/auth.go | 35 +++++++++++++++++++++++++++++++++++ server/config.go | 9 +++++---- server/server.go | 8 ++++++++ 4 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 server/auth.go diff --git a/install/featurebase.conf b/install/featurebase.conf index 73895ed6d..d3fd41deb 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -371,3 +371,11 @@ log-path = "/var/log/molecula/featurebase.log" # ============================================================================== +# Enable/Disable AuthN/AuthZ for featurebase +# Can choose identity provider, pass authorize and user-info endpoints, and client id +# [auth] +# enable = false +# identity-provider-url = "http://place-holder" +# authorize-url = "http://place-holder" +# user-info-url = "http://place-holder" +# client-id = "http://place-holder" \ No newline at end of file diff --git a/server/auth.go b/server/auth.go new file mode 100644 index 000000000..f74444963 --- /dev/null +++ b/server/auth.go @@ -0,0 +1,35 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +type Options struct { + Enable bool `toml:"enable"` + IdentityProviderURL string `toml:"identity-provider-url"` + AuthorizeURL string `toml:"authorize-url"` + UserInfoURL string `toml:"user-info-url"` + ClientId string `toml:"client-id"` +} + +func authenticateUser(opt Options) (resp bool) { + + // fmt.Println("IdentityProviderURL", opt.IdentityProviderURL) + // fmt.Println("AuthorizeURL", opt.AuthorizeURL) + // fmt.Println("UserInfoURL", opt.UserInfoURL) + // fmt.Println("ClientId", opt.ClientId) + + resp = false + + return resp +} diff --git a/server/config.go b/server/config.go index da9c9e43a..c726cb5be 100644 --- a/server/config.go +++ b/server/config.go @@ -411,11 +411,12 @@ func NewConfig() *Config { c.SchemaDetailsOn = true // AuthZ/AuthN disabled by default + // default identity provider is azure active directory c.Auth.Enable = false - c.Auth.IdentityProviderURL = "" - c.Auth.AuthorizeURL = "" - c.Auth.UserInfoURL = "" - c.Auth.ClientId = "" + c.Auth.IdentityProviderURL = "http://holder-identity-provider" + c.Auth.AuthorizeURL = "http://holder-authorize-url" + c.Auth.UserInfoURL = "http://holder-user-info-url" + c.Auth.ClientId = "http://holder-client-id" return c } diff --git a/server/server.go b/server/server.go index 29eb20f1d..e24c261cb 100644 --- a/server/server.go +++ b/server/server.go @@ -234,6 +234,14 @@ func (m *Command) Start() (err error) { return errors.Wrap(err, "setting resource limits") } + if m.Config.Auth.Enable == true { + // check authentication for user + resp := authenticateUser(m.Config.Auth) + if resp == false { + log.Fatalf("Authentication failed: Unable to access to featurebase server") + } + } + // Initialize server. if err = m.Server.Open(); err != nil { return errors.Wrap(err, "opening server") From 8b20406f45374e5e6203596afab2d739d3a7c974 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 30 Nov 2021 11:38:07 -0600 Subject: [PATCH 04/23] experiment: handle errors more gracefully, but don't stream CreateKeys We were trying to write an error to a ResponseWriter After attempting to write to it, and this produces messages about superfluous WriteHeaders, which is correct. This patch changes things so that we report messages more clearly and verbosely if we hit them before writing, and if we try to write and fail, we log the message because that's all we can do. This does change semantics slightly, in that now we're marshalling separately from trying to write the marshalled data. I think this is probably a reasonable call because it lets us get diagnostics about a hypothetical encoding problem, but in practice I don't think there should be any encoding problems. So my guess is the actual error will occur in that last line, and be logged to the server console instead of failing to write over HTTP. Also note that this changes some of the messages to include the underlying error they're complaining about. We also merge the create/find and index/field cases because only a couple of lines of code changed between four largeish functions, and we test some of the failure cases. We don't have test coverage on the "field isn't provided" type things because the mux won't actually route things there without them, so far as I know. --- http/handler.go | 214 +++++++++++-------------------------------- http/handler_test.go | 112 +++++++++++++++++++++- 2 files changed, 167 insertions(+), 159 deletions(-) diff --git a/http/handler.go b/http/handler.go index d3aa20d6a..f000d3738 100644 --- a/http/handler.go +++ b/http/handler.go @@ -3091,7 +3091,7 @@ func (h *Handler) handlePostTranslateIndexDB(w http.ResponseWriter, r *http.Requ resp.write(w, err) } -func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { +func (h *Handler) handleFindOrCreateKeys(w http.ResponseWriter, r *http.Request, requireField bool, create bool) { // Verify input and output types if r.Header.Get("Content-Type") != "application/json" { http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) @@ -3101,178 +3101,76 @@ func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { http.Error(w, "Not acceptable", http.StatusNotAcceptable) return } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - + var indexName, fieldName string var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } + err := func() error { + var ok bool + indexName, ok = mux.Vars(r)["index"] + if !ok { + return errors.New("index name is required") + } - translations, err := h.api.FindIndexKeys(r.Context(), indexName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } + if requireField { + fieldName, ok = mux.Vars(r)["field"] + if !ok { + return errors.New("field name is required") + } + } - err = json.NewEncoder(w).Encode(translations) + bd, err := readBody(r) + if err != nil { + return fmt.Errorf("failed to read body: %v", err) + } + + err = json.Unmarshal(bd, &keys) + if err != nil { + return fmt.Errorf("failed to decode request: %v", err) + } + return nil + }() if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) + http.Error(w, err.Error(), http.StatusBadRequest) + } + var translations map[string]uint64 + switch { + case requireField && create: + translations, err = h.api.CreateFieldKeys(r.Context(), indexName, fieldName, keys...) + case requireField && !create: + translations, err = h.api.FindFieldKeys(r.Context(), indexName, fieldName, keys...) + case !requireField && create: + translations, err = h.api.CreateIndexKeys(r.Context(), indexName, keys...) + case !requireField && !create: + translations, err = h.api.FindIndexKeys(r.Context(), indexName, keys...) + + } + if err != nil { + http.Error(w, fmt.Sprintf("translating keys: %v", err), http.StatusInternalServerError) return } + data, err := json.Marshal(translations) + if err != nil { + http.Error(w, fmt.Sprintf("encoding response: %v", err), http.StatusInternalServerError) + } + _, err = w.Write(data) + if err != nil { + h.logger.Printf("writing CreateFieldKeys response: %v", err) + } +} + +func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { + h.handleFindOrCreateKeys(w, r, false, false) } func (h *Handler) handleFindFieldKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - fieldName, ok := mux.Vars(r)["field"] - if !ok { - http.Error(w, "field name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.FindFieldKeys(r.Context(), indexName, fieldName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, true, false) } func (h *Handler) handleCreateIndexKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.CreateIndexKeys(r.Context(), indexName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, false, true) } func (h *Handler) handleCreateFieldKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - fieldName, ok := mux.Vars(r)["field"] - if !ok { - http.Error(w, "field name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.CreateFieldKeys(r.Context(), indexName, fieldName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, true, true) } func (h *Handler) handleMatchField(w http.ResponseWriter, r *http.Request) { diff --git a/http/handler_test.go b/http/handler_test.go index bfea699d3..ca841268b 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -19,6 +19,7 @@ import ( "fmt" "net" gohttp "net/http" + "strings" "testing" pilosa "github.com/molecula/featurebase/v2" @@ -167,6 +168,115 @@ func TestIngestSchemaHandler(t *testing.T) { schemaURL := fmt.Sprintf("%s/internal/schema", m.URL()) resp := test.Do(t, "POST", schemaURL, string(schema)) if resp.StatusCode != gohttp.StatusOK { - t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + // now, try again, expecting a failure: + resp = test.Do(t, "POST", schemaURL, string(schema)) + if resp.StatusCode != gohttp.StatusConflict { + t.Errorf("invalid status: expected 409, got %d, body=%s", resp.StatusCode, resp.Body) + } +} + +func TestTranslationHandlers(t *testing.T) { + // reusable data for the tests + nameBytes, err := json.Marshal([]string{"a", "b", "c"}) + if err != nil { + t.Fatalf("marshalling json: %v", err) + } + names := string(nameBytes) + + c := test.MustRunCluster(t, 1) + defer c.Close() + + schema := ` +{ + "index-name": "example", + "primary-key-type": "string", + "index-action": "create", + "fields": [ + { + "field-name": "stringset", + "field-type": "string", + "field-options": { + "cache-type": "ranked", + "cache-size": 100000 + } + } + ] +} +` + m := c.GetPrimary() + schemaURL := fmt.Sprintf("%s/internal/schema", m.URL()) + resp := test.Do(t, "POST", schemaURL, string(schema)) + if resp.StatusCode != gohttp.StatusOK { + t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + baseURLs := []string{ + fmt.Sprintf("%s/internal/translate/index/example/", m.URL()), + fmt.Sprintf("%s/internal/translate/field/example/stringset/", m.URL()), + fmt.Sprintf("%s/internal/translate/field/example/nonexistent/", m.URL()), + } + for _, url := range baseURLs { + expectFailure := strings.HasSuffix(url, "/nonexistent/") + createURL := url + "keys/create" + findURL := url + "keys/find" + var results map[string]uint64 + + if expectFailure { + resp := test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusInternalServerError { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + resp = test.Do(t, "POST", createURL, names) + if resp.StatusCode != gohttp.StatusInternalServerError { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + continue + } + + // try to find them when they don't exist + resp := test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err := json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 0 { + t.Fatalf("finding keys before any were set: expected no results, got %d (%q)", len(results), results) + } + + // try to create them, but malformed, so we expect an error + resp = test.Do(t, "POST", createURL, names[:6]) + if resp.StatusCode != gohttp.StatusBadRequest { + t.Fatalf("invalid status: expected 400, got %d, body=%s", resp.StatusCode, resp.Body) + } + + // try to create them + resp = test.Do(t, "POST", createURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err = json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 3 { + t.Fatalf("finding keys before any were set: expected 3 results, got %d (%q)", len(results), results) + } + + // try to find them now that they exist + resp = test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err = json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 3 { + t.Fatalf("finding keys before any were set: expected 3 results, got %d (%q)", len(results), results) + } } } From b5ba3fb2ead2f9ef2fe42746826691ded51e83d0 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 13:09:13 -0600 Subject: [PATCH 05/23] added auth arg validation and set up auth package --- auth/auth.go | 47 +++++++++++++++++++ ctl/server.go | 10 ++-- install/featurebase.conf | 9 ++-- server/auth.go | 35 -------------- server/config.go | 56 ++++++++++++++++++----- server/config_internal_test.go | 83 ++++++++++++++++++++++++++++++++++ server/server.go | 8 ++-- 7 files changed, 189 insertions(+), 59 deletions(-) create mode 100644 auth/auth.go delete mode 100644 server/auth.go diff --git a/auth/auth.go b/auth/auth.go new file mode 100644 index 000000000..95b222bf4 --- /dev/null +++ b/auth/auth.go @@ -0,0 +1,47 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +type AUTH struct { + ClientId string + ClientSecret string + AuthorizeURL string + TokenURL string + GroupEndpointURL string +} + +// func (c *config) Init(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) { +// c.ClientId = ClientId +// c.ClientSecret = ClientSecret +// c.AuthorizeURL = AuthorizeURL +// c.TokenURL = TokenURL +// c.GroupEndpointURL = GroupEndpointURL +// } + +// apiOption is a functional option type for pilosa.API +type authOption func(*AUTH) error + +func OptAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) authOption { + return func(a *AUTH) error { + a.ClientId = ClientId + a.ClientSecret = ClientSecret + a.AuthorizeURL = AuthorizeURL + a.TokenURL = TokenURL + a.GroupEndpointURL = GroupEndpointURL + return nil + } +} + +// redirectURL diff --git a/ctl/server.go b/ctl/server.go index ac1dcd204..66a8c63d4 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -124,8 +124,10 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // OAuth2.0 identity provider configuration flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") - flags.StringVar(&srv.Config.Auth.IdentityProviderURL, "auth.identity-provider-url", srv.Config.Auth.IdentityProviderURL, "Base URL for identity provider.") - flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Base URL for authorize.") - flags.StringVar(&srv.Config.Auth.UserInfoURL, "auth.user-info-url", srv.Config.Auth.UserInfoURL, "Base URL for user info.") - flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Application/Client ID") + flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Identity Provider's Application/Client ID.") + flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Application/Client Secret.") + flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") + flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL for identity provider.") + flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") + } diff --git a/install/featurebase.conf b/install/featurebase.conf index d3fd41deb..7807134e7 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -375,7 +375,8 @@ log-path = "/var/log/molecula/featurebase.log" # Can choose identity provider, pass authorize and user-info endpoints, and client id # [auth] # enable = false -# identity-provider-url = "http://place-holder" -# authorize-url = "http://place-holder" -# user-info-url = "http://place-holder" -# client-id = "http://place-holder" \ No newline at end of file +# client-id = "" +# client-secret = "" +# authorize-url = "" +# token-url = "" +# group-endpoint-url = "" \ No newline at end of file diff --git a/server/auth.go b/server/auth.go deleted file mode 100644 index f74444963..000000000 --- a/server/auth.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2017 Pilosa Corp. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package server - -type Options struct { - Enable bool `toml:"enable"` - IdentityProviderURL string `toml:"identity-provider-url"` - AuthorizeURL string `toml:"authorize-url"` - UserInfoURL string `toml:"user-info-url"` - ClientId string `toml:"client-id"` -} - -func authenticateUser(opt Options) (resp bool) { - - // fmt.Println("IdentityProviderURL", opt.IdentityProviderURL) - // fmt.Println("AuthorizeURL", opt.AuthorizeURL) - // fmt.Println("UserInfoURL", opt.UserInfoURL) - // fmt.Println("ClientId", opt.ClientId) - - resp = false - - return resp -} diff --git a/server/config.go b/server/config.go index c726cb5be..c65ae7f9c 100644 --- a/server/config.go +++ b/server/config.go @@ -19,6 +19,7 @@ import ( "fmt" "log" "net" + "net/url" "runtime" "strconv" "strings" @@ -246,17 +247,20 @@ type Config struct { // Enable AuthZ/AuthN for featurebase server Enable bool `toml:"enable"` - // Base URL for identity provider - IdentityProviderURL string `toml:"identity-provider-url"` + // Application/Client ID + ClientId string `toml:"client-id"` + + // Client Secret + ClientSecret string `toml:"client-secret"` // Authorize URL AuthorizeURL string `toml:"authorize-url"` - // User info URL - UserInfoURL string `toml:"user-info-url"` + // Token URL + TokenURL string `toml:"token-url"` - // Application/Client ID - ClientId string `toml:"client-id"` + // Group Endpoint URL + GroupEndpointURL string `toml:"group-endpoint-url"` } `toml:"auth"` } @@ -291,6 +295,7 @@ func (c *Config) validate() error { "Etcd.ClusterURL", c.Etcd.ClusterURL, "Postgres.Bind", c.Postgres.Bind, } + ports := make(map[int]bool) n := len(hostPort) for i := 0; i < n; i += 2 { @@ -411,12 +416,7 @@ func NewConfig() *Config { c.SchemaDetailsOn = true // AuthZ/AuthN disabled by default - // default identity provider is azure active directory c.Auth.Enable = false - c.Auth.IdentityProviderURL = "http://holder-identity-provider" - c.Auth.AuthorizeURL = "http://holder-authorize-url" - c.Auth.UserInfoURL = "http://holder-user-info-url" - c.Auth.ClientId = "http://holder-client-id" return c } @@ -628,3 +628,37 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin // No IPv4 address, return the first resolved address instead. return addrs[0].String(), nil } + +func (c *Config) ValidateAuth() error { + authURL := []string{ + "ClientId", c.Auth.ClientId, + "ClientSecret", c.Auth.ClientSecret, + "AuthorizeURL", c.Auth.AuthorizeURL, + "TokenURL", c.Auth.TokenURL, + "GroupEndpointURL", c.Auth.GroupEndpointURL, + } + + n := len(authURL) + for i := 0; i < n; i += 2 { + name := authURL[i] + value := authURL[i+1] + if strings.Contains(name, "URL") { + _, err := url.ParseRequestURI(value) + if err != nil { + return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) + } + } else { + if value == "" { + return fmt.Errorf("Empty string for auth config %s", name) + } + } + } + return nil +} + +func (c *Config) MustValidateAuth() { + err := c.ValidateAuth() + if err != nil { + panic(err) + } +} diff --git a/server/config_internal_test.go b/server/config_internal_test.go index f48db1a16..97b1cfa20 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -288,3 +288,86 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { }) } } + +type params struct { + enable bool + clientId string + clientSecret string + authorizeURL string + tokenURL string + groupEndpointURL string +} + +func TestConfig_validateAuth(t *testing.T) { + tests := []struct { + expErr string + input params + expected params + }{ + {"Empty string for auth config ClientId", + params{true, "", "", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Empty string for auth config ClientSecret", + params{true, "clientid", "", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Empty string for auth config ClientId", + params{true, "", "clientSecret", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config AuthorizeURL", + params{true, "client", "secret", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config TokenURL", + params{true, "client", "secret", "https://url.com/", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config GroupEndpointURL", + params{true, "client", "secret", "https://url.com/", "https://url.com/", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config AuthorizeURL", + params{true, "client", "secret", "string", "https://url.com/", "https://url.com/"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config TokenURL", + params{true, "client", "secret", "https://url.com/", "not-a-url", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config GroupEndpointURL", + params{true, "client", "secret", "https://url.com/", "https://url.com/", "not-valid-url"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"", + params{true, "client", "secret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + c := NewConfig() + c.Auth.Enable = test.input.enable + c.Auth.ClientId = test.input.clientId + c.Auth.ClientSecret = test.input.clientSecret + c.Auth.AuthorizeURL = test.input.authorizeURL + c.Auth.TokenURL = test.input.tokenURL + c.Auth.GroupEndpointURL = test.input.groupEndpointURL + + err := c.ValidateAuth() + + if err != nil && test.expErr == "" { + t.Fatal(err) + } else if err == nil && test.expErr != "" { + t.Fatalf("expected error string to contain %s, but got no error", test.expErr) + } else if err != nil && test.expErr != "" { + if !strings.Contains(err.Error(), test.expErr) { + t.Fatalf("expected error string to contain %s, but got %s", test.expErr, err.Error()) + } + return + } + }) + } +} diff --git a/server/server.go b/server/server.go index e24c261cb..30b1794ee 100644 --- a/server/server.go +++ b/server/server.go @@ -41,6 +41,7 @@ import ( "golang.org/x/sync/errgroup" pilosa "github.com/molecula/featurebase/v2" + "github.com/molecula/featurebase/v2/auth" "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/encoding/proto" petcd "github.com/molecula/featurebase/v2/etcd" @@ -235,11 +236,8 @@ func (m *Command) Start() (err error) { } if m.Config.Auth.Enable == true { - // check authentication for user - resp := authenticateUser(m.Config.Auth) - if resp == false { - log.Fatalf("Authentication failed: Unable to access to featurebase server") - } + m.Config.MustValidateAuth() + auth.OptAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) } // Initialize server. From 64b31da4a16e8d76321507d242c63fe75f728eab Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:00:51 -0600 Subject: [PATCH 06/23] resolved duplicated line --- server/config_internal_test.go | 66 +++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 97b1cfa20..98da9c0a9 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -299,50 +299,60 @@ type params struct { } func TestConfig_validateAuth(t *testing.T) { + errorMesgClientID := "Empty string for auth config ClientId" + errorMesgClientSecret := "Empty string for auth config ClientSecret" + errorMesgAuthURL := "Invalid URL for auth config AuthorizeURL" + errorMesgTokenURL := "Invalid URL for auth config TokenURL" + errorMesgGroupEndpointURL := "Invalid URL for auth config GroupEndpointURL" + validTestURL := "https://url.com/" + validClientID := "clientid" + validClientSecret := "clientSecret" + notValidURL := "not-a-url" + tests := []struct { expErr string input params expected params }{ - {"Empty string for auth config ClientId", + {errorMesgClientID, params{true, "", "", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Empty string for auth config ClientSecret", - params{true, "clientid", "", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgClientSecret, + params{true, validClientID, "", "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Empty string for auth config ClientId", - params{true, "", "clientSecret", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgClientID, + params{true, "", validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config AuthorizeURL", - params{true, "client", "secret", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgAuthURL, + params{true, validClientID, validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config TokenURL", - params{true, "client", "secret", "https://url.com/", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgTokenURL, + params{true, validClientID, validClientSecret, validTestURL, "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config GroupEndpointURL", - params{true, "client", "secret", "https://url.com/", "https://url.com/", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgGroupEndpointURL, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config AuthorizeURL", - params{true, "client", "secret", "string", "https://url.com/", "https://url.com/"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgAuthURL, + params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config TokenURL", - params{true, "client", "secret", "https://url.com/", "not-a-url", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgTokenURL, + params{true, validClientID, validClientSecret, validTestURL, notValidURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config GroupEndpointURL", - params{true, "client", "secret", "https://url.com/", "https://url.com/", "not-valid-url"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgGroupEndpointURL, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {"", - params{true, "client", "secret", "https://url.com/", "https://url.com/", "https://url.com/"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, } From 91c8bf5e05524b39134c6ff10539c98047fa1801 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:04:21 -0600 Subject: [PATCH 07/23] fixed duplicated empty string --- server/config_internal_test.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 98da9c0a9..60735d45e 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -308,6 +308,7 @@ func TestConfig_validateAuth(t *testing.T) { validClientID := "clientid" validClientSecret := "clientSecret" notValidURL := "not-a-url" + emptyString := "" tests := []struct { expErr string @@ -315,27 +316,27 @@ func TestConfig_validateAuth(t *testing.T) { expected params }{ {errorMesgClientID, - params{true, "", "", "", "", ""}, + params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientSecret, - params{true, validClientID, "", "", "", ""}, + params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientID, - params{true, "", validClientSecret, "", "", ""}, + params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, - params{true, validClientID, validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, "", ""}, + params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, @@ -343,14 +344,14 @@ func TestConfig_validateAuth(t *testing.T) { params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, notValidURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"", + {emptyString, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, From 7de3eaa935f0e243a9621359264b896621b31e17 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:19:59 -0600 Subject: [PATCH 08/23] resolved review's comment --- server/config_internal_test.go | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 60735d45e..b8aede210 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -311,49 +311,38 @@ func TestConfig_validateAuth(t *testing.T) { emptyString := "" tests := []struct { - expErr string - input params - expected params + expErr string + input params }{ {errorMesgClientID, params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientSecret, params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientID, params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {emptyString, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, } From aebd4c4c62f1ee8fc3bcaffebdbf8bcba98ffe30 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:22:08 -0600 Subject: [PATCH 09/23] fixed formatting --- server/config_internal_test.go | 41 +++++++++------------------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index b8aede210..1bd33be84 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -309,41 +309,22 @@ func TestConfig_validateAuth(t *testing.T) { validClientSecret := "clientSecret" notValidURL := "not-a-url" emptyString := "" + enable := true tests := []struct { expErr string input params }{ - {errorMesgClientID, - params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, - }, - {errorMesgClientSecret, - params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, - }, - {errorMesgClientID, - params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, - }, - {errorMesgAuthURL, - params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, - }, - {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, - }, - {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, - }, - {errorMesgAuthURL, - params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, - }, - {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, - }, - {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, - }, - {emptyString, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, - }, + {errorMesgClientID, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgClientSecret, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgClientID, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgAuthURL, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, + {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, + {errorMesgAuthURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, + {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, + {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, + {emptyString, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}}, } for i, test := range tests { From 50d80f2f1c67f53f00349c5e01bf03d4a4ca3dad Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:52:38 -0600 Subject: [PATCH 10/23] fixed arg cli descriptions --- ctl/server.go | 4 ++-- server/config.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ctl/server.go b/ctl/server.go index 66a8c63d4..a368637e3 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -125,9 +125,9 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // OAuth2.0 identity provider configuration flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Identity Provider's Application/Client ID.") - flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Application/Client Secret.") + flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Client Secret.") flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") - flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL for identity provider.") + flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.") flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") } diff --git a/server/config.go b/server/config.go index c65ae7f9c..9f7a24e61 100644 --- a/server/config.go +++ b/server/config.go @@ -630,7 +630,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() error { - authURL := []string{ + authConfig := []string{ "ClientId", c.Auth.ClientId, "ClientSecret", c.Auth.ClientSecret, "AuthorizeURL", c.Auth.AuthorizeURL, @@ -638,10 +638,10 @@ func (c *Config) ValidateAuth() error { "GroupEndpointURL", c.Auth.GroupEndpointURL, } - n := len(authURL) + n := len(authConfig) for i := 0; i < n; i += 2 { - name := authURL[i] - value := authURL[i+1] + name := authConfig[i] + value := authConfig[i+1] if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { From 1df90af26e98bbcd35383257af1e1aed4a9899f6 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Thu, 2 Dec 2021 15:57:52 -0600 Subject: [PATCH 11/23] free bitmap pages on deallocate --- client/client_it_test.go | 1 - rbf/README.md | 6 +-- rbf/tx.go | 8 +++- rbf/tx_test.go | 99 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 5 deletions(-) diff --git a/client/client_it_test.go b/client/client_it_test.go index 1b5ca2884..b9137bee9 100644 --- a/client/client_it_test.go +++ b/client/client_it_test.go @@ -592,7 +592,6 @@ func TestClientAgainstCluster(t *testing.T) { target := []uint64{100} require.Equalf(t, target, resp.Result().Row().Columns, "Row Result Columns") }) - t.Run("StoreQuery", func(t *testing.T) { schema := NewSchema() testIndexStore := schema.Index("test-index-store") diff --git a/rbf/README.md b/rbf/README.md index 4f6efa072..cbc5b74a7 100644 --- a/rbf/README.md +++ b/rbf/README.md @@ -93,12 +93,12 @@ The leaf page contains a series of cells with the header of: [8] highbits [4] flag [4] child count - [*] array or RLE data + [*] array or RLE data or Handle (a pageno) to Bitmap Data -### Bitmap page +### Bitmap Data page -The data for the bitmap page takes up the entire 8KB. +The data for the bitmap data page takes up the entire 8KB. ## Proof of Concept Notes diff --git a/rbf/tx.go b/rbf/tx.go index 812d35738..52cc49366 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -333,7 +333,6 @@ func (tx *Tx) DeleteBitmapsWithPrefix(prefix string) error { if !strings.HasPrefix(name.(string), prefix) { continue } - // Deallocate all pages in the tree. if err := tx.deallocateTree(pgno.(uint32)); err != nil { return err @@ -1065,6 +1064,13 @@ func (tx *Tx) deallocateTree(pgno uint32) error { return tx.freePgno(pgno) case PageTypeLeaf: + for i, n := 0, readCellN(page); i < n; i++ { + if cell := readLeafCell(page, i); cell.Type == ContainerTypeBitmapPtr { + if err := tx.freePgno(toPgno(cell.Data)); err != nil { + return err + } + } + } return tx.freePgno(pgno) default: return fmt.Errorf("rbf.Tx.deallocateTree(): invalid page type: pgno=%d type=%d", pgno, typ) diff --git a/rbf/tx_test.go b/rbf/tx_test.go index e4b68f4b6..8a354a916 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -684,3 +684,102 @@ func TestTx_CreateBitmap(t *testing.T) { } }) } + +func TestTx_DeleteBitmapsWithPrefix(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + prefix := "abc" + bitmapSize := 10000 + // var err error + // create a interleaved set up array and bitmap containers + + bits := make([]uint64, bitmapSize) + x := uint64(1) + for i := 0; i < len(bits); i++ { + bits[i] = x + x = x + 2 + } + ifError := func(err error) { + if err != nil { + t.Fatal(err) + } + } + checkInfos := func() { + tx := MustBegin(t, db, false) + defer tx.Rollback() + infos, err := tx.PageInfos() + ifError(err) + for pgno, info := range infos { + switch info := info.(type) { + case *rbf.MetaPageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "meta") + fmt.Printf("pageN=%d,walid=%d,rootrec=%d,freelist=%d\n", info.PageN, info.WALID, info.RootRecordPageNo, info.FreelistPageNo) + + case *rbf.RootRecordPageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "rootrec") + fmt.Printf("next=%d\n", info.Next) + + case *rbf.LeafPageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "leaf") + fmt.Printf("flags=x%x,celln=%d\n", info.Flags, info.CellN) + + case *rbf.BranchPageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "branch") + fmt.Printf("flags=x%x,celln=%d\n", info.Flags, info.CellN) + + case *rbf.BitmapPageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "bitmap") + fmt.Printf("-\n") + + case *rbf.FreePageInfo: + fmt.Printf("%-8d ", pgno) + fmt.Printf("%-10s ", "free") + fmt.Printf("-\n") + + default: + t.Fatal(fmt.Sprintf("unexpected page info type %T", info)) + } + } + + } + populate := func() { + tx := MustBegin(t, db, true) + defer tx.Rollback() + for i := uint64(0); i < 16; i++ { + bm := roaring.NewBitmap() + if i%3 == 0 { + bm.Put(i, roaring.NewContainerBitmap(6144, bits)) + if _, err := tx.AddRoaring(prefix, bm); err != nil { + panic(err) + } + } else { + bm.Put(i, roaring.NewContainerArray([]uint16{2, 4, 5, 7})) + if _, err := tx.AddRoaring(prefix, bm); err != nil { + panic(err) + } + + } + } + ifError(tx.Commit()) + } + + checkInfos() + populate() + checkInfos() + ifError(db.Check()) + + tx := MustBegin(t, db, true) + tx.DeleteBitmapsWithPrefix(prefix) + ifError(tx.Commit()) + ifError(db.Check()) + checkInfos() + populate() + ifError(db.Check()) + checkInfos() + +} From 78e11e0fda2aebdf7760b6eb9418c0b5899b8e0a Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 16:15:15 -0600 Subject: [PATCH 12/23] resolved reviewer's comments --- auth/auth.go | 30 +++++++++--------------------- server/config.go | 3 --- server/server.go | 5 ++++- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 95b222bf4..2abfda28f 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -22,26 +22,14 @@ type AUTH struct { GroupEndpointURL string } -// func (c *config) Init(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) { -// c.ClientId = ClientId -// c.ClientSecret = ClientSecret -// c.AuthorizeURL = AuthorizeURL -// c.TokenURL = TokenURL -// c.GroupEndpointURL = GroupEndpointURL -// } - -// apiOption is a functional option type for pilosa.API -type authOption func(*AUTH) error - -func OptAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) authOption { - return func(a *AUTH) error { - a.ClientId = ClientId - a.ClientSecret = ClientSecret - a.AuthorizeURL = AuthorizeURL - a.TokenURL = TokenURL - a.GroupEndpointURL = GroupEndpointURL - return nil +func NewAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) AUTH { + a := AUTH{ + ClientId: ClientId, + ClientSecret: ClientSecret, + AuthorizeURL: AuthorizeURL, + TokenURL: TokenURL, + GroupEndpointURL: GroupEndpointURL, } -} -// redirectURL + return a +} diff --git a/server/config.go b/server/config.go index 9f7a24e61..f8e411f37 100644 --- a/server/config.go +++ b/server/config.go @@ -415,9 +415,6 @@ func NewConfig() *Config { // Schema Details Toggle c.SchemaDetailsOn = true - // AuthZ/AuthN disabled by default - c.Auth.Enable = false - return c } diff --git a/server/server.go b/server/server.go index 30b1794ee..768f000ed 100644 --- a/server/server.go +++ b/server/server.go @@ -56,6 +56,7 @@ import ( "github.com/molecula/featurebase/v2/statsd" "github.com/molecula/featurebase/v2/syswrap" "github.com/molecula/featurebase/v2/testhook" + "github.com/molecula/featurebase/v2/vprint" "github.com/pelletier/go-toml" "github.com/pkg/errors" ) @@ -237,7 +238,9 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable == true { m.Config.MustValidateAuth() - auth.OptAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) + authArgs := auth.NewAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) + vprint.VV("Auth: %v", authArgs) + // print statement is so that binary compiles, and golang doesn't complaint about declared but unused var } // Initialize server. From 978f236c5948e1c34afd05db30f3a719434e8173 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 16:36:04 -0600 Subject: [PATCH 13/23] resolved additional comments --- server/config.go | 28 ++++++++++++---------------- server/config_internal_test.go | 27 +++++++++++++-------------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/server/config.go b/server/config.go index f8e411f37..e93244dcb 100644 --- a/server/config.go +++ b/server/config.go @@ -627,35 +627,31 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() error { - authConfig := []string{ - "ClientId", c.Auth.ClientId, - "ClientSecret", c.Auth.ClientSecret, - "AuthorizeURL", c.Auth.AuthorizeURL, - "TokenURL", c.Auth.TokenURL, - "GroupEndpointURL", c.Auth.GroupEndpointURL, + authConfig := map[string]string{ + "ClientId": c.Auth.ClientId, + "ClientSecret": c.Auth.ClientSecret, + "AuthorizeURL": c.Auth.AuthorizeURL, + "TokenURL": c.Auth.TokenURL, + "GroupEndpointURL": c.Auth.GroupEndpointURL, } - n := len(authConfig) - for i := 0; i < n; i += 2 { - name := authConfig[i] - value := authConfig[i+1] + for name, value := range authConfig { + if value == "" { + return fmt.Errorf("Empty string for auth config %s", name) + } + if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) } - } else { - if value == "" { - return fmt.Errorf("Empty string for auth config %s", name) - } } } return nil } func (c *Config) MustValidateAuth() { - err := c.ValidateAuth() - if err != nil { + if err := c.ValidateAuth(); err != nil { panic(err) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 1bd33be84..5daac4233 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -299,32 +299,31 @@ type params struct { } func TestConfig_validateAuth(t *testing.T) { - errorMesgClientID := "Empty string for auth config ClientId" - errorMesgClientSecret := "Empty string for auth config ClientSecret" - errorMesgAuthURL := "Invalid URL for auth config AuthorizeURL" - errorMesgTokenURL := "Invalid URL for auth config TokenURL" - errorMesgGroupEndpointURL := "Invalid URL for auth config GroupEndpointURL" + errorMesgEmpty := "Empty string" + errorMesgURL := "Invalid URL" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" notValidURL := "not-a-url" emptyString := "" enable := true + disable := false tests := []struct { expErr string input params }{ - {errorMesgClientID, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgClientSecret, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgClientID, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgAuthURL, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, - {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, - {errorMesgAuthURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, - {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, - {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, + {errorMesgEmpty, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgEmpty, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgEmpty, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgEmpty, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, + {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, + {errorMesgURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, + {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, + {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, {emptyString, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}}, + {errorMesgEmpty, params{disable, emptyString, emptyString, emptyString, emptyString, emptyString}}, } for i, test := range tests { From 63c5c11108ea223e693ae41bea9cc16ebf25abe9 Mon Sep 17 00:00:00 2001 From: reesporte Date: Fri, 5 Nov 2021 15:42:53 -0500 Subject: [PATCH 14/23] fix some staticcheck issues --- client/csv/csv.go | 14 ++--- cmd/badloader/badloader.go | 24 ++++---- cmd/random-query/main.go | 18 +++--- cmd/slurp/slurp.go | 30 +++++----- ctl/backup.go | 2 +- ctl/restore.go | 2 +- dbshard.go | 16 ++--- etcd/embed.go | 6 +- fragment.go | 12 ++-- holder.go | 4 +- http/client.go | 3 + internal/clustertests/pause_node_test.go | 7 +-- lru/lru.go | 22 ------- pprof.go | 22 +++---- pql/ast.go | 2 +- rbf.go | 4 +- rbf/rbf.go | 30 +++++----- rbf/tx.go | 24 ++++---- rbf/util.go | 10 ++-- rrtx.go | 4 +- stattx.go | 76 ++++++++++++------------ test/holder.go | 8 +-- testhook/auditor.go | 2 + txfactory.go | 50 ++++++++-------- view.go | 14 ++--- 25 files changed, 193 insertions(+), 213 deletions(-) diff --git a/client/csv/csv.go b/client/csv/csv.go index 54335a082..a9c193d39 100644 --- a/client/csv/csv.go +++ b/client/csv/csv.go @@ -56,7 +56,7 @@ func ColumnUnmarshallerWithTimestamp(format Format, timestampFormat string) Reco column := client.Column{} parts := strings.Split(text, ",") if len(parts) < 2 { - return nil, errors.New("Invalid CSV line") + return nil, errors.New("invalid CSV line") } hasRowKey := format == RowKeyColumnID || format == RowKeyColumnKey @@ -67,7 +67,7 @@ func ColumnUnmarshallerWithTimestamp(format Format, timestampFormat string) Reco } else { column.RowID, err = strconv.ParseUint(parts[0], 10, 64) if err != nil { - return nil, errors.New("Invalid row ID") + return nil, errors.New("invalid row ID") } } @@ -76,7 +76,7 @@ func ColumnUnmarshallerWithTimestamp(format Format, timestampFormat string) Reco } else { column.ColumnID, err = strconv.ParseUint(parts[1], 10, 64) if err != nil { - return nil, errors.New("Invalid column ID") + return nil, errors.New("invalid column ID") } } @@ -166,17 +166,17 @@ func FieldValueUnmarshaller(format Format) RecordUnmarshaller { return func(text string) (client.Record, error) { parts := strings.Split(text, ",") if len(parts) < 2 { - return nil, errors.New("Invalid CSV") + return nil, errors.New("invalid CSV") } value, err := strconv.ParseInt(parts[1], 10, 64) if err != nil { - return nil, errors.New("Invalid value") + return nil, errors.New("invalid value") } switch format { case ColumnID: columnID, err := strconv.ParseUint(parts[0], 10, 64) if err != nil { - return nil, errors.New("Invalid column ID at line: %d") + return nil, errors.New("invalid column ID at line: %d") } return client.FieldValue{ ColumnID: uint64(columnID), @@ -188,7 +188,7 @@ func FieldValueUnmarshaller(format Format) RecordUnmarshaller { Value: value, }, nil default: - return nil, fmt.Errorf("Invalid format: %d", format) + return nil, fmt.Errorf("invalid format: %d", format) } } } diff --git a/cmd/badloader/badloader.go b/cmd/badloader/badloader.go index fabb50216..d64d93cde 100644 --- a/cmd/badloader/badloader.go +++ b/cmd/badloader/badloader.go @@ -20,16 +20,15 @@ import ( "context" "time" - //"fmt" "fmt" "io" "io/ioutil" gohttp "net/http" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/http" pnet "github.com/molecula/featurebase/v2/net" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" "os" "strconv" @@ -38,7 +37,6 @@ import ( func UploadTar(srcFile string, client *http.InternalClient) error { t0 := time.Now() - f, err := os.Open(srcFile) if err != nil { return (err) @@ -65,7 +63,7 @@ func UploadTar(srcFile string, client *http.InternalClient) error { header, err := tarReader.Next() if err == io.EOF { if header != nil { - PanicOn("header should not be nil on err io.EOF") + vprint.PanicOn("header should not be nil on err io.EOF") } //submit any stuff we have left if len(viewData) > 0 { @@ -75,13 +73,13 @@ func UploadTar(srcFile string, client *http.InternalClient) error { // Submit(lastIndex, lastField, lastShard, request) uri := GetImportRoaringURI(lastIndex, lastShard) err := client.ImportRoaring(context.Background(), uri, lastIndex, lastField, lastShard, false, request) - PanicOn(err) + vprint.PanicOn(err) } return nil } n++ if n%500 == 0 { - VV("n = %v, progress, elapsed '%v'", n, time.Since(t0)) + vprint.VV("n = %v, progress, elapsed '%v'", n, time.Since(t0)) } parts := strings.Split(header.Name, "/") //vv("parts = '%#v'", parts) @@ -100,7 +98,7 @@ func UploadTar(srcFile string, client *http.InternalClient) error { } //vv("about to submit lastIndex='%v' lastShard='%v'", lastIndex, lastShard) uri := GetImportRoaringURI(lastIndex, lastShard) - PanicOn(client.ImportRoaring(context.Background(), uri, lastIndex, lastField, lastShard, false, request)) + vprint.PanicOn(client.ImportRoaring(context.Background(), uri, lastIndex, lastField, lastShard, false, request)) viewData = make(map[string][]byte) //vv("done with submit lastIndex='%v' lastShard='%v'; took='%v'", lastIndex, lastShard, time.Since(t0)) @@ -111,7 +109,7 @@ func UploadTar(srcFile string, client *http.InternalClient) error { return err } if _, already := viewData[view]; already { - PanicOn(fmt.Sprintf("view '%v' already present!", view)) + vprint.PanicOn(fmt.Sprintf("view '%v' already present!", view)) } viewData[view] = roaringData lastIndex = index @@ -130,12 +128,12 @@ func main() { host := "127.0.0.1:10101" h := &gohttp.Client{} c, err := http.NewInternalClient(host, h) - PanicOn(err) + vprint.PanicOn(err) tarSrcPath := "q2.tar.gz" t0 := time.Now() - PanicOn(UploadTar(tarSrcPath, c)) - VV("total elapsed '%v'", time.Since(t0)) + vprint.PanicOn(UploadTar(tarSrcPath, c)) + vprint.VV("total elapsed '%v'", time.Since(t0)) } var globURI *pnet.URI @@ -143,7 +141,7 @@ var globURI *pnet.URI func init() { var err error globURI, err = pnet.NewURIFromHostPort("127.0.0.1", 10101) - PanicOn(err) + vprint.PanicOn(err) } // get correct node to go to. diff --git a/cmd/random-query/main.go b/cmd/random-query/main.go index 1faafad1e..32c0b6552 100644 --- a/cmd/random-query/main.go +++ b/cmd/random-query/main.go @@ -26,10 +26,10 @@ import ( "strings" "time" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/pql" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" ) // RandomQueryConfig @@ -168,9 +168,9 @@ func (cfg *RandomQueryConfig) Run() (err error) { dur := time.Since(t0) if dur > 0 { qps := 1e9 * float64(totalQ) / float64(dur) - AlwaysPrintf("totalQueries run: %v elapsed: %v qps: %0.02f", totalQ, dur, qps) + vprint.AlwaysPrintf("totalQueries run: %v elapsed: %v qps: %0.02f", totalQ, dur, qps) } else { - AlwaysPrintf("totalQueries run: %v elapsed: %v qps: N/A", totalQ, dur) + vprint.AlwaysPrintf("totalQueries run: %v elapsed: %v qps: N/A", totalQ, dur) } } defer report() @@ -211,7 +211,7 @@ NewSetup: index := indexes[cfg.Rnd.Intn(len(indexes))] pql, err := cfg.GenQuery(index) - PanicOn(err) + vprint.PanicOn(err) if cfg.Verbose { fmt.Printf("pql = '%v'\n", pql) @@ -220,7 +220,7 @@ NewSetup: // Query node0. res, err := cli.Query(ctx, index, &pilosa.QueryRequest{Index: index, Query: pql}) if err != nil { - AlwaysPrintf("QUERY FAILED! queries before this=%v; err = '%v', pql='%v'", loops, err, pql) + vprint.AlwaysPrintf("QUERY FAILED! queries before this=%v; err = '%v', pql='%v'", loops, err, pql) return err } if cfg.VeryVerbose { @@ -356,7 +356,7 @@ func (cfg *RandomQueryConfig) Setup(api API) (err error) { pql := fmt.Sprintf("Rows(%v)", fld.Name) res, err := api.Query(ctx, ii.Name, &pilosa.QueryRequest{Index: ii.Name, Query: pql}) - PanicOn(err) + vprint.PanicOn(err) if cfg.VeryVerbose { fmt.Printf("success on pql = '%v'; res='%v'\n", pql, res.Results[0]) } @@ -379,7 +379,7 @@ func (cfg *RandomQueryConfig) Setup(api API) (err error) { case "decimal": cfg.AddIntField(ii.Name, fld.Name, fld.Options.Min, fld.Options.Max, fld.Options.Scale, fld.Options.Type == "decimal") default: - AlwaysPrintf("ignoring field %q: unhandled type %q\n", fld.Name, fld.Options.Type) + vprint.AlwaysPrintf("ignoring field %q: unhandled type %q\n", fld.Name, fld.Options.Type) } } } @@ -412,7 +412,7 @@ func (cfg *RandomQueryConfig) AddIntField(index, field string, min, max pql.Deci cfg.IndexMap[index] = f } if min.Scale != scale || max.Scale != scale { - PanicOn(fmt.Sprintf("scale error; min scale %d, max scale %d, field scale %d, assumed they'd be equal", + vprint.PanicOn(fmt.Sprintf("scale error; min scale %d, max scale %d, field scale %d, assumed they'd be equal", min.Scale, max.Scale, scale)) } diff --git a/cmd/slurp/slurp.go b/cmd/slurp/slurp.go index cee15aa4c..e97768c4a 100644 --- a/cmd/slurp/slurp.go +++ b/cmd/slurp/slurp.go @@ -30,10 +30,10 @@ import ( "strings" "time" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/http" pnet "github.com/molecula/featurebase/v2/net" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" ) // slurp: slurp is a load-tester for importing bulk data. @@ -60,7 +60,7 @@ func (r *stateMachine) NewHeader(h *tar.Header, tr *tar.Reader) error { field := parts[2] view := parts[4] shard, err := strconv.ParseUint(parts[6], 10, 64) - PanicOn(err) + vprint.PanicOn(err) if index != r.lastIndex || field != r.lastField || shard != r.lastShard { err := r.Upload() if err != nil { @@ -84,7 +84,7 @@ func (r *stateMachine) NewHeader(h *tar.Header, tr *tar.Reader) error { if err != nil { return err } - VV("Finished import %v", time.Since(r.start)) + vprint.VV("Finished import %v", time.Since(r.start)) if r.profile != "" { stopProfile(r.host, r.profile) @@ -104,21 +104,21 @@ func (r *stateMachine) NewHeader(h *tar.Header, tr *tar.Reader) error { } byteData, err := ioutil.ReadAll(tr) - PanicOn(err) + vprint.PanicOn(err) br := bytes.NewReader(byteData) err = r.client.ImportFieldKeys(context.Background(), uri, index, fieldName, false, br) if err != nil { return err } default: - VV("%v", h.Name) + vprint.VV("%v", h.Name) index := parts[1] partition, err := strconv.ParseUint(v, 10, 64) if err != nil { return err } byteData, err := ioutil.ReadAll(tr) - PanicOn(err) + vprint.PanicOn(err) br := bytes.NewReader(byteData) err = r.client.ImportIndexKeys(context.Background(), uri, index, int(partition), false, br) @@ -176,10 +176,10 @@ func UploadTar(srcFile string, client *http.InternalClient, profile, host string break } if err != nil { - PanicOn(err) + vprint.PanicOn(err) } err = runner.NewHeader(header, tarReader) - PanicOn(err) + vprint.PanicOn(err) } return nil } @@ -194,7 +194,7 @@ func main() { flag.Parse() uri, err := pnet.NewURIFromAddress(host) - PanicOn(err) + vprint.PanicOn(err) globURI = uri h := &gohttp.Client{} @@ -202,12 +202,12 @@ func main() { startProfile(host) } c, err := http.NewInternalClient(host, h) - PanicOn(err) + vprint.PanicOn(err) t0 := time.Now() println("uploading", tarSrcPath) - PanicOn(UploadTar(tarSrcPath, c, profile, host)) - VV("total elapsed '%v'", time.Since(t0)) + vprint.PanicOn(UploadTar(tarSrcPath, c, profile, host)) + vprint.VV("total elapsed '%v'", time.Since(t0)) } func startProfile(host string) { @@ -248,10 +248,10 @@ func stopProfile(host, outfile string) { } fd, err := os.Create(outfile) - PanicOn(err) + vprint.PanicOn(err) defer fd.Close() _, err = io.Copy(fd, resp.Body) - PanicOn(err) + vprint.PanicOn(err) } diff --git a/ctl/backup.go b/ctl/backup.go index cdcfb1427..3a50a223b 100644 --- a/ctl/backup.go +++ b/ctl/backup.go @@ -103,7 +103,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) { } } if len(indexes) <= 0 { - return fmt.Errorf("Index not found to back up") + return fmt.Errorf("index not found to back up") } } diff --git a/ctl/restore.go b/ctl/restore.go index 87d4d3c87..7d763be01 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -153,7 +153,7 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. //NOTE SHOULD ONLY BE ONE for _, index := range schema.Indexes { if exists(index.Name) { - return fmt.Errorf("Index Exists %v", index.Name) + return fmt.Errorf("index Exists %v", index.Name) } logger.Printf("Create INDEX %v", index.Name) err = cmd.client.CreateIndex(ctx, index.Name, index.Options) diff --git a/dbshard.go b/dbshard.go index 15bd41915..e978609ff 100644 --- a/dbshard.go +++ b/dbshard.go @@ -28,7 +28,7 @@ import ( "github.com/molecula/featurebase/v2/storage" "github.com/pkg/errors" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" ) var _ = sort.Sort @@ -279,7 +279,7 @@ func (per *DBPerShard) LoadExistingDBs() (err error) { func (txf *TxFactory) NewDBPerShard(typ txtype, holderDir string, holder *Holder) (d *DBPerShard) { if holder.cfg == nil || holder.cfg.RBFConfig == nil || holder.cfg.StorageConfig == nil { - PanicOn("must have holder.cfg.RBFConfig and holder.cfg.StorageConfig set here") + vprint.PanicOn("must have holder.cfg.RBFConfig and holder.cfg.StorageConfig set here") } hasRoaring := false @@ -422,7 +422,7 @@ func (per *DBPerShard) unprotectedGetDBShard(index string, shard uint64, idx *In if dbs != nil && dbs.closed { // roaring txn are nil/fake anyway. Don't freak out. if per.typ != roaringTxn { - PanicOn(fmt.Sprintf("cannot retain closed dbs across holder ReOpen dbs='%p'; per.typ='%v'", dbs, per.typ)) + vprint.PanicOn(fmt.Sprintf("cannot retain closed dbs across holder ReOpen dbs='%p'; per.typ='%v'", dbs, per.typ)) } } if !ok { @@ -449,11 +449,11 @@ func (per *DBPerShard) unprotectedGetDBShard(index string, shard uint64, idx *In registry = globalRbfDBReg registry.(*rbfDBRegistrar).SetRBFConfig(per.RBFConfig) default: - PanicOn(fmt.Sprintf("unknown txtyp: '%v'", dbs.typ)) + vprint.PanicOn(fmt.Sprintf("unknown txtyp: '%v'", dbs.typ)) } path := dbs.pathForType(dbs.typ) w, err := registry.OpenDBWrapper(path, DetectMemAccessPastTx, per.StorageConfig) - PanicOn(err) + vprint.PanicOn(err) h := idx.Holder() w.SetHolder(h) dbs.Open = true @@ -470,7 +470,7 @@ func (per *DBPerShard) Close() (err error) { for _, dbi := range per.dbh.Index { for _, dbs := range dbi.Shard { err = dbs.Close() - PanicOn(err) + vprint.PanicOn(err) } } return @@ -546,7 +546,7 @@ func (per *DBPerShard) TypedDBPerShardGetShardsForIndex(ty txtype, idx *Index, r ignoreEmpty := false includeRoot := true dbf, err := listDirUnderDir(path, includeRoot, ignoreEmpty) - PanicOn(err) + vprint.PanicOn(err) for _, nm := range dbf { base := filepath.Base(nm) @@ -561,7 +561,7 @@ func (per *DBPerShard) TypedDBPerShardGetShardsForIndex(ty txtype, idx *Index, r // Parse filename into integer. shard, err := strconv.ParseUint(base[lenOfShardPrefix:], 10, 64) if err != nil { - PanicOn(err) + vprint.PanicOn(err) continue } diff --git a/etcd/embed.go b/etcd/embed.go index 7da2c9806..6a939fb4a 100644 --- a/etcd/embed.go +++ b/etcd/embed.go @@ -555,7 +555,7 @@ func (e *Etcd) deleteNodeData(key []byte, revision int64) error { e.knownNodes[peerID].resizeState = "" e.nodeStatesDirty = true default: - return fmt.Errorf("node watch: invalid prefix %q\n", prefix) + return fmt.Errorf("node watch: invalid prefix %q", prefix) } return nil } @@ -586,7 +586,7 @@ func (e *Etcd) putNodeData(key []byte, value []byte, revision int64) (err error) var newNode topology.Node err := json.Unmarshal(value, &newNode) if err != nil { - return fmt.Errorf("json unmarshal of node metadata: %v\n", err) + return fmt.Errorf("json unmarshal of node metadata: %v", err) } e.knownNodes[peerID].topologyNode = &newNode // This saves us one remake of the node later, probably. @@ -599,7 +599,7 @@ func (e *Etcd) putNodeData(key []byte, value []byte, revision int64) (err error) e.knownNodes[peerID].resizeState = string(value) e.nodeStatesDirty = true default: - return fmt.Errorf("node watch: invalid prefix %q\n", prefix) + return fmt.Errorf("node watch: invalid prefix %q", prefix) } return nil } diff --git a/fragment.go b/fragment.go index 674f2da65..613f4bc27 100644 --- a/fragment.go +++ b/fragment.go @@ -51,7 +51,7 @@ import ( "github.com/molecula/featurebase/v2/testhook" "github.com/molecula/featurebase/v2/topology" "github.com/molecula/featurebase/v2/tracing" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" "github.com/pkg/errors" ) @@ -204,7 +204,7 @@ func newFragment(holder *Holder, spec fragSpec, shard uint64, flags byte) *fragm idx := holder.Index(spec.index.name) if idx == nil { - PanicOn(fmt.Sprintf("got nil idx back for '%v' from holder!", spec.index)) + vprint.PanicOn(fmt.Sprintf("got nil idx back for '%v' from holder!", spec.index)) } f := &fragment{ @@ -615,7 +615,7 @@ func (f *fragment) row(tx Tx, rowID uint64) (*Row, error) { func (f *fragment) mustRow(tx Tx, rowID uint64) *Row { row, err := f.row(tx, rowID) if err != nil { - PanicOn(err) + vprint.PanicOn(err) } return row } @@ -1072,7 +1072,7 @@ func (f *fragment) setValueBase(txOrig Tx, columnID uint64, bitDepth uint64, val tx = f.idx.holder.txf.NewTx(Txo{Write: writable, Index: f.idx, Fragment: f, Shard: f.shard}) defer func() { if err == nil { - PanicOn(tx.Commit()) + vprint.PanicOn(tx.Commit()) } else { tx.Rollback() } @@ -1975,7 +1975,7 @@ func (f *fragment) Blocks() ([]FragmentBlock, error) { idx := f.holder.Index(f.index()) if idx == nil { err := fmt.Errorf("index() was nil in fragment.Blocks(): f.index()='%v'", f.index()) - PanicOn(err) + vprint.PanicOn(err) return nil, err } tx := idx.holder.txf.NewTx(Txo{Write: !writable, Index: idx, Fragment: f, Shard: f.shard}) @@ -2350,7 +2350,7 @@ func (p *parallelSlices) fullPrune() { return } if len(p.rows) != len(p.cols) { - PanicOn("parallelSlices must have same length for rows and columns") + vprint.PanicOn("parallelSlices must have same length for rows and columns") } unsorted := p.prune() if unsorted { diff --git a/holder.go b/holder.go index 909bd6ecd..a1770adaf 100644 --- a/holder.go +++ b/holder.go @@ -36,7 +36,7 @@ import ( "github.com/molecula/featurebase/v2/storage" "github.com/molecula/featurebase/v2/testhook" "github.com/molecula/featurebase/v2/topology" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -300,7 +300,7 @@ func NewHolder(path string, cfg *HolderConfig) *Holder { storage.SetRowCacheOn(cfg.RowcacheOn) txf, err := NewTxFactory(cfg.StorageConfig.Backend, h.IndexesPath(), h) - PanicOn(err) + vprint.PanicOn(err) h.txf = txf _ = testhook.Created(h.Auditor, h, nil) diff --git a/http/client.go b/http/client.go index 628c2a390..8f78dde4d 100644 --- a/http/client.go +++ b/http/client.go @@ -667,6 +667,9 @@ func (c *InternalClient) importHelper(ctx context.Context, req pilosa.Message, p // request over the wire, even though we still have to go through // the http interface. nodes, err = c.Nodes(ctx) + if err != nil { + return errors.Wrap(err, "getting nodes") + } } // "us" is a usable local node if any, "them" is every node that we need diff --git a/internal/clustertests/pause_node_test.go b/internal/clustertests/pause_node_test.go index 2de330a74..7558af214 100644 --- a/internal/clustertests/pause_node_test.go +++ b/internal/clustertests/pause_node_test.go @@ -31,7 +31,6 @@ import ( boltdb "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/disco" "github.com/molecula/featurebase/v2/http" - picli "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/net" "github.com/molecula/featurebase/v2/topology" "github.com/pkg/errors" @@ -81,7 +80,7 @@ func getAddress(node string) string { func getClients(addrs []string) ([]*http.InternalClient, error) { clients := make([]*http.InternalClient, 0, len(addrs)) for _, addr := range addrs { - c, err := picli.NewInternalClient(addr, picli.GetHTTPClient(nil)) + c, err := http.NewInternalClient(addr, http.GetHTTPClient(nil)) if err != nil { return nil, err } @@ -102,7 +101,7 @@ func getURIsFromAddresses(addrs []string) ([]*net.URI, error) { return uris, nil } -func readIndexTranslateData(ctx context.Context, client *picli.InternalClient, dirPath, index string, partition int) error { +func readIndexTranslateData(ctx context.Context, client *http.InternalClient, dirPath, index string, partition int) error { // read translateStore contents from endpoint r, err := client.IndexTranslateDataReader(ctx, index, partition) if err != nil { @@ -186,7 +185,7 @@ var errOpRetriable = errors.New("If operation failed on this error, it can be re func verifyNodeHasGivenKeys(ctx context.Context, node, index, dirPath string, keys []string) error { // get client that's connected to node address := getAddress(node) - client, err := picli.NewInternalClient(address, picli.GetHTTPClient(nil)) + client, err := http.NewInternalClient(address, http.GetHTTPClient(nil)) if err != nil { return err } diff --git a/lru/lru.go b/lru/lru.go index 7f2e6dc22..59f71f69a 100644 --- a/lru/lru.go +++ b/lru/lru.go @@ -82,16 +82,6 @@ func (c *Cache) Get(key Key) (value interface{}, ok bool) { return nil, false } -// remove removes the provided key from the cache. -func (c *Cache) remove(key Key) { // nolint: staticcheck,unused - if c.cache == nil { - return - } - if ele, hit := c.cache[key]; hit { - c.removeElement(ele) - } -} - // removeOldest removes the oldest item from the cache. func (c *Cache) removeOldest() { if c.cache == nil { @@ -119,15 +109,3 @@ func (c *Cache) Len() int { } return c.ll.Len() } - -// clear purges all stored items from the cache. -func (c *Cache) clear() { // nolint: staticcheck,unused - if c.OnEvicted != nil { - for _, e := range c.cache { - kv := e.Value.(*entry) - c.OnEvicted(kv.key, kv.value) - } - } - c.ll = nil - c.cache = nil -} diff --git a/pprof.go b/pprof.go index 6e10c0bb5..ff34a7544 100644 --- a/pprof.go +++ b/pprof.go @@ -24,7 +24,7 @@ import ( _ "net/http/pprof" // Imported for its side-effect of registering pprof endpoints with the server. "github.com/molecula/featurebase/v2/storage" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" ) // CPUProfileForDur (where "Dur" is short for "Duration"), is used for @@ -38,18 +38,18 @@ func CPUProfileForDur(dur time.Duration, outpath string) { } path := outpath + "." + backend f, err := os.Create(path) - PanicOn(err) + vprint.PanicOn(err) if dur == 0 { dur = time.Minute } - AlwaysPrintf("starting cpu profile for dur '%v', output to '%v'", dur, path) + vprint.AlwaysPrintf("starting cpu profile for dur '%v', output to '%v'", dur, path) _ = pprof.StartCPUProfile(f) go func() { <-time.After(dur) pprof.StopCPUProfile() f.Close() - AlwaysPrintf("stopping cpu profile after dur '%v', output: '%v'", dur, path) + vprint.AlwaysPrintf("stopping cpu profile after dur '%v', output: '%v'", dur, path) }() } @@ -64,20 +64,20 @@ func MemProfileForDur(dur time.Duration, outpath string) { } path := outpath + "." + backend f, err := os.Create(path) - PanicOn(err) + vprint.PanicOn(err) if dur == 0 { dur = time.Minute } - AlwaysPrintf("will write memory profile after dur '%v', output to '%v'", dur, path) + vprint.AlwaysPrintf("will write memory profile after dur '%v', output to '%v'", dur, path) go func() { <-time.After(dur) runtime.GC() // get up-to-date statistics if err := pprof.WriteHeapProfile(f); err != nil { - PanicOn(fmt.Sprintf("could not write memory profile: %v", err)) + vprint.PanicOn(fmt.Sprintf("could not write memory profile: %v", err)) } f.Close() - AlwaysPrintf("wrote memory profile after dur '%v', output: '%v'", dur, path) + vprint.AlwaysPrintf("wrote memory profile after dur '%v', output: '%v'", dur, path) }() } @@ -92,7 +92,7 @@ var _ = pprofProfile{} func newPprof() (pp *pprofProfile) { pp = &pprofProfile{} f, err := os.Create("cpu.manual.pprof") - PanicOn(err) + vprint.PanicOn(err) pp.fdCpu = f _ = pprof.StartCPUProfile(pp.fdCpu) @@ -105,11 +105,11 @@ func (pp *pprofProfile) Close() { pp.fdCpu.Close() f, err := os.Create("mem.manual.pprof") - PanicOn(err) + vprint.PanicOn(err) runtime.GC() // get up-to-date statistics if err := pprof.WriteHeapProfile(f); err != nil { - PanicOn(fmt.Sprintf("could not write memory profile: %v", err)) + vprint.PanicOn(fmt.Sprintf("could not write memory profile: %v", err)) } f.Close() } diff --git a/pql/ast.go b/pql/ast.go index dddf72812..4e9dc1f2a 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -586,7 +586,7 @@ func (c *Call) CheckCallInfo() error { case string, int64: continue default: - return fmt.Errorf("'%s': arg '%s' needed a string or integer value, got %T.", + return fmt.Errorf("'%s': arg '%s' needed a string or integer value, got %T", c.String(), k, v) } } diff --git a/rbf.go b/rbf.go index 910ecce78..b75efa331 100644 --- a/rbf.go +++ b/rbf.go @@ -28,7 +28,7 @@ import ( txkey "github.com/molecula/featurebase/v2/short_txkey" "github.com/molecula/featurebase/v2/storage" - . "github.com/molecula/featurebase/v2/vprint" // nolint:staticcheck + "github.com/molecula/featurebase/v2/vprint" "github.com/pkg/errors" ) @@ -411,7 +411,7 @@ func (tx *RBFTx) ImportRoaringBits(index, field, view string, shard uint64, rit func (tx *RBFTx) NewTxIterator(index, field, view string, shard uint64) *roaring.Iterator { b, err := tx.RoaringBitmap(index, field, view, shard) - PanicOn(err) + vprint.PanicOn(err) return b.Iterator() } diff --git a/rbf/rbf.go b/rbf/rbf.go index 2245cf189..8cba59f4d 100644 --- a/rbf/rbf.go +++ b/rbf/rbf.go @@ -30,7 +30,7 @@ import ( "github.com/benbjohnson/immutable" "github.com/molecula/featurebase/v2/roaring" "github.com/molecula/featurebase/v2/shardwidth" - . "github.com/molecula/featurebase/v2/vprint" + "github.com/molecula/featurebase/v2/vprint" ) const ( @@ -356,7 +356,7 @@ func (c *leafCell) Bitmap(tx *Tx) []uint64 { _, bm, _ := tx.leafCellBitmap(toPgno(c.Data)) return bm default: - PanicOn(fmt.Errorf("invalid container type: %d", c.Type)) + vprint.PanicOn(fmt.Errorf("invalid container type: %d", c.Type)) } return nil } @@ -383,7 +383,7 @@ func (c *leafCell) Values(tx *Tx) []uint16 { case ContainerTypeNone: return []uint16{} default: - PanicOn(fmt.Errorf("invalid container type: %d", c.Type)) + vprint.PanicOn(fmt.Errorf("invalid container type: %d", c.Type)) } return nil } @@ -411,7 +411,7 @@ func (c *leafCell) firstValue(tx *Tx) uint16 { return r[0].Start case ContainerTypeBitmapPtr: _, slc, err := tx.leafCellBitmap(toPgno(c.Data)) - PanicOn(err) + vprint.PanicOn(err) for i, v := range slc { for j := uint(0); j < 64; j++ { if v&(1< Date: Fri, 3 Dec 2021 10:56:21 -0600 Subject: [PATCH 15/23] resolved reviewer's suggestions and made it pretty & user friendly --- auth/auth.go | 32 ++--- server/config.go | 42 +++--- server/config_internal_test.go | 226 +++++++++++++++++++++++++++------ server/server.go | 5 - 4 files changed, 221 insertions(+), 84 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 2abfda28f..de7ed303d 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -14,22 +14,22 @@ package auth -type AUTH struct { - ClientId string - ClientSecret string - AuthorizeURL string - TokenURL string - GroupEndpointURL string -} +type Auth struct { + // Enable AuthZ/AuthN for featurebase server + Enable bool `toml:"enable"` -func NewAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) AUTH { - a := AUTH{ - ClientId: ClientId, - ClientSecret: ClientSecret, - AuthorizeURL: AuthorizeURL, - TokenURL: TokenURL, - GroupEndpointURL: GroupEndpointURL, - } + // Application/Client ID + ClientId string `toml:"client-id"` - return a + // Client Secret + ClientSecret string `toml:"client-secret"` + + // Authorize URL + AuthorizeURL string `toml:"authorize-url"` + + // Token URL + TokenURL string `toml:"token-url"` + + // Group Endpoint URL + GroupEndpointURL string `toml:"group-endpoint-url"` } diff --git a/server/config.go b/server/config.go index e93244dcb..79ae314ad 100644 --- a/server/config.go +++ b/server/config.go @@ -25,6 +25,7 @@ import ( "strings" "time" + "github.com/molecula/featurebase/v2/auth" petcd "github.com/molecula/featurebase/v2/etcd" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/storage" @@ -243,25 +244,7 @@ type Config struct { SchemaDetailsOn bool `toml:"schema-details-on"` // Enable AuthZ/AuthN - Auth struct { - // Enable AuthZ/AuthN for featurebase server - Enable bool `toml:"enable"` - - // Application/Client ID - ClientId string `toml:"client-id"` - - // Client Secret - ClientSecret string `toml:"client-secret"` - - // Authorize URL - AuthorizeURL string `toml:"authorize-url"` - - // Token URL - TokenURL string `toml:"token-url"` - - // Group Endpoint URL - GroupEndpointURL string `toml:"group-endpoint-url"` - } `toml:"auth"` + Auth auth.Auth `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -626,7 +609,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin return addrs[0].String(), nil } -func (c *Config) ValidateAuth() error { +func (c *Config) ValidateAuth() ([]error, error) { authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret, @@ -635,23 +618,32 @@ func (c *Config) ValidateAuth() error { "GroupEndpointURL": c.Auth.GroupEndpointURL, } + errors := make([]error, 0) for name, value := range authConfig { if value == "" { - return fmt.Errorf("Empty string for auth config %s", name) + errors = append(errors, fmt.Errorf("Empty string for auth config %s", name)) + continue } if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { - return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) + errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err)) + continue } } } - return nil + if len(errors) > 0 { + return errors, fmt.Errorf("there were errors validating config") + } + return errors, nil } func (c *Config) MustValidateAuth() { - if err := c.ValidateAuth(); err != nil { - panic(err) + if errors, err := c.ValidateAuth(); err != nil { + for _, e := range errors { + log.Println(e) + } + log.Fatal(err) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 5daac4233..927f65327 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -21,6 +21,8 @@ import ( "os" "strings" "testing" + + "github.com/molecula/featurebase/v2/auth" ) type addrs struct{ bind, advertise string } @@ -289,15 +291,6 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } } -type params struct { - enable bool - clientId string - clientSecret string - authorizeURL string - tokenURL string - groupEndpointURL string -} - func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty := "Empty string" errorMesgURL := "Invalid URL" @@ -310,43 +303,200 @@ func TestConfig_validateAuth(t *testing.T) { disable := false tests := []struct { - expErr string - input params + expErrs []string + input auth.Auth }{ - {errorMesgEmpty, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, - {emptyString, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}}, - {errorMesgEmpty, params{disable, emptyString, emptyString, emptyString, emptyString, emptyString}}, + + { + // Auth enabled, all configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, + []string{ + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: notValidURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + }, + }, + { + []string{ + errorMesgURL, + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: notValidURL, + GroupEndpointURL: notValidURL, + }, + }, + { + []string{ + errorMesgEmpty, + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: notValidURL, + }, + }, + { + []string{}, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + }, + }, + { + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: disable, + ClientId: emptyString, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { c := NewConfig() - c.Auth.Enable = test.input.enable - c.Auth.ClientId = test.input.clientId - c.Auth.ClientSecret = test.input.clientSecret - c.Auth.AuthorizeURL = test.input.authorizeURL - c.Auth.TokenURL = test.input.tokenURL - c.Auth.GroupEndpointURL = test.input.groupEndpointURL + c.Auth = test.input - err := c.ValidateAuth() - - if err != nil && test.expErr == "" { - t.Fatal(err) - } else if err == nil && test.expErr != "" { - t.Fatalf("expected error string to contain %s, but got no error", test.expErr) - } else if err != nil && test.expErr != "" { - if !strings.Contains(err.Error(), test.expErr) { - t.Fatalf("expected error string to contain %s, but got %s", test.expErr, err.Error()) + errors, err := c.ValidateAuth() + if len(test.expErrs) > 0 { + if err == nil { + t.Fatal("expected errors, but none were found") + } + } + + if len(errors) != len(test.expErrs) { + fmt.Printf("%+v\n", errors) + t.Fatalf("expected %v errors but got %v", len(test.expErrs), len(errors)) + } + + for i, e := range errors { + if !strings.Contains(e.Error(), test.expErrs[i]) { + t.Errorf("expected error to contain %s, but got %s", test.expErrs[i], e.Error()) } - return } }) } diff --git a/server/server.go b/server/server.go index 768f000ed..943813b07 100644 --- a/server/server.go +++ b/server/server.go @@ -41,7 +41,6 @@ import ( "golang.org/x/sync/errgroup" pilosa "github.com/molecula/featurebase/v2" - "github.com/molecula/featurebase/v2/auth" "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/encoding/proto" petcd "github.com/molecula/featurebase/v2/etcd" @@ -56,7 +55,6 @@ import ( "github.com/molecula/featurebase/v2/statsd" "github.com/molecula/featurebase/v2/syswrap" "github.com/molecula/featurebase/v2/testhook" - "github.com/molecula/featurebase/v2/vprint" "github.com/pelletier/go-toml" "github.com/pkg/errors" ) @@ -238,9 +236,6 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable == true { m.Config.MustValidateAuth() - authArgs := auth.NewAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) - vprint.VV("Auth: %v", authArgs) - // print statement is so that binary compiles, and golang doesn't complaint about declared but unused var } // Initialize server. From 6425fc50fcf1f29224636317cb514ceef2f6024a Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 11:24:18 -0600 Subject: [PATCH 16/23] added identity provider scope url as parameter --- auth/auth.go | 3 + ctl/server.go | 1 + server/config.go | 1 + server/config_internal_test.go | 100 +++++++++++++++++---------------- 4 files changed, 58 insertions(+), 47 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index de7ed303d..4e617c998 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -32,4 +32,7 @@ type Auth struct { // Group Endpoint URL GroupEndpointURL string `toml:"group-endpoint-url"` + + // Scope URL + ScopeURL string `toml:"scope-url"` } diff --git a/ctl/server.go b/ctl/server.go index a368637e3..c5d43a937 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -129,5 +129,6 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.") flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") + flags.StringVar(&srv.Config.Auth.ScopeURL, "auth.scope-url", srv.Config.Auth.ScopeURL, "Identity Provider's Scope URL.") } diff --git a/server/config.go b/server/config.go index 79ae314ad..4d69521a1 100644 --- a/server/config.go +++ b/server/config.go @@ -616,6 +616,7 @@ func (c *Config) ValidateAuth() ([]error, error) { "AuthorizeURL": c.Auth.AuthorizeURL, "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, + "ScopeURL": c.Auth.ScopeURL, } errors := make([]error, 0) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 927f65327..8917d0fc2 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -315,6 +315,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -323,6 +324,45 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: emptyString, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + ScopeURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { @@ -336,27 +376,11 @@ func TestConfig_validateAuth(t *testing.T) { auth.Auth{ Enable: enable, ClientId: validClientID, - ClientSecret: emptyString, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - auth.Auth{ - Enable: enable, - ClientId: emptyString, ClientSecret: validClientSecret, AuthorizeURL: emptyString, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { @@ -366,21 +390,6 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - }, auth.Auth{ Enable: enable, ClientId: validClientID, @@ -388,12 +397,14 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { // Auth enabled, some configs are set to empty string []string{ errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -402,10 +413,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: validTestURL, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { - // Auth enabled, + // Auth enabled, some strings are set to invalid URL []string{ errorMesgURL, }, @@ -416,9 +428,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: notValidURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, }, }, { + // Auth enabled, some strings are set to invalid URL []string{ errorMesgURL, errorMesgURL, @@ -430,23 +444,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: notValidURL, GroupEndpointURL: notValidURL, + ScopeURL: validTestURL, }, }, { - []string{ - errorMesgEmpty, - errorMesgURL, - }, - auth.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: emptyString, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: notValidURL, - }, - }, - { + // Auth enabled, all configs are set properly []string{}, auth.Auth{ Enable: enable, @@ -455,15 +457,18 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, }, }, { + // Auth disabled, all configs are set to empty string []string{ errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: disable, @@ -472,6 +477,7 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: emptyString, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, } From cebba84beeb6c591dae7b04ee435a7764afe34a1 Mon Sep 17 00:00:00 2001 From: souhailanoor <90720110+souhailanoor@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:38:43 -0600 Subject: [PATCH 17/23] add scope to install/featurebase.conf Co-authored-by: Samir Patel <48686912+54mir@users.noreply.github.com> --- install/featurebase.conf | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/install/featurebase.conf b/install/featurebase.conf index 7807134e7..540a410f4 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -379,4 +379,5 @@ log-path = "/var/log/molecula/featurebase.log" # client-secret = "" # authorize-url = "" # token-url = "" -# group-endpoint-url = "" \ No newline at end of file +# group-endpoint-url = "" +# scope-url = "" \ No newline at end of file From e8f54581007e576746ca52c51ea3f09d9a95d94f Mon Sep 17 00:00:00 2001 From: souhailanoor <90720110+souhailanoor@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:58:08 -0600 Subject: [PATCH 18/23] only validate config when auth is enabled Co-authored-by: reese <45641995+reesporte@users.noreply.github.com> --- server/config.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/config.go b/server/config.go index 4d69521a1..6bf2b22c4 100644 --- a/server/config.go +++ b/server/config.go @@ -610,6 +610,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() ([]error, error) { + if !c.Auth.Enable { + return []error{}, nil + } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret, From 90c3c67ce42048c2c345752c5b79668d12ba61d3 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 12:00:07 -0600 Subject: [PATCH 19/23] fixed test for auth disabled --- server/config_internal_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 8917d0fc2..8d5e1fea0 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -462,14 +462,7 @@ func TestConfig_validateAuth(t *testing.T) { }, { // Auth disabled, all configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, + []string{}, auth.Auth{ Enable: disable, ClientId: emptyString, From 7bcbb5eacaa2c510d3d047bf5697f8e94ebb02cc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 12:10:31 -0600 Subject: [PATCH 20/23] fixed indentation --- server/config.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/config.go b/server/config.go index 6bf2b22c4..0c1989c7a 100644 --- a/server/config.go +++ b/server/config.go @@ -610,9 +610,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() ([]error, error) { - if !c.Auth.Enable { - return []error{}, nil - } + if !c.Auth.Enable { + return []error{}, nil + } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret, From c2964f7c7bb9a4231ba6aaa80857181b8e980ab1 Mon Sep 17 00:00:00 2001 From: reesporte Date: Fri, 3 Dec 2021 14:56:18 -0600 Subject: [PATCH 21/23] rename to pilosa thanks to alan's comment [here](https://molecula.atlassian.net/browse/FB-1021?focusedCommentId=11721) --- cmd/restore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/restore.go b/cmd/restore.go index 88605b2ad..24b9a20f6 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -35,8 +35,8 @@ The Restore command will take a backup archive and restore it to a new, clean cl }, } flags := restoreCmd.Flags() - flags.StringVarP(&cmd.Path, "source", "s", "", "pilosa backup file; specify '-' to restore from stdin tar stream") - flags.StringVar(&cmd.Host, "host", "localhost:10101", "host:port of Pilosa.") + flags.StringVarP(&cmd.Path, "source", "s", "", "backup file; specify '-' to restore from stdin tar stream") + flags.StringVar(&cmd.Host, "host", "localhost:10101", "host:port of FeatureBase.") flags.IntVar(&cmd.Concurrency, "concurrency", 1, "number of concurrent uploads") ctl.SetTLSConfig( flags, "", From 58b4f40cdcde6caca98fae50ed9588ab7071234e Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 3 Dec 2021 11:23:43 -0600 Subject: [PATCH 22/23] enable TopK on mutex fields I think it was just an oversight that it wasn't, because this seems to work --- executor.go | 2 +- executor_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/executor.go b/executor.go index 009f55c7b..20af2da4a 100644 --- a/executor.go +++ b/executor.go @@ -2098,7 +2098,7 @@ func (e *executor) executeTopKShard(ctx context.Context, qcx *Qcx, index string, return e.executeTopKShardTime(ctx, tx, filterBitmap, index, fieldName, shard, fromTime, toTime) } fallthrough - case FieldTypeSet: + case FieldTypeSet, FieldTypeMutex: return e.executeTopKShardSet(ctx, tx, filterBitmap, index, fieldName, shard) default: return nil, errors.Errorf("field type %q is not yet supported by TopK", ftype) diff --git a/executor_test.go b/executor_test.go index e4924887e..49590df95 100644 --- a/executor_test.go +++ b/executor_test.go @@ -1719,6 +1719,36 @@ func TestExecutor_Execute_TopK_Set(t *testing.T) { } } +func TestExecutor_Execute_TopK_Mutex(t *testing.T) { + c := test.MustRunCluster(t, 3) + defer c.Close() + + // Load some test data into a mutex field. + c.CreateField(t, "i", pilosa.IndexOptions{TrackExistence: true}, "f", pilosa.OptFieldTypeMutex(pilosa.CacheTypeRanked, 10)) + c.ImportBits(t, "i", "f", [][2]uint64{ + {0, 0}, + {0, ShardWidth + 2}, + {10, 2}, + {10, ShardWidth}, + {10, 2 * ShardWidth}, + {10, ShardWidth + 1}, + {20, ShardWidth}, + }) + + // Execute query. + if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopK(f, k=2)`}); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(result.Results, []interface{}{&pilosa.PairsField{ + Pairs: []pilosa.Pair{ + {ID: 10, Count: 3}, + {ID: 0, Count: 2}, + }, + Field: "f", + }}) { + t.Fatalf("unexpected result: %s", spew.Sdump(result)) + } +} + func TestExecutor_Execute_TopK_Time(t *testing.T) { c := test.MustRunCluster(t, 3) defer c.Close() From bd3e73ba66ab4bf48502e3afbabf4e3b283c924c Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 3 Dec 2021 16:40:23 -0600 Subject: [PATCH 23/23] refactor test to reduce duplication I guess this is actually better... thanks SonarCloud! --- executor_test.go | 93 ++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/executor_test.go b/executor_test.go index 49590df95..ed4ffa735 100644 --- a/executor_test.go +++ b/executor_test.go @@ -1688,64 +1688,63 @@ func TestExecutor_Execute_SetValue(t *testing.T) { } -func TestExecutor_Execute_TopK_Set(t *testing.T) { - c := test.MustRunCluster(t, 3) - defer c.Close() - - // Load some test data into a set field. - c.CreateField(t, "i", pilosa.IndexOptions{TrackExistence: true}, "f") - c.ImportBits(t, "i", "f", [][2]uint64{ +func TestExecutor_ExecuteTopK(t *testing.T) { + baseBits := [][2]uint64{ {0, 0}, - {0, 1}, {0, ShardWidth + 2}, {10, 2}, {10, ShardWidth}, {10, 2 * ShardWidth}, {10, ShardWidth + 1}, {20, ShardWidth}, - }) - - // Execute query. - if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopK(f, k=2)`}); err != nil { - t.Fatal(err) - } else if !reflect.DeepEqual(result.Results, []interface{}{&pilosa.PairsField{ - Pairs: []pilosa.Pair{ - {ID: 10, Count: 4}, - {ID: 0, Count: 3}, - }, - Field: "f", - }}) { - t.Fatalf("unexpected result: %s", spew.Sdump(result)) } -} - -func TestExecutor_Execute_TopK_Mutex(t *testing.T) { + tests := []struct { + fieldName string + fieldOptions []pilosa.FieldOption + bits [][2]uint64 + query string + result []pilosa.Pair + }{ + { + fieldName: "f", + bits: append(baseBits, [2]uint64{0, 1}), + query: "TopK(f, k=2)", + result: []pilosa.Pair{ + {ID: 10, Count: 4}, + {ID: 0, Count: 3}, + }, + }, + { + fieldName: "fmutex", + fieldOptions: []pilosa.FieldOption{pilosa.OptFieldTypeMutex(pilosa.CacheTypeRanked, 10)}, + bits: baseBits, + query: "TopK(f, k=2)", + result: []pilosa.Pair{ + {ID: 10, Count: 3}, + {ID: 0, Count: 2}, + }, + }, + } c := test.MustRunCluster(t, 3) defer c.Close() - // Load some test data into a mutex field. - c.CreateField(t, "i", pilosa.IndexOptions{TrackExistence: true}, "f", pilosa.OptFieldTypeMutex(pilosa.CacheTypeRanked, 10)) - c.ImportBits(t, "i", "f", [][2]uint64{ - {0, 0}, - {0, ShardWidth + 2}, - {10, 2}, - {10, ShardWidth}, - {10, 2 * ShardWidth}, - {10, ShardWidth + 1}, - {20, ShardWidth}, - }) - - // Execute query. - if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopK(f, k=2)`}); err != nil { - t.Fatal(err) - } else if !reflect.DeepEqual(result.Results, []interface{}{&pilosa.PairsField{ - Pairs: []pilosa.Pair{ - {ID: 10, Count: 3}, - {ID: 0, Count: 2}, - }, - Field: "f", - }}) { - t.Fatalf("unexpected result: %s", spew.Sdump(result)) + for _, tst := range tests { + t.Run(tst.fieldName, func(t *testing.T) { + pilosa.OptFieldTypeMutex(pilosa.CacheTypeRanked, 10) + c.CreateField(t, "i", pilosa.IndexOptions{TrackExistence: true}, tst.fieldName) + c.ImportBits(t, "i", tst.fieldName, tst.bits) + if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: tst.query}); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(result.Results, []interface{}{&pilosa.PairsField{ + Pairs: []pilosa.Pair{ + {ID: 10, Count: 4}, + {ID: 0, Count: 3}, + }, + Field: "f", + }}) { + t.Fatalf("unexpected result: %s", spew.Sdump(result)) + } + }) } }