From d96af68565830ed8ce1a17561952d95fddb7a72a Mon Sep 17 00:00:00 2001 From: Yoni Date: Tue, 3 Oct 2023 03:21:20 +0300 Subject: [PATCH] Fix quickstart bug: branch protection (#6682) --- esti/repository_test.go | 30 ++++ pkg/api/controller.go | 9 +- pkg/catalog/catalog.go | 4 +- pkg/catalog/interface.go | 5 +- pkg/graveler/branch/protection_manager.go | 24 +-- .../branch/protection_manager_test.go | 32 +--- pkg/graveler/graveler.go | 25 +-- pkg/graveler/mock/graveler.go | 24 +-- pkg/graveler/settings/manager.go | 105 +++++------ pkg/graveler/settings/manager_test.go | 170 ++---------------- pkg/kv/local/store.go | 1 - pkg/samplerepo/samplecontent.go | 10 +- webui/src/lib/api/index.js | 21 +-- .../repository/settings/branches.jsx | 23 +-- 14 files changed, 155 insertions(+), 328 deletions(-) diff --git a/esti/repository_test.go b/esti/repository_test.go index 36bed17a03b..0c4440fa3c8 100644 --- a/esti/repository_test.go +++ b/esti/repository_test.go @@ -5,7 +5,11 @@ import ( "net/http" "testing" + "github.com/go-openapi/swag" "github.com/stretchr/testify/require" + "github.com/treeverse/lakefs/pkg/api/apigen" + "github.com/treeverse/lakefs/pkg/api/apiutil" + "github.com/treeverse/lakefs/pkg/logging" ) func TestRepositoryBasicOps(t *testing.T) { @@ -31,3 +35,29 @@ func TestRepositoryBasicOps(t *testing.T) { require.Equal(t, http.StatusNoContent, resp.StatusCode()) } } + +func TestRepositoryCreateSampleRepo(t *testing.T) { + ctx := context.Background() + name := generateUniqueRepositoryName() + storageNamespace := generateUniqueStorageNamespace(name) + name = makeRepositoryName(name) + logger.WithFields(logging.Fields{ + "repository": name, + "storage_namespace": storageNamespace, + "name": name, + }).Debug("Create repository for test") + resp, err := client.CreateRepositoryWithResponse(ctx, &apigen.CreateRepositoryParams{}, apigen.CreateRepositoryJSONRequestBody{ + DefaultBranch: apiutil.Ptr(mainBranch), + Name: name, + StorageNamespace: storageNamespace, + SampleData: swag.Bool(true), + }) + require.NoErrorf(t, err, "failed to create repository '%s', storage '%s'", name, storageNamespace) + require.NoErrorf(t, verifyResponse(resp.HTTPResponse, resp.Body), + "create repository '%s', storage '%s'", name, storageNamespace) + _, err = client.GetRepositoryWithResponse(ctx, name) + require.NoErrorf(t, err, "failed to get repository '%s'", name) + listResp, err := client.ListObjectsWithResponse(ctx, name, mainBranch, &apigen.ListObjectsParams{}) + require.NoErrorf(t, err, "failed to list objects in repository '%s'", name) + require.NotEmptyf(t, listResp.JSON200.Results, "repository '%s' has no objects in main branch", name) +} diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 1edaddbdeb6..971a4d35a49 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -1794,7 +1794,7 @@ func (c *Controller) GetBranchProtectionRules(w http.ResponseWriter, r *http.Req Pattern: pattern, }) } - w.Header().Set("ETag", eTag) + w.Header().Set("ETag", swag.StringValue(eTag)) writeResponse(w, r, http.StatusOK, resp) } @@ -1822,6 +1822,9 @@ func (c *Controller) SetBranchProtectionRules(w http.ResponseWriter, r *http.Req } } eTag := params.IfMatch + if swag.StringValue(eTag) == "" { + eTag = nil + } err := c.Catalog.SetBranchProtectionRules(ctx, repository, rules, eTag) if c.handleAPIError(ctx, w, r, err) { return @@ -3089,7 +3092,7 @@ func (c *Controller) InternalDeleteBranchProtectionRule(w http.ResponseWriter, r for p := range rules.BranchPatternToBlockedActions { if p == body.Pattern { delete(rules.BranchPatternToBlockedActions, p) - err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil) + err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum)) if c.handleAPIError(ctx, w, r, err) { return } @@ -3143,7 +3146,7 @@ func (c *Controller) InternalCreateBranchProtectionRule(w http.ResponseWriter, r Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE, graveler.BranchProtectionBlockedAction_COMMIT}, } rules.BranchPatternToBlockedActions[body.Pattern] = blockedActions - err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, nil) + err = c.Catalog.SetBranchProtectionRules(ctx, repository, rules, swag.String(graveler.BranchProtectionSkipValidationChecksum)) if c.handleAPIError(ctx, w, r, err) { return } diff --git a/pkg/catalog/catalog.go b/pkg/catalog/catalog.go index b268952e8dd..a403bb84e2d 100644 --- a/pkg/catalog/catalog.go +++ b/pkg/catalog/catalog.go @@ -2079,10 +2079,10 @@ func (c *Catalog) SetGarbageCollectionRules(ctx context.Context, repositoryID st return c.Store.SetGarbageCollectionRules(ctx, repository, rules) } -func (c *Catalog) GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, string, error) { +func (c *Catalog) GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, *string, error) { repository, err := c.getRepository(ctx, repositoryID) if err != nil { - return nil, "", err + return nil, nil, err } return c.Store.GetBranchProtectionRules(ctx, repository) diff --git a/pkg/catalog/interface.go b/pkg/catalog/interface.go index 840f8c8a0fd..e067df88d79 100644 --- a/pkg/catalog/interface.go +++ b/pkg/catalog/interface.go @@ -165,10 +165,11 @@ type Interface interface { // GetBranchProtectionRules returns the branch protection rules for the given repository. // The returned checksum represents the current state of the rules, and can be passed to SetBranchProtectionRules for conditional updates. - GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, string, error) + GetBranchProtectionRules(ctx context.Context, repositoryID string) (*graveler.BranchProtectionRules, *string, error) // SetBranchProtectionRules sets the branch protection rules for the given repository. // If lastKnownChecksum doesn't match the current state, the update will fail with ErrPreconditionFailed. - // If lastKnownChecksum is nil, the update will be performed regardless of the current state of the rules. + // If lastKnownChecksum is nil, the update is performed only if no rules exist. + // If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed. SetBranchProtectionRules(ctx context.Context, repositoryID string, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error // SetLinkAddress to validate single use limited in time of a given physical address diff --git a/pkg/graveler/branch/protection_manager.go b/pkg/graveler/branch/protection_manager.go index 04aac4797b7..8a3d8cd6fd0 100644 --- a/pkg/graveler/branch/protection_manager.go +++ b/pkg/graveler/branch/protection_manager.go @@ -35,30 +35,16 @@ func NewProtectionManager(settingManager *settings.Manager) *ProtectionManager { return &ProtectionManager{settingManager: settingManager, matchers: cache.NewCache(matcherCacheSize, matcherCacheExpiry, cache.NewJitterFn(matcherCacheJitter))} } -func (m *ProtectionManager) Delete(ctx context.Context, repository *graveler.RepositoryRecord, branchNamePattern string) error { - return m.settingManager.Update(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}, func(message proto.Message) (proto.Message, error) { - rules := message.(*graveler.BranchProtectionRules) - if rules.BranchPatternToBlockedActions == nil { - rules.BranchPatternToBlockedActions = make(map[string]*graveler.BranchProtectionBlockedActions) - } - if _, ok := rules.BranchPatternToBlockedActions[branchNamePattern]; !ok { - return nil, ErrRuleNotExists - } - delete(rules.BranchPatternToBlockedActions, branchNamePattern) - return rules, nil - }) -} - -func (m *ProtectionManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, string, error) { +func (m *ProtectionManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, *string, error) { rulesMsg, checksum, err := m.settingManager.GetLatest(ctx, repository, ProtectionSettingKey, &graveler.BranchProtectionRules{}) if errors.Is(err, graveler.ErrNotFound) { - return &graveler.BranchProtectionRules{}, "", nil + return &graveler.BranchProtectionRules{}, nil, nil } if err != nil { - return nil, "", err + return nil, nil, err } if proto.Size(rulesMsg) == 0 { - return &graveler.BranchProtectionRules{}, checksum, nil + return &graveler.BranchProtectionRules{}, nil, nil } return rulesMsg.(*graveler.BranchProtectionRules), checksum, nil } @@ -66,7 +52,7 @@ func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.R return m.settingManager.Save(ctx, repository, ProtectionSettingKey, rules) } -func (m *ProtectionManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum string) error { +func (m *ProtectionManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { err := m.settingManager.SaveIf(ctx, repository, ProtectionSettingKey, rules, lastKnownChecksum) if errors.Is(err, kv.ErrPredicateFailed) { return graveler.ErrPreconditionFailed diff --git a/pkg/graveler/branch/protection_manager_test.go b/pkg/graveler/branch/protection_manager_test.go index bbd0f2d8ed0..17ee1bd0017 100644 --- a/pkg/graveler/branch/protection_manager_test.go +++ b/pkg/graveler/branch/protection_manager_test.go @@ -6,6 +6,7 @@ import ( "errors" "testing" + "github.com/go-openapi/swag" "github.com/go-test/deep" "github.com/golang/mock/gomock" "github.com/treeverse/lakefs/pkg/graveler" @@ -62,41 +63,12 @@ func TestSetWrongETag(t *testing.T) { graveler.BranchProtectionBlockedAction_STAGING_WRITE}, }, }, - }, base64.StdEncoding.EncodeToString([]byte("WRONG_ETAG"))) + }, swag.String(base64.StdEncoding.EncodeToString([]byte("WRONG_ETAG")))) if !errors.Is(err, graveler.ErrPreconditionFailed) { t.Fatalf("expected ErrPreconditionFailed, got %v", err) } } -func TestDelete(t *testing.T) { - ctx := context.Background() - bpm := prepareTest(t, ctx) - err := bpm.Delete(ctx, repository, "main*") - if !errors.Is(err, branch.ErrRuleNotExists) { - t.Fatalf("expected ErrRuleNotExists, got %v", err) - } - testutil.Must(t, bpm.SetRules(ctx, repository, &graveler.BranchProtectionRules{ - BranchPatternToBlockedActions: map[string]*graveler.BranchProtectionBlockedActions{ - "main*": {Value: []graveler.BranchProtectionBlockedAction{ - graveler.BranchProtectionBlockedAction_STAGING_WRITE}, - }, - }, - })) - rules, _, err := bpm.GetRules(ctx, repository) - testutil.Must(t, err) - expectedActions := &graveler.BranchProtectionBlockedActions{Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_STAGING_WRITE}} - if diff := deep.Equal(expectedActions, rules.BranchPatternToBlockedActions["main*"]); diff != nil { - t.Fatalf("got unexpected blocked actions. diff=%s", diff) - } - testutil.Must(t, bpm.Delete(ctx, repository, "main*")) - - rules, _, err = bpm.GetRules(ctx, repository) - testutil.Must(t, err) - if len(rules.BranchPatternToBlockedActions) != 0 { - t.Fatalf("expected no rules, got %d rules", len(rules.BranchPatternToBlockedActions)) - } -} - func TestIsBlocked(t *testing.T) { ctx := context.Background() var ( diff --git a/pkg/graveler/graveler.go b/pkg/graveler/graveler.go index a793ca31fc2..abd793bd489 100644 --- a/pkg/graveler/graveler.go +++ b/pkg/graveler/graveler.go @@ -9,6 +9,7 @@ import ( "time" "github.com/cenkalti/backoff/v4" + "github.com/go-openapi/swag" "github.com/google/uuid" "github.com/hashicorp/go-multierror" "github.com/prometheus/client_golang/prometheus" @@ -45,6 +46,8 @@ const ( RepoMetadataUpdateRandomFactor = 0.5 ) +const BranchProtectionSkipValidationChecksum = "skip-checksum-validation" + // Basic Types // DiffType represents the type of the change @@ -471,7 +474,7 @@ type KeyValueStore interface { // List lists values on repository / ref List(ctx context.Context, repository *RepositoryRecord, ref Ref, batchSize int) (ValueIterator, error) - // ListStaging returns ValueIterator for branch staging area. Exposed to be used by catalog in PrepareGCUncommitted + // ListStaging returns ValueIterator for branch staging area. Exposed to be used by X in PrepareGCUncommitted ListStaging(ctx context.Context, branch *Branch, batchSize int) (ValueIterator, error) } @@ -611,11 +614,12 @@ type VersionController interface { // GetBranchProtectionRules return all branch protection rules for the repository. // The returned checksum represents the current state of the rules, and can be passed to SetBranchProtectionRules for a conditional update. - GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, string, error) + GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, *string, error) // SetBranchProtectionRules sets the branch protection rules for the repository. // If lastKnownChecksum doesn't match the current state, the update fails with ErrPreconditionFailed. - // If lastKnownChecksum is nil, the update is always performed. + // If lastKnownChecksum is nil, the update is performed only if no rules exist. + // If lastKnownChecksum is equal to BranchProtectionSkipValidationChecksum, the update is always performed. SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error // SetLinkAddress stores the address token under the repository. The token will be valid for addressTokenTime. @@ -1493,15 +1497,16 @@ func (g *Graveler) GCNewRunID() string { return g.garbageCollectionManager.NewID() } -func (g *Graveler) GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, string, error) { +func (g *Graveler) GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, *string, error) { return g.protectedBranchesManager.GetRules(ctx, repository) } func (g *Graveler) SetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error { - if lastKnownChecksum == nil { + if swag.StringValue(lastKnownChecksum) == BranchProtectionSkipValidationChecksum { + // TODO(johnnyaug): remove this logic and constant once the legacy API is dropped. return g.protectedBranchesManager.SetRules(ctx, repository, rules) } - return g.protectedBranchesManager.SetRulesIf(ctx, repository, rules, *lastKnownChecksum) + return g.protectedBranchesManager.SetRulesIf(ctx, repository, rules, lastKnownChecksum) } // getFromStagingArea returns the most updated value of a given key in a branch staging area. @@ -3235,16 +3240,14 @@ type GarbageCollectionManager interface { } type ProtectedBranchesManager interface { - // Delete deletes the rule for the given name pattern, or returns ErrRuleNotExists if there is no such rule. - Delete(ctx context.Context, repository *RepositoryRecord, branchNamePattern string) error // GetRules returns all branch protection rules for the repository. // The returned checksum represents the current state of the rules, and can be passed to SetRulesIf for conditional updates. - GetRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, string, error) + GetRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, *string, error) SetRules(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules) error // SetRulesIf sets the branch protection rules for the repository. // If lastKnownChecksum does not match the current checksum, returns ErrPreconditionFailed. - // If lastKnownChecksum is empty, the rules are set only if no rules are currently set. - SetRulesIf(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum string) error + // If lastKnownChecksum is nil, the rules are set only if no rules are currently set. + SetRulesIf(ctx context.Context, repository *RepositoryRecord, rules *BranchProtectionRules, lastKnownChecksum *string) error // IsBlocked returns whether the action is blocked by any branch protection rule matching the given branch. IsBlocked(ctx context.Context, repository *RepositoryRecord, branchID BranchID, action BranchProtectionBlockedAction) (bool, error) } diff --git a/pkg/graveler/mock/graveler.go b/pkg/graveler/mock/graveler.go index b62a4010e60..3d8d514001b 100644 --- a/pkg/graveler/mock/graveler.go +++ b/pkg/graveler/mock/graveler.go @@ -481,11 +481,11 @@ func (mr *MockVersionControllerMockRecorder) GetBranch(ctx, repository, branchID } // GetBranchProtectionRules mocks base method. -func (m *MockVersionController) GetBranchProtectionRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, string, error) { +func (m *MockVersionController) GetBranchProtectionRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, *string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetBranchProtectionRules", ctx, repository) ret0, _ := ret[0].(*graveler.BranchProtectionRules) - ret1, _ := ret[1].(string) + ret1, _ := ret[1].(*string) ret2, _ := ret[2].(error) return ret0, ret1, ret2 } @@ -2870,26 +2870,12 @@ func (m *MockProtectedBranchesManager) EXPECT() *MockProtectedBranchesManagerMoc return m.recorder } -// Delete mocks base method. -func (m *MockProtectedBranchesManager) Delete(ctx context.Context, repository *graveler.RepositoryRecord, branchNamePattern string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", ctx, repository, branchNamePattern) - ret0, _ := ret[0].(error) - return ret0 -} - -// Delete indicates an expected call of Delete. -func (mr *MockProtectedBranchesManagerMockRecorder) Delete(ctx, repository, branchNamePattern interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockProtectedBranchesManager)(nil).Delete), ctx, repository, branchNamePattern) -} - // GetRules mocks base method. -func (m *MockProtectedBranchesManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, string, error) { +func (m *MockProtectedBranchesManager) GetRules(ctx context.Context, repository *graveler.RepositoryRecord) (*graveler.BranchProtectionRules, *string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetRules", ctx, repository) ret0, _ := ret[0].(*graveler.BranchProtectionRules) - ret1, _ := ret[1].(string) + ret1, _ := ret[1].(*string) ret2, _ := ret[2].(error) return ret0, ret1, ret2 } @@ -2930,7 +2916,7 @@ func (mr *MockProtectedBranchesManagerMockRecorder) SetRules(ctx, repository, ru } // SetRulesIf mocks base method. -func (m *MockProtectedBranchesManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum string) error { +func (m *MockProtectedBranchesManager) SetRulesIf(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, lastKnownChecksum *string) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "SetRulesIf", ctx, repository, rules, lastKnownChecksum) ret0, _ := ret[0].(error) diff --git a/pkg/graveler/settings/manager.go b/pkg/graveler/settings/manager.go index 0c91b97d861..40ab46bc2cb 100644 --- a/pkg/graveler/settings/manager.go +++ b/pkg/graveler/settings/manager.go @@ -2,12 +2,12 @@ package settings import ( "context" - "encoding/base64" + "crypto/sha256" + "encoding/hex" "errors" - "fmt" "time" - "github.com/cenkalti/backoff/v4" + "github.com/go-openapi/swag" "github.com/treeverse/lakefs/pkg/cache" "github.com/treeverse/lakefs/pkg/graveler" "github.com/treeverse/lakefs/pkg/kv" @@ -26,8 +26,6 @@ type cacheKey struct { Key string } -type updateFunc func(proto.Message) (proto.Message, error) - // Manager is a key-value store for Graveler repository-level settings. // Each setting is stored under a key, and can be any proto.Message. // Fetched settings are cached using cache.Cache with a default expiry time of 1 second. Hence, the store is eventually consistent. @@ -70,39 +68,64 @@ func (m *Manager) Save(ctx context.Context, repository *graveler.RepositoryRecor // SaveIf persists the given setting under the given repository and key. Overrides settings key in KV Store. // The setting is persisted only if the current version of the setting matches the given checksum. -func (m *Manager) SaveIf(ctx context.Context, repository *graveler.RepositoryRecord, key string, setting proto.Message, lastKnownChecksum string) error { +// If lastKnownChecksum is nil, the setting is persisted only if it does not exist. +func (m *Manager) SaveIf(ctx context.Context, repository *graveler.RepositoryRecord, key string, setting proto.Message, lastKnownChecksum *string) error { logSetting(logging.FromContext(ctx), repository.RepositoryID, key, setting, "saving repository-level setting") - decodedChecksum, err := base64.StdEncoding.DecodeString(lastKnownChecksum) - if err != nil { - return fmt.Errorf("decode checksum: %w", err) + valueWithPredicate, err := m.store.Get(ctx, []byte(graveler.RepoPartition(repository)), []byte(graveler.SettingsPath(key))) + if err != nil && !errors.Is(err, kv.ErrNotFound) { + return err + } + var currentChecksum *string + var currentPredicate kv.Predicate + if valueWithPredicate != nil { + if valueWithPredicate.Value != nil { + currentChecksum, err = computeChecksum(valueWithPredicate.Value) + } + if err != nil { + return err + } + currentPredicate = valueWithPredicate.Predicate + } + if swag.StringValue(currentChecksum) != swag.StringValue(lastKnownChecksum) { + return graveler.ErrPreconditionFailed + } + err = kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, currentPredicate) + if err != nil && errors.Is(err, kv.ErrPredicateFailed) { + return graveler.ErrPreconditionFailed } - return kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting, decodedChecksum) + return err } -func (m *Manager) getWithPredicate(ctx context.Context, repo *graveler.RepositoryRecord, key string, data proto.Message) (kv.Predicate, error) { - pred, err := kv.GetMsg(ctx, m.store, graveler.RepoPartition(repo), []byte(graveler.SettingsPath(key)), data) +func computeChecksum(value []byte) (*string, error) { + h := sha256.New() + _, err := h.Write(value) if err != nil { - if errors.Is(err, kv.ErrNotFound) { - err = graveler.ErrNotFound - } return nil, err } - return pred, nil + return swag.String(hex.EncodeToString(h.Sum(nil))), nil } // GetLatest returns the latest setting under the given repository and key, without using the cache. // The returned checksum represents the version of the setting, and can be passed to SaveIf for conditional updates. -func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, string, error) { - data := settingTemplate.ProtoReflect().Interface() - pred, err := m.getWithPredicate(ctx, repository, key, data) +func (m *Manager) GetLatest(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message) (proto.Message, *string, error) { + settings, err := m.store.Get(ctx, []byte(graveler.RepoPartition(repository)), []byte(graveler.SettingsPath(key))) if err != nil { if errors.Is(err, kv.ErrNotFound) { err = graveler.ErrNotFound } - return nil, "", err + return nil, nil, err + } + checksum, err := computeChecksum(settings.Value) + if err != nil { + return nil, nil, err + } + data := settingTemplate.ProtoReflect().Interface() + err = proto.Unmarshal(settings.Value, data) + if err != nil { + return nil, nil, err } logSetting(logging.FromContext(ctx), repository.RepositoryID, key, data, "got repository-level setting") - return data, base64.StdEncoding.EncodeToString(pred.([]byte)), nil + return data, checksum, nil } // Get fetches the setting under the given repository and key, and returns the result. @@ -130,46 +153,6 @@ func (m *Manager) Get(ctx context.Context, repository *graveler.RepositoryRecord return setting.(proto.Message), nil } -// Update atomically gets a setting, performs the update function, and persists the setting to the store. -// The settingTemplate parameter is used to determine the type passed to the update function. -func (m *Manager) Update(ctx context.Context, repository *graveler.RepositoryRecord, key string, settingTemplate proto.Message, update updateFunc) error { - const ( - maxIntervalSec = 2 - maxElapsedSec = 5 - ) - bo := backoff.NewExponentialBackOff() - bo.MaxInterval = maxIntervalSec * time.Second - bo.MaxElapsedTime = maxElapsedSec * time.Second - - err := backoff.Retry(func() error { - data := settingTemplate.ProtoReflect().Interface() - pred, err := m.getWithPredicate(ctx, repository, key, data) - if errors.Is(err, graveler.ErrNotFound) { - data = proto.Clone(settingTemplate) - } else if err != nil { - return backoff.Permanent(err) - } - - logSetting(logging.FromContext(ctx), repository.RepositoryID, key, data, "update repository-level setting") - newData, err := update(data) - if err != nil { - return backoff.Permanent(err) - } - err = kv.SetMsgIf(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), newData, pred) - if errors.Is(err, kv.ErrPredicateFailed) { - logging.FromContext(ctx).WithError(err).Warn("Predicate failed on settings update. Retrying") - return graveler.ErrPreconditionFailed - } else if err != nil { - return backoff.Permanent(err) - } - return nil - }, bo) - if errors.Is(err, graveler.ErrPreconditionFailed) { - return fmt.Errorf("update settings: %w", graveler.ErrTooManyTries) - } - return err -} - func logSetting(logger logging.Logger, repositoryID graveler.RepositoryID, key string, setting proto.Message, logMsg string) { if logger.IsTracing() { logger. diff --git a/pkg/graveler/settings/manager_test.go b/pkg/graveler/settings/manager_test.go index dfe43fac9e1..87a96fcd0ad 100644 --- a/pkg/graveler/settings/manager_test.go +++ b/pkg/graveler/settings/manager_test.go @@ -3,13 +3,11 @@ package settings_test import ( "context" "errors" - "sync" "testing" + "github.com/go-openapi/swag" "github.com/go-test/deep" "github.com/golang/mock/gomock" - "github.com/hashicorp/go-multierror" - "github.com/stretchr/testify/require" "github.com/treeverse/lakefs/pkg/block" "github.com/treeverse/lakefs/pkg/block/mem" "github.com/treeverse/lakefs/pkg/cache" @@ -18,7 +16,6 @@ import ( "github.com/treeverse/lakefs/pkg/graveler/settings" "github.com/treeverse/lakefs/pkg/kv/kvtest" "github.com/treeverse/lakefs/pkg/testutil" - "google.golang.org/protobuf/proto" ) type mockCache struct { @@ -81,176 +78,43 @@ func TestGetLatest(t *testing.T) { ctx := context.Background() m, _ := prepareTest(t, ctx, nil, nil) emptySettings := &settings.ExampleSettings{} - setting, eTag, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) + setting, _, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) if !errors.Is(err, graveler.ErrNotFound) { t.Fatalf("expected ErrNotFound, got %v", err) } err = m.Save(ctx, repository, "settingKey", &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}}) testutil.Must(t, err) - setting, eTag, err = m.GetLatest(ctx, repository, "settingKey", emptySettings) + setting, eTag, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) testutil.Must(t, err) if diff := deep.Equal(&settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}}, setting); diff != nil { t.Fatal("got unexpected settings:", diff) } - if eTag == "" { + if eTag == nil || *eTag == "" { t.Fatal("got empty eTag") } } -func TestUpdate(t *testing.T) { +func TestSaveIf(t *testing.T) { ctx := context.Background() - m, _ := prepareTest(t, ctx, nil, nil) - initialData := &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}} - testutil.Must(t, m.Save(ctx, repository, "settingKey", initialData)) - - validationData := &settings.ExampleSettings{ - ExampleInt: initialData.ExampleInt + 1, - ExampleStr: "goodbye", - ExampleMap: map[string]int32{"boo": initialData.ExampleMap["boo"] + 1}, - } - update := func(settingsToEdit proto.Message) (proto.Message, error) { - newSettings := settings.ExampleSettings{ExampleMap: map[string]int32{}} - newSettings.ExampleStr = validationData.ExampleStr - newSettings.ExampleInt = validationData.ExampleInt - newSettings.ExampleMap["boo"] = validationData.ExampleMap["boo"] - return &newSettings, nil - } - emptySettings := &settings.ExampleSettings{} - require.NoError(t, m.Update(ctx, repository, "settingKey", emptySettings, update)) - gotSettings, err := m.Get(ctx, repository, "settingKey", emptySettings) - - require.NoError(t, err) - if diff := deep.Equal(validationData, gotSettings); diff != nil { - t.Fatal("got unexpected settings:", diff) - } - - badData := &settings.ExampleSettings{ - ExampleInt: initialData.ExampleInt + 1, - ExampleStr: "bad", - ExampleMap: map[string]int32{"boo": -1}, - } - - // Failed update attempt with retry - update = func(settingsToEdit proto.Message) (proto.Message, error) { - newSettings := settings.ExampleSettings{ExampleMap: map[string]int32{}} - - newSettings.ExampleStr = initialData.ExampleStr - newSettings.ExampleInt = initialData.ExampleInt - newSettings.ExampleMap["boo"] = initialData.ExampleMap["boo"] - require.NoError(t, m.Save(ctx, repository, "settingKey", badData)) - return &newSettings, nil - } - require.NoError(t, m.Update(ctx, repository, "settingKey", emptySettings, update)) - gotSettings, err = m.Get(ctx, repository, "settingKey", emptySettings) - require.NoError(t, err) - if diff := deep.Equal(initialData, gotSettings); diff != nil { - t.Fatal("got unexpected settings:", diff) - } - - // Failed update attempt with retry exhausted - update = func(settingsToEdit proto.Message) (proto.Message, error) { - newSettings := settings.ExampleSettings{ExampleMap: map[string]int32{}} - - validationData.ExampleInt++ - require.NoError(t, m.Save(ctx, repository, "settingKey", validationData)) - return &newSettings, nil - } - require.ErrorIs(t, m.Update(ctx, repository, "settingKey", emptySettings, update), graveler.ErrTooManyTries) - gotSettings, eTag3, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) - require.NoError(t, err) - if diff := deep.Equal(validationData, gotSettings); diff != nil { - t.Fatal("got unexpected settings:", diff) - } - - // Failed update attempt with unknown error - testErr := errors.New("test error") - update = func(settingsToEdit proto.Message) (proto.Message, error) { - return nil, testErr - } - require.ErrorIs(t, m.Update(ctx, repository, "settingKey", emptySettings, update), testErr) - gotSettings, eTag4, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) - if eTag4 != eTag3 { - t.Fatal("expected eTag4 to be equal to eTag3") - } - require.NoError(t, err) - if diff := deep.Equal(validationData, gotSettings); diff != nil { - t.Fatal("got unexpected settings:", diff) + mc := &mockCache{ + c: make(map[interface{}]interface{}), } -} - -// TODO: locking is irrelevant for KV, create a new test for it -// Relevant only to DB implementation since KV is lockless -func TestMultipleUpdates(t *testing.T) { - ctx := context.Background() - const IncrementCount = 20 - var lockStartWaitGroup sync.WaitGroup - var lock sync.Mutex - lockStartWaitGroup.Add(IncrementCount) - - m, _ := prepareTest(t, ctx, nil, func(_ context.Context, _ *graveler.RepositoryRecord, _ graveler.BranchID, f func() (interface{}, error)) (interface{}, error) { - lockStartWaitGroup.Done() - lockStartWaitGroup.Wait() // wait until all goroutines ask for the lock - lock.Lock() - retVal, err := f() - lock.Unlock() - return retVal, err - }) + m, _ := prepareTest(t, ctx, mc, nil) emptySettings := &settings.ExampleSettings{} - expectedSettings := &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}} - err := m.Save(ctx, repository, "settingKey", expectedSettings) - testutil.Must(t, err) - update := func(settingsToEdit proto.Message) (proto.Message, error) { - newSettings := settings.ExampleSettings{ExampleMap: map[string]int32{}} - - newSettings.ExampleStr = settingsToEdit.(*settings.ExampleSettings).ExampleStr - newSettings.ExampleInt = settingsToEdit.(*settings.ExampleSettings).ExampleInt + 1 - newSettings.ExampleMap["boo"] = settingsToEdit.(*settings.ExampleSettings).ExampleMap["boo"] + 1 - return &newSettings, nil - } - var wg multierror.Group - for i := 0; i < IncrementCount; i++ { - wg.Go(func() error { - return m.Update(ctx, repository, "settingKey", &settings.ExampleSettings{}, update) - }) - } - err = wg.Wait().ErrorOrNil() + firstSettings := &settings.ExampleSettings{ExampleInt: 5, ExampleStr: "hello", ExampleMap: map[string]int32{"boo": 6}} + err := m.SaveIf(ctx, repository, "settingKey", firstSettings, nil) testutil.Must(t, err) - gotSettings, err := m.Get(ctx, repository, "settingKey", emptySettings) + gotSettings, checksum, err := m.GetLatest(ctx, repository, "settingKey", emptySettings) testutil.Must(t, err) - expectedSettings.ExampleInt += IncrementCount - expectedSettings.ExampleMap["boo"] += IncrementCount - if diff := deep.Equal(expectedSettings, gotSettings); diff != nil { + if diff := deep.Equal(firstSettings, gotSettings); diff != nil { t.Fatal("got unexpected settings:", diff) } -} - -// TestEmpty tests the setting store for keys which have not been set. -func TestEmpty(t *testing.T) { - ctx := context.Background() - m, _ := prepareTest(t, ctx, nil, nil) - emptySettings := &settings.ExampleSettings{} - _, err := m.Get(ctx, repository, "settingKey", emptySettings) - // the key was not set, an error should be returned - if !errors.Is(err, graveler.ErrNotFound) { - t.Fatalf("expected error %v, got %v", graveler.ErrNotFound, err) - } - // when using Update on an unset key, the update function gets an empty setting object to operate on - err = m.Update(ctx, repository, "settingKey", emptySettings, func(setting proto.Message) (proto.Message, error) { - newSettings := proto.Clone(setting).(*settings.ExampleSettings) - - if newSettings.ExampleMap == nil { - newSettings.ExampleMap = make(map[string]int32) - } - newSettings.ExampleInt++ - newSettings.ExampleMap["boo"]++ - return newSettings, nil - }) - testutil.Must(t, err) - gotSettings, err := m.Get(ctx, repository, "settingKey", emptySettings) + secondSettings := &settings.ExampleSettings{ExampleInt: 15, ExampleStr: "hi", ExampleMap: map[string]int32{"boo": 16}} + err = m.SaveIf(ctx, repository, "settingKey", secondSettings, checksum) testutil.Must(t, err) - expectedSettings := &settings.ExampleSettings{ExampleInt: 1, ExampleMap: map[string]int32{"boo": 1}} - if diff := deep.Equal(expectedSettings, gotSettings); diff != nil { - t.Fatal("got unexpected settings:", diff) + err = m.SaveIf(ctx, repository, "settingKey", secondSettings, swag.String("WRONG_CHECKSUM")) + if !errors.Is(err, graveler.ErrPreconditionFailed) { + t.Fatalf("expected ErrPreconditionFailed, got %v", err) } } diff --git a/pkg/kv/local/store.go b/pkg/kv/local/store.go index 313c32a4169..bc9292afeb3 100644 --- a/pkg/kv/local/store.go +++ b/pkg/kv/local/store.go @@ -69,7 +69,6 @@ func (s *Store) Get(ctx context.Context, partitionKey, key []byte) (*kv.ValueWit if err != nil { return nil, err } - return &kv.ValueWithPredicate{ Value: value, Predicate: kv.Predicate(value), diff --git a/pkg/samplerepo/samplecontent.go b/pkg/samplerepo/samplecontent.go index a8deb4d8aae..314507df93c 100644 --- a/pkg/samplerepo/samplecontent.go +++ b/pkg/samplerepo/samplecontent.go @@ -119,12 +119,18 @@ func PopulateSampleRepo(ctx context.Context, repo *catalog.Repository, cat catal func AddBranchProtection(ctx context.Context, repo *catalog.Repository, cat catalog.Interface) error { // Set branch protection on the main branch - rules, eTag, err := cat.GetBranchProtectionRules(ctx, repo.Name) + rules, checksum, err := cat.GetBranchProtectionRules(ctx, repo.Name) if err != nil { return err } + if rules == nil { + rules = &graveler.BranchProtectionRules{} + } + if rules.BranchPatternToBlockedActions == nil { + rules.BranchPatternToBlockedActions = make(map[string]*graveler.BranchProtectionBlockedActions) + } rules.BranchPatternToBlockedActions[repo.DefaultBranch] = &graveler.BranchProtectionBlockedActions{ Value: []graveler.BranchProtectionBlockedAction{graveler.BranchProtectionBlockedAction_COMMIT}, } - return cat.SetBranchProtectionRules(ctx, repo.Name, rules, swag.String(eTag)) + return cat.SetBranchProtectionRules(ctx, repo.Name, rules, checksum) } diff --git a/webui/src/lib/api/index.js b/webui/src/lib/api/index.js index 8324970e995..6365239fe02 100644 --- a/webui/src/lib/api/index.js +++ b/webui/src/lib/api/index.js @@ -1003,28 +1003,19 @@ class BranchProtectionRules { } async setRules(repoID, rules, lastKnownChecksum) { + let additionalHeaders = {} + if (lastKnownChecksum) { + console.log(`setting branch protection rules with checksum ${lastKnownChecksum}`) + additionalHeaders['If-Match'] = lastKnownChecksum + } const response = await apiRequest(`/repositories/${encodeURIComponent(repoID)}/settings/branch_protection`, { method: 'PUT', body: JSON.stringify(rules), - }, {'If-Match': lastKnownChecksum}); + }, additionalHeaders); if (response.status !== 204) { throw new Error(`could not create protection rule: ${await extractError(response)}`); } } - - async deleteRule(repoID, pattern) { - const response = await apiRequest(`/repositories/${encodeURIComponent(repoID)}/branch_protection`, { - method: 'DELETE', - body: JSON.stringify({pattern: pattern}) - }); - if (response.status === 404) { - throw new NotFoundError('branch protection rule not found') - } - if (response.status !== 204) { - throw new Error(`could not delete protection rule: ${await extractError(response)}`); - } - } - } class Statistics { diff --git a/webui/src/pages/repositories/repository/settings/branches.jsx b/webui/src/pages/repositories/repository/settings/branches.jsx index fa7e7c0ba2c..5f93e841c54 100644 --- a/webui/src/pages/repositories/repository/settings/branches.jsx +++ b/webui/src/pages/repositories/repository/settings/branches.jsx @@ -22,6 +22,18 @@ const SettingsContainer = () => { const {response: rulesResponse, error: rulesError, loading: rulesLoading} = useAPI(async () => { return branchProtectionRules.getRules(repo.id) }, [repo, refresh]) + const deleteRule = (pattern) => { + let updatedRules = [...rulesResponse['rules']] + let lastKnownChecksum = rulesResponse['checksum'] + updatedRules = updatedRules.filter(r => r.pattern !== pattern) + branchProtectionRules.setRules(repo.id, updatedRules, lastKnownChecksum).then(() => { + setRefresh(!refresh) + setDeleteButtonDisabled(false) + }).catch(err => { + setDeleteButtonDisabled(false) + setActionError(err) + }) + } if (error) return ; if (rulesError) return ; if (actionError) return ; @@ -51,16 +63,7 @@ const SettingsContainer = () => { return
{r.pattern} - +
}) : There aren't any rules yet.}