Fix quickstart bug: branch protection (#6682)

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)
+}

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
 	}

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)

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

pkg/graveler/branch/protection_manager.go

@@ -35,38 +35,24 @@ 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
 }
 func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules) error {
 	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

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 (

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)
 }