Move setting endpoints to be under `/settings/` #6649

johnnyaug · 2023-09-26T13:13:42Z

Resolves #6535.

Deprecate existing endpoints for setting/getting branch protection rules and GC rules.
Create new endpoints under /settings:
- /settings/gc_rules: GET/PUT/DELETE all garbage collection rules.
- /settings/branch_protect: GET/PUT all branch protection rules.

Notes

Adding a single branch protection rule is no longer supported with the API. Instead, one should set all the rules at once. In order to avoid race conditions, a new ETag header is returned when getting the rules. The existing ETag should be specified as the If-Match header when setting the rules.

github-actions · 2023-09-26T13:14:42Z

♻️ PR Preview 91c95f3 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

guy-har

Nice and clean 😄
Added some comments, didn't go over tests and controller yet

guy-har · 2023-09-27T10:01:35Z

api/swagger.yml

@@ -4077,8 +4201,8 @@ paths:

    post:
      tags:
-        - retention


why not leave retention and add internal? same goes for other removed tags

The tags are reflected in the method names in the SDK, we want them to appear as "internal" to emphasize the fact that they are deprecated. These endpoints will be removed in the future.

guy-har · 2023-09-27T10:11:17Z

cmd/lakectl/cmd/gc_set_config.go

 		err = json.NewDecoder(reader).Decode(&body)
 		if err != nil {
 			DieErr(err)
 		}
 		client := getClient()
-		resp, err := client.SetGarbageCollectionRulesWithResponse(cmd.Context(), u.Repository, body)
+		resp, err := client.SetGCRules(cmd.Context(), u.Repository, body)


With response

Not needed here, I think?

guy-har · 2023-09-27T10:40:48Z

pkg/graveler/branch/protection_manager.go

-	if errors.Is(err, graveler.ErrNotFound) {
-		return &graveler.BranchProtectionRules{}, nil
-	}


This was removed, and seems good, just checking it was intentional

Intentional: decided to propagate the error to the controller, and handle it there.

guy-har · 2023-09-27T10:43:35Z

pkg/graveler/branch/protection_manager.go

-	}
-	if err != nil {
-		return nil, err
+func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, ifMatchETag *string) error {


Suggested change

func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, ifMatchETag *string) error {

func (m *ProtectionManager) SetRules(ctx context.Context, repository *graveler.RepositoryRecord, rules *graveler.BranchProtectionRules, etagPredicate *string) error {

changed to lastKnownChecksum. Didn't want to reuse the "predicate" word, it's not really a predicate

guy-har · 2023-09-27T10:48:26Z

pkg/graveler/graveler.go

+	// If ifMatchETag is not nil, the update will only succeed if the current ETag matches the given one.
+	// Otherwise, ErrPreconditionFailed is returned.


And if ifMatchETag? no validation?

Added clarification and changed the behavior in some of the levels.

guy-har · 2023-09-27T10:50:32Z

pkg/graveler/settings/manager.go

@@ -67,6 +68,21 @@ func (m *Manager) Save(ctx context.Context, repository *graveler.RepositoryRecor
 	return kv.SetMsg(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting)
 }

+// SaveIf persists the given setting under the given repository and key. Overrides settings key in KV Store.
+// The setting is persisted only if the given ETag matches the ETag of the current setting.


Same here, I think we should document what happens ifMatchETag is nil

guy-har · 2023-09-27T10:57:21Z

api/swagger.yml

@@ -4059,8 +4183,8 @@ paths:
          type: string
    get:
      tags:
-        - retention
-      operationId: getGarbageCollectionRules
+        - internal


with the new settings path, are the old ones internal or deprecated?

They are deprecated: marking them as internal was a way to make sure users know they are deprecated. I will also add the deprecated notation like you did in your recent PR.

guy-har · 2023-09-27T11:00:23Z

webui/src/lib/api/index.js

+        const response = await apiRequest(`/repositories/${encodeURIComponent(repoID)}/settings/branch_protection`, {
+            method: 'PUT',
+            body: JSON.stringify(rules),
+        }, {'ETag': lastKnownChecksum});


Are we sure we want the name to be ETag, it's basically an implementation detail, for the user it's just a lock/token/predicate or something like that

I do not care much either way: ETag is by now a term of HTTP, not only of S3. It's an actual standard header.

ETag is part of the HTTP spec and is intended to be used together with the If-Match header.

UPDATE: this was a mistake, the header used here should be If-Match.

arielshaqed

Thanks! Nothing major except my confusion around ETags and the if-match stuff, sorry.

arielshaqed · 2023-09-27T08:14:03Z

cmd/lakectl/cmd/branch_protect.go

+		resp, err := client.GetBranchProtectionRulesWithResponse(cmd.Context(), u.Repository)
+		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)
+		rules := *resp.JSON200
+		for i, rule := range *resp.JSON200 {


Can we use https://pkg.go.dev/golang.org/x/exp/slices#DeleteFunc here instead?

Done, thanks!

arielshaqed · 2023-09-27T13:07:50Z

pkg/api/controller.go

+	if eTag == "" {
+		// workaround since swagger doesn't allow empty headers
+		// https://github.com/deepmap/oapi-codegen/issues/954
+		eTag = base64.StdEncoding.EncodeToString([]byte("EMPTY"))
+	}


This is quite worrying. Can we omit the etag if this happens? What if we used openapi-codegen version 6.6.0, or 7.0.0? @Jonathan-Rosenberg & I need to bump at least to 6.6.0 and probably to 7.0.0.

Removed this.

arielshaqed · 2023-09-27T13:10:14Z

pkg/api/controller.go

+	eTag := params.IfMatch
+	if swag.StringValue(eTag) == "" {
+		// workaround since swagger doesn't allow empty headers
+		// https://github.com/deepmap/oapi-codegen/issues/954
+		eTag = swag.String(base64.StdEncoding.EncodeToString([]byte("EMPTY")))
+	}


I don't understand this. If there is no If-Match header, I think this makes it behave as though we did ask for a particular ETag.

You're right, this is not needed.

arielshaqed · 2023-09-27T13:11:12Z

pkg/api/controller.go

@@ -2113,7 +2241,9 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response
 	case errors.Is(err, graveler.ErrTooManyTries):
 		log.Debug("Retried too many times")
 		cb(w, r, http.StatusLocked, "Too many attempts, try again later")
-
+	case errors.Is(err, graveler.ErrPreconditionFailed):


What changed that we need this now? Or is it a quick fix for a bug that you discovered along the way?

This error is now returned from Graveler when the conditional update of branch protection rules fails.

arielshaqed · 2023-09-27T13:16:26Z

webui/src/lib/api/index.js

+        const response = await apiRequest(`/repositories/${encodeURIComponent(repoID)}/settings/branch_protection`, {
+            method: 'PUT',
+            body: JSON.stringify(rules),
+        }, {'ETag': lastKnownChecksum});


I do not care much either way: ETag is by now a term of HTTP, not only of S3. It's an actual standard header.

N-o-Z · 2023-09-27T13:50:01Z

pkg/api/controller.go

@@ -2113,7 +2241,9 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response
 	case errors.Is(err, graveler.ErrTooManyTries):
 		log.Debug("Retried too many times")
 		cb(w, r, http.StatusLocked, "Too many attempts, try again later")
-
+	case errors.Is(err, graveler.ErrPreconditionFailed):


N-o-Z · 2023-09-27T14:11:18Z

pkg/graveler/settings/manager.go

@@ -67,6 +68,21 @@ func (m *Manager) Save(ctx context.Context, repository *graveler.RepositoryRecor
 	return kv.SetMsg(ctx, m.store, graveler.RepoPartition(repository), []byte(graveler.SettingsPath(key)), setting)
 }

+// SaveIf persists the given setting under the given repository and key. Overrides settings key in KV Store.
+// The setting is persisted only if the given ETag matches the ETag of the current setting.
+// Otherwise, ErrPreconditionFailed is returned.


Usually in our code when predicate (ifMatchETag) is nil - this is an indication for saveIf not exists. This might cause a bit of confusion

Also, will this make the Save method redundant?

Fixed (kind of): changed this to no longer be a pointer. The update is always conditional - empty string means update-if-not-exists. Now branch.ProtectionManager exposes both SetRules and SetRulesIf. SetRules uses the unconditional Save.

However, the catalog and graveler still use a pointer, where nil means unconditional update, and empty string means update-if-not-exists.

N-o-Z · 2023-09-27T14:24:53Z

esti/ugc_test.go

Why did we remove this?
We are removing test coverage without providing an alternative

Note that these are not Spark tests - they test the lakeFS API

This test is no longer used (it tests code that doesn't exist). It is replaced by "unified GC tests".

N-o-Z · 2023-09-27T15:55:50Z

pkg/api/controller.go

-		return
-	}
-	writeResponse(w, r, http.StatusNoContent, nil)
+func (c *Controller) InternalSetGarbageCollectionRules(w http.ResponseWriter, r *http.Request, body apigen.InternalSetGarbageCollectionRulesJSONRequestBody, repository string) {


Can you explain why we need the Internal functions

We are keeping them for backward compatibility, and they will be removed in the future.

OK, I see what you did there - the api endpoint didn't change

N-o-Z · 2023-09-27T16:00:23Z

pkg/graveler/graveler.go

@@ -610,15 +610,12 @@ type VersionController interface {
 	GCNewRunID() string

 	// GetBranchProtectionRules return all branch protection rules for the repository
-	GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, error)
+	GetBranchProtectionRules(ctx context.Context, repository *RepositoryRecord) (*BranchProtectionRules, string, error)


Can you update documentation on the returned values.
Also why is the etag necessary on the branch protection rules but not in GC rules?

arielshaqed

Thanks!

johnnyaug added 3 commits September 21, 2023 15:29

move legacy settings API to "internal"

938c0a0

add etag to branch protection

4c906dd

config

fb2cbd1

johnnyaug added the include-changelog PR description should be included in next release changelog label Sep 26, 2023

johnnyaug changed the title ~~1.0/settings endpoints~~ Move setting endpoints to be under `/settings/ Sep 26, 2023

johnnyaug changed the title ~~Move setting endpoints to be under `/settings/~~ Move setting endpoints to be under /settings/ Sep 26, 2023

johnnyaug added area/API Improvements or additions to the API v1.0.0-blocker Issues that should be closed before going out with v1.0.0 and removed v1.0.0-blocker Issues that should be closed before going out with v1.0.0 labels Sep 26, 2023

johnnyaug added 7 commits September 26, 2023 17:01

fixes+tests

eff3038

remove add

f1c5441

fix lints

783977f

remove get

925b850

update mock

aceb8fb

fix compilation

cbea2ad

fix compilation

83efe42

johnnyaug requested a review from guy-har September 26, 2023 15:38

johnnyaug marked this pull request as ready for review September 26, 2023 20:58

johnnyaug requested a review from arielshaqed September 26, 2023 20:58

arielshaqed added the v1.0.0-blocker Issues that should be closed before going out with v1.0.0 label Sep 27, 2023

johnnyaug requested a review from N-o-Z September 27, 2023 09:02

guy-har reviewed Sep 27, 2023

View reviewed changes

arielshaqed requested changes Sep 27, 2023

View reviewed changes

N-o-Z reviewed Sep 27, 2023

View reviewed changes

johnnyaug added 3 commits September 27, 2023 17:37

CR fixes

bd37197

CR fixes

e6f1c40

fix tests

41177d3

johnnyaug added 2 commits September 27, 2023 18:23

cr fix: use DeleteFunc

e956035

Merge branch 'master' into 1.0/settings_endpoints

578a120

N-o-Z reviewed Sep 27, 2023

View reviewed changes

cr fix: comments and naming

2f8aa41

johnnyaug requested review from N-o-Z, arielshaqed and guy-har September 28, 2023 08:09

johnnyaug added 2 commits October 1, 2023 12:22

Merge branch 'master' into 1.0/settings_endpoints

b754b61

clients after merge conflict

91c95f3

arielshaqed approved these changes Oct 1, 2023

View reviewed changes

johnnyaug merged commit 2676ff3 into master Oct 1, 2023
34 checks passed

johnnyaug deleted the 1.0/settings_endpoints branch October 1, 2023 10:05

	func (m ProtectionManager) SetRules(ctx context.Context, repository graveler.RepositoryRecord, rules graveler.BranchProtectionRules, ifMatchETag string) error {
	func (m ProtectionManager) SetRules(ctx context.Context, repository graveler.RepositoryRecord, rules graveler.BranchProtectionRules, etagPredicate string) error {

		// If ifMatchETag is not nil, the update will only succeed if the current ETag matches the given one.
		// Otherwise, ErrPreconditionFailed is returned.

Move setting endpoints to be under /settings/ #6649

Move setting endpoints to be under /settings/ #6649

Conversation

johnnyaug commented Sep 26, 2023 • edited Loading

Notes

github-actions bot commented Sep 26, 2023 • edited Loading

guy-har 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

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

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

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

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

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

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Move setting endpoints to be under `/settings/` #6649

Move setting endpoints to be under `/settings/` #6649

johnnyaug commented Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 26, 2023 •

edited

Loading