Validate new repo isn't using existing storage namespace #3104

itaiad200 · 2022-03-23T11:34:25Z

eden-ohana

LGTM

.github/workflows/nessie.yaml

arielshaqed

Not sure how this should work. What if I type this sequence of commands?

lakectl repo create lakefs://foo s3://bucket/quux & lakectl repo create lakefs://bar s3://bucket/quux
# Both might succeed, the empty commit only occurs after `ensureStorageNamespace`.
lakectl fs upload -s file lakefs://foo/main/file
lakectl fs upload -s file lakefs://boo/main/file
# Now I have two nonempty repos!

arielshaqed · 2022-03-23T15:58:53Z

nessie/repository_test.go

@@ -24,10 +25,39 @@ func TestRepositoryBasicOps(t *testing.T) {
 		require.Contains(t, listedRepos, repo, "repository missing in listing")
 	}

+	// get repo and try creating another repo with the same storage namespace
+	testDupStorageNamespace(t, repos[0])


Can't this be a unit test?

Deleted and added a UT.

arielshaqed · 2022-03-23T15:59:49Z

nessie/repository_test.go

+	respUpload, err := uploadContent(ctx, repo, respGetRepo.JSON200.DefaultBranch, "path/to/file", "bla-bla")
+	require.NoError(t, err, "failed to upload file")
+	require.Equal(t, http.StatusCreated, respUpload.StatusCode())
+
+	respCommit, err := client.CommitWithResponse(ctx, repo, respGetRepo.JSON200.DefaultBranch, api.CommitJSONRequestBody{
+		Message: "nessie:repo test",
+	})
+	require.NoError(t, err, "failed to commit")
+	require.Equal(t, http.StatusCreated, respCommit.StatusCode())


Are these commits needed here? I'd expect this feature to make it impossible to create the same repo twice, even if it has no commits. Maybe the "dummy" file would tip us off.

arielshaqed · 2022-03-23T16:07:35Z

pkg/api/controller.go

+		return err
+	}
+
+	// if a single file exists under the lakeFS prefix in the storage namespace


AFAICS this allows multiple repositories to use the same storage namespace, as long as I don't use them first. (So it is also a race if I create both repos and then upload a file to each.)

Why not just check for existence of the "dummy" file? (It would still be a race, but at least it would prevent this case.)

Thinking about it, one safe way to do this without a full database would be to keep a Graveler object with all storage namespaces. Then you could verify-nonoverlap-and-add, and use a commit-like operation to prevent races. Maybe you could have a hidden "global lakeFS" repo and keep a file named for each storage namespace.

Anyway, the current code is racy -- and since the time between creating a repo and using it is unbounded, there is no protection against this race.

+1 for using the dummy file.
I think we can live with the race on the creation of the dummy file- but if we really want to solve it we can implement a conditional put in the block adapter (like If-None-Match in S3).

Note that using a "global lakeFS" repository will not solve the race for multiple lakeFS installations (to me it's not really a problem but just noting).

Yeah, I missed Oz remarks on the requirements in #3077 - we need to tighten it some more.
The requirements states cross-installations enforcement, so a graveler object won't do the trick here.

I avoided checking the dummy object because I feel less comfortable treating it as a contract between repositories. It's there because of a one-time check, no one ever checks it the second time, and it may very well be deleted without us knowing (GC, lifecycle policies, etc..). But I seem to be the only one who believes it's fragile, so I'll do this check..

For example, now that we delete a repo we also need to delete that dummy file.

Good catch. And also the configurations under _lakefs :(

Or we decide we leave it to the user.

arielshaqed

Thanks!

This is cool and certainly (much much) better than the existing, but still keeps the race condition. And it can happen, e.g. if automation sets up the two repos.

Obviously loads better than the old way of doing nothing, but still dangerous. Perhaps we will be able to do better with the refstore?

(Note that no matter what you do there is danger! E.g. I can set up two distinct lakeFS installations and share storage namespaces between them. There will always be danger, unless we keep our own directory listings for each lakeFS repo and avoid re-using filenames...)

arielshaqed · 2022-03-24T15:38:36Z

pkg/api/controller.go

 	if err != nil {
+		msg := "Could not access storage namespace"


It would be better to keep this as a field rather than as a message. Otherwise it gets harder to grep for the error message when it pops up in the log, and harder to understand what it means. Maybe log "Bad storage namespace" with a field "reason" that is either "bad url" or "invalid" or "already in user" or "access failed"? Or even drop it entirely, rephrase as a constant message "Bad storage namespace", and the error already says what's wrong.

arielshaqed · 2022-03-27T06:52:25Z

pkg/api/controller.go

@@ -1238,19 +1242,28 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo
 	writeResponse(w, http.StatusCreated, response)
 }

-func ensureStorageNamespace(ctx context.Context, adapter block.Adapter, storageNamespace string) error {
+var errorStorageNamespaceInUse = errors.New("lakeFS repositories can't share storage namespace")


Suggested change

var errorStorageNamespaceInUse = errors.New("lakeFS repositories can't share storage namespace")

var errStorageNamespaceInUse = errors.New("lakeFS repositories can't share storage namespace")

arielshaqed · 2022-03-27T06:54:17Z

pkg/api/controller.go

-	err := adapter.Put(ctx, obj, objLen, strings.NewReader(dummyData), block.PutOpts{})
-	if err != nil {
+	if _, err := c.BlockAdapter.Get(ctx, obj, objLen); err == nil {
+		return fmt.Errorf("found lakeFS objects in the storage namespace(%s): %w",


Suggested change

return fmt.Errorf("found lakeFS objects in the storage namespace(%s): %w",

return fmt.Errorf("%s: %w",

(and maybe rephrase the error itself if that doesn't look nice enough).

Would rather keep it as is. The first sentence makes it clearer IMO.

itaiad200 requested review from arielshaqed and eden-ohana March 23, 2022 11:34

itaiad200 added area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Mar 23, 2022

itaiad200 added the team/versioning-engine Team versioning engine label Mar 23, 2022

eden-ohana approved these changes Mar 23, 2022

View reviewed changes

.github/workflows/nessie.yaml Show resolved Hide resolved

arielshaqed requested changes Mar 23, 2022

View reviewed changes

itaiad200 added 3 commits March 24, 2022 15:14

validate new repo isn't overriding existing one

513cd0c

Remove unnecessary copied objects

b295fed

Use just the dummy file

4553cb5

itaiad200 force-pushed the 3077-dup-repo-creation branch from 5bb9fad to 4553cb5 Compare March 24, 2022 14:13

undo nessie test

f3b4e45

itaiad200 requested review from arielshaqed and johnnyaug March 24, 2022 14:16

arielshaqed approved these changes Mar 27, 2022

View reviewed changes

Fix tests

6ec9066

itaiad200 changed the title ~~Validate new repo isn't overriding existing one~~ Validate new repo isn't using existing storage namesapce Mar 27, 2022

itaiad200 force-pushed the 3077-dup-repo-creation branch 2 times, most recently from 56a1f7d to 160f0f7 Compare March 27, 2022 11:53

fix more tests

fe9068d

itaiad200 force-pushed the 3077-dup-repo-creation branch from 160f0f7 to fe9068d Compare March 27, 2022 12:17

itaiad200 changed the title ~~Validate new repo isn't using existing storage namesapce~~ Validate new repo isn't using existing storage namespace Mar 27, 2022

itaiad200 merged commit 30ef49e into master Mar 27, 2022

itaiad200 deleted the 3077-dup-repo-creation branch March 27, 2022 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate new repo isn't using existing storage namespace #3104

Validate new repo isn't using existing storage namespace #3104

itaiad200 commented Mar 23, 2022

eden-ohana left a comment

arielshaqed left a comment

arielshaqed Mar 23, 2022

itaiad200 Mar 24, 2022

arielshaqed Mar 23, 2022

arielshaqed Mar 23, 2022

johnnyaug Mar 23, 2022 •

edited

Loading

itaiad200 Mar 24, 2022 •

edited

Loading

itaiad200 Mar 24, 2022

johnnyaug Mar 24, 2022

johnnyaug Mar 24, 2022

arielshaqed left a comment

arielshaqed Mar 24, 2022

itaiad200 Mar 27, 2022

arielshaqed Mar 27, 2022

itaiad200 Mar 27, 2022

arielshaqed Mar 27, 2022

itaiad200 Mar 27, 2022

	var errorStorageNamespaceInUse = errors.New("lakeFS repositories can't share storage namespace")
	var errStorageNamespaceInUse = errors.New("lakeFS repositories can't share storage namespace")

	return fmt.Errorf("found lakeFS objects in the storage namespace(%s): %w",
	return fmt.Errorf("%s: %w",

Validate new repo isn't using existing storage namespace #3104

Validate new repo isn't using existing storage namespace #3104

Conversation

itaiad200 commented Mar 23, 2022

eden-ohana left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

itaiad200 Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug Mar 23, 2022 •

edited

Loading

itaiad200 Mar 24, 2022 •

edited

Loading