From 4da70617d1b3566cefeef4212c5302a5cfe25d79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Fri, 22 Mar 2024 16:06:07 +0100 Subject: [PATCH 1/3] Increase test coverage --- internal/api/keppel/accounts_test.go | 42 ++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index 30c1d9f2..93961faf 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -377,7 +377,7 @@ func TestAccountsAPI(t *testing.T) { } } - // check editing of RBAC policies + // check editing of metadata and RBAC policies newRBACPoliciesJSON := []assert.JSONObject{ // rbacPoliciesJSON[0] is deleted // rbacPoliciesJSON[1] is updated as follows: @@ -393,6 +393,10 @@ func TestAccountsAPI(t *testing.T) { "permissions": []string{"pull", "delete"}, }, } + newMetadataJSON := assert.JSONObject{ + "foo": "bingo", + "bar": "buz", + } assert.HTTPRequest{ Method: "PUT", Path: "/keppel/v1/accounts/second", @@ -400,7 +404,9 @@ func TestAccountsAPI(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "rbac_policies": newRBACPoliciesJSON, + // add metadata + "metadata": newMetadataJSON, + "rbac_policies": newRBACPoliciesJSON, }, }, ExpectStatus: http.StatusOK, @@ -409,7 +415,7 @@ func TestAccountsAPI(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": newMetadataJSON, "rbac_policies": newRBACPoliciesJSON, }, }, @@ -603,6 +609,19 @@ func TestPutAccountErrorCases(t *testing.T) { ExpectBody: assert.StringData("account names with the prefix \"keppel\" are reserved for internal use\n"), }.Check(t, h) + assert.HTTPRequest{ + Method: "PUT", + Path: "/keppel/v1/accounts/v1", + Header: map[string]string{"X-Test-Perms": "change:tenant1"}, + Body: assert.JSONObject{ + "account": assert.JSONObject{ + "auth_tenant_id": "tenant1", + }, + }, + ExpectStatus: http.StatusUnprocessableEntity, + ExpectBody: assert.StringData("account names that look like API versions are reserved for internal use\n"), + }.Check(t, h) + assert.HTTPRequest{ Method: "PUT", Path: "/keppel/v1/accounts/first", @@ -695,6 +714,23 @@ func TestPutAccountErrorCases(t *testing.T) { }.Check(t, h) s.SD.ForbidNewAccounts = false + // test setting up invalid required_labels + assert.HTTPRequest{ + Method: "PUT", + Path: "/keppel/v1/accounts/second", + Header: map[string]string{"X-Test-Perms": "change:tenant1"}, + Body: assert.JSONObject{ + "account": assert.JSONObject{ + "auth_tenant_id": "tenant1", + "validation": assert.JSONObject{ + "required_labels": []string{"foo,", ",bar"}, + }, + }, + }, + ExpectStatus: http.StatusUnprocessableEntity, + ExpectBody: assert.StringData("invalid label name: \"foo,\"\n"), + }.Check(t, h) + // test malformed GC policies gcPolicyTestcases := []struct { GCPolicyJSON assert.JSONObject From b192a16e62f6c86b194bacaf72347f5b2a360380 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Thu, 14 Mar 2024 17:12:17 +0100 Subject: [PATCH 2/3] Fix typos --- internal/drivers/basic/ratelimit.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/drivers/basic/ratelimit.go b/internal/drivers/basic/ratelimit.go index cd0dc634..4b33ebbd 100644 --- a/internal/drivers/basic/ratelimit.go +++ b/internal/drivers/basic/ratelimit.go @@ -65,10 +65,10 @@ func init() { }) } -// PluginTypeID implements the keppel.FederationDriver interface. +// PluginTypeID implements the keppel.RateLimitDriver interface. func (d RateLimitDriver) PluginTypeID() string { return "basic" } -// Init implements the keppel.FederationDriver interface. +// Init implements the keppel.RateLimitDriver interface. func (d RateLimitDriver) Init(ad keppel.AuthDriver, cfg keppel.Configuration) error { for action, envVars := range envVars { rate, err := parseRateLimit(envVars.RateLimit) From 8e68a811ecac6b54585e18503824c7ac7ca8aa01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Thu, 14 Mar 2024 17:11:34 +0100 Subject: [PATCH 3/3] ReplicationPolicy: add ApplyToAccount() --- internal/api/keppel/accounts.go | 51 ++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/internal/api/keppel/accounts.go b/internal/api/keppel/accounts.go index 3f6fd90d..0bdbd6a1 100644 --- a/internal/api/keppel/accounts.go +++ b/internal/api/keppel/accounts.go @@ -183,6 +183,32 @@ func renderReplicationPolicy(dbAccount keppel.Account) *ReplicationPolicy { return nil } +func (r ReplicationPolicy) ApplyToAccount(db *keppel.DB, dbAccount *keppel.Account) (httpStatus int, err error) { + switch r.Strategy { + case "on_first_use": + peerCount, err := db.SelectInt(`SELECT COUNT(*) FROM peers WHERE hostname = $1`, r.UpstreamPeerHostName) + if err != nil { + return http.StatusInternalServerError, err + } + + if peerCount == 0 { + return http.StatusUnprocessableEntity, fmt.Errorf(`unknown peer registry: %q`, r.UpstreamPeerHostName) + } + dbAccount.UpstreamPeerHostName = r.UpstreamPeerHostName + case "from_external_on_first_use": + if r.ExternalPeer.URL == "" { + return http.StatusUnprocessableEntity, errors.New(`missing upstream URL for "from_external_on_first_use" replication`) + } + dbAccount.ExternalPeerURL = r.ExternalPeer.URL + dbAccount.ExternalPeerUserName = r.ExternalPeer.UserName + dbAccount.ExternalPeerPassword = r.ExternalPeer.Password + default: + return http.StatusUnprocessableEntity, fmt.Errorf("strategy %s is unsupported", r.Strategy) + } + + return http.StatusOK, nil +} + func renderValidationPolicy(dbAccount keppel.Account) *ValidationPolicy { if dbAccount.RequiredLabels == "" { return nil @@ -348,27 +374,12 @@ func (a *API) handlePutAccount(w http.ResponseWriter, r *http.Request) { if req.Account.ReplicationPolicy != nil { rp := *req.Account.ReplicationPolicy - switch rp.Strategy { - case "on_first_use": - peerCount, err := a.db.SelectInt(`SELECT COUNT(*) FROM peers WHERE hostname = $1`, rp.UpstreamPeerHostName) - if respondwith.ErrorText(w, err) { - return - } - if peerCount == 0 { - http.Error(w, fmt.Sprintf(`unknown peer registry: %q`, rp.UpstreamPeerHostName), http.StatusUnprocessableEntity) - return - } - accountToCreate.UpstreamPeerHostName = rp.UpstreamPeerHostName - case "from_external_on_first_use": - if rp.ExternalPeer.URL == "" { - http.Error(w, `missing upstream URL for "from_external_on_first_use" replication`, http.StatusUnprocessableEntity) - return - } - accountToCreate.ExternalPeerURL = rp.ExternalPeer.URL - accountToCreate.ExternalPeerUserName = rp.ExternalPeer.UserName - accountToCreate.ExternalPeerPassword = rp.ExternalPeer.Password - //NOTE: There are some delayed checks below which require the existing account to be loaded from the DB first. + httpStatus, err := rp.ApplyToAccount(a.db, &accountToCreate) + if err != nil { + http.Error(w, err.Error(), httpStatus) + return } + //NOTE: There are some delayed checks below which require the existing account to be loaded from the DB first. } // validate validation policy