From 3ec51dcf456b2afe02857089895a261653abddd4 Mon Sep 17 00:00:00 2001 From: Nico Cesar Date: Thu, 7 Jan 2021 11:23:21 -0500 Subject: [PATCH] 17014: controller handles container requests This commit has: * Container Requests in the new codepath * runtime_constraints and scheduling parameters have now default values and cannot be nil. This affects Railsapi CR comparison in Final state * Adapted Login and Logout so they work for CR * Tests in lib/controller/integration_test.go that take in account runtime_token with intermediate clusters * Some tests in lib/controller/federation_test.go had to be addapted, but for the most part remained as is to gurantee compatibilty * Added the check that SystemRootToken has to be non-empty * Railsapi fixtures that include finalized CR that are used in the tests to make sure we return the right object * Minimal changes in the documentation to reflect all the user visible changes * Added expires_at parameter for login and related test Arvados-DCO-1.1-Signed-off-by: Nico Cesar --- .../container_requests_controller_test.rb | 1 - .../container_requests.html.textile.liquid | 2 +- lib/boot/supervisor.go | 2 + lib/controller/cmd.go | 1 + lib/controller/federation/conn.go | 62 +++++++ lib/controller/federation/generate.go | 2 +- lib/controller/federation/generated.go | 41 +++++ lib/controller/federation_test.go | 137 +++++++++------- lib/controller/handler.go | 2 + lib/controller/handler_test.go | 2 + lib/controller/integration_test.go | 153 +++++++++++++++++- lib/controller/localdb/conn.go | 9 +- lib/controller/localdb/login.go | 31 ++-- lib/controller/localdb/login_ldap.go | 6 +- lib/controller/localdb/login_ldap_test.go | 4 +- lib/controller/localdb/login_oidc.go | 9 +- lib/controller/localdb/login_pam.go | 6 +- lib/controller/localdb/login_pam_test.go | 4 +- lib/controller/localdb/login_testuser.go | 6 +- lib/controller/localdb/login_testuser_test.go | 4 +- lib/controller/router/response.go | 76 ++++++--- lib/controller/router/router.go | 35 ++++ lib/controller/rpc/conn.go | 64 +++++++- lib/crunchrun/crunchrun.go | 6 +- lib/crunchrun/crunchrun_test.go | 6 +- sdk/go/arvados/api.go | 10 ++ sdk/go/arvados/container.go | 5 +- sdk/go/arvadostest/api.go | 20 +++ .../controllers/user_sessions_controller.rb | 37 +++-- services/api/app/models/arvados_model.rb | 34 ++++ services/api/app/models/container.rb | 10 +- services/api/app/models/container_request.rb | 25 +-- .../api/test/fixtures/container_requests.yml | 47 ++++-- .../v1/container_requests_controller_test.rb | 25 ++- .../user_sessions_controller_test.rb | 25 +++ .../api/test/unit/container_request_test.rb | 47 +----- services/api/test/unit/container_test.rb | 14 +- 37 files changed, 743 insertions(+), 227 deletions(-) diff --git a/apps/workbench/test/controllers/container_requests_controller_test.rb b/apps/workbench/test/controllers/container_requests_controller_test.rb index 73d357f3a6..c8709df3c3 100644 --- a/apps/workbench/test/controllers/container_requests_controller_test.rb +++ b/apps/workbench/test/controllers/container_requests_controller_test.rb @@ -138,7 +138,6 @@ class ContainerRequestsControllerTest < ActionController::TestCase assert_includes @response.body, "href=\"\/collections/fa7aeb5140e2848d39b416daeef4ffc5+45/foobar\?" # locator on command assert_includes @response.body, "href=\"\/collections/fa7aeb5140e2848d39b416daeef4ffc5+45/foo" # mount input1 assert_includes @response.body, "href=\"\/collections/fa7aeb5140e2848d39b416daeef4ffc5+45/bar" # mount input2 - assert_includes @response.body, "href=\"\/collections/f9ddda46bb293b6847da984e3aa735db+290" # mount workflow assert_includes @response.body, "href=\"#Log\"" assert_includes @response.body, "href=\"#Provenance\"" end diff --git a/doc/api/methods/container_requests.html.textile.liquid b/doc/api/methods/container_requests.html.textile.liquid index cd566f5ce4..b24a24e067 100644 --- a/doc/api/methods/container_requests.html.textile.liquid +++ b/doc/api/methods/container_requests.html.textile.liquid @@ -49,7 +49,7 @@ table(table table-bordered table-condensed). |cwd|string|Initial working directory, given as an absolute path (in the container) or a path relative to the WORKDIR given in the image's Dockerfile.|Required.| |command|array of strings|Command to execute in the container.|Required. e.g., @["echo","hello"]@| |output_path|string|Path to a directory or file inside the container that should be preserved as container's output when it finishes. This path must be one of the mount targets. For best performance, point output_path to a writable collection mount. See "Pre-populate output using Mount points":#pre-populate-output for details regarding optional output pre-population using mount points and "Symlinks in output":#symlinks-in-output for additional details.|Required.| -|output_name|string|Desired name for the output collection. If null, a name will be assigned automatically.|| +|output_name|string|Desired name for the output collection. If null or empty, a name will be assigned automatically.|| |output_ttl|integer|Desired lifetime for the output collection, in seconds. If zero, the output collection will not be deleted automatically.|| |priority|integer|Range 0-1000. Indicate scheduling order preference.|Clients are expected to submit container requests with zero priority in order to preview the container that will be used to satisfy it. Priority can be null if and only if state!="Committed". See "below for more details":#priority .| |expires_at|datetime|After this time, priority is considered to be zero.|Not yet implemented.| diff --git a/lib/boot/supervisor.go b/lib/boot/supervisor.go index f2e715a766..752466c2a2 100644 --- a/lib/boot/supervisor.go +++ b/lib/boot/supervisor.go @@ -59,6 +59,8 @@ type Supervisor struct { environ []string // for child processes } +func (super *Supervisor) Cluster() *arvados.Cluster { return super.cluster } + func (super *Supervisor) Start(ctx context.Context, cfg *arvados.Config, cfgPath string) { super.ctx, super.cancel = context.WithCancel(ctx) super.done = make(chan struct{}) diff --git a/lib/controller/cmd.go b/lib/controller/cmd.go index 0c46e857b3..7ab7f5305b 100644 --- a/lib/controller/cmd.go +++ b/lib/controller/cmd.go @@ -13,6 +13,7 @@ import ( "github.com/prometheus/client_golang/prometheus" ) +// Command starts a controller service. See cmd/arvados-server/cmd.go var Command cmd.Handler = service.Command(arvados.ServiceNameController, newHandler) func newHandler(_ context.Context, cluster *arvados.Cluster, _ string, _ *prometheus.Registry) service.Handler { diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go index 130368124c..00523c7826 100644 --- a/lib/controller/federation/conn.go +++ b/lib/controller/federation/conn.go @@ -336,6 +336,68 @@ func (conn *Conn) ContainerUnlock(ctx context.Context, options arvados.GetOption return conn.chooseBackend(options.UUID).ContainerUnlock(ctx, options) } +func (conn *Conn) ContainerRequestList(ctx context.Context, options arvados.ListOptions) (arvados.ContainerRequestList, error) { + return conn.generated_ContainerRequestList(ctx, options) +} + +func (conn *Conn) ContainerRequestCreate(ctx context.Context, options arvados.CreateOptions) (arvados.ContainerRequest, error) { + be := conn.chooseBackend(options.ClusterID) + if be == conn.local { + return be.ContainerRequestCreate(ctx, options) + } + if _, ok := options.Attrs["runtime_token"]; !ok { + // If runtime_token is not set, create a new token + aca, err := conn.local.APIClientAuthorizationCurrent(ctx, arvados.GetOptions{}) + if err != nil { + // This should probably be StatusUnauthorized + // (need to update test in + // lib/controller/federation_test.go): + // When RoR is out of the picture this should be: + // return arvados.ContainerRequest{}, httpErrorf(http.StatusUnauthorized, "%w", err) + return arvados.ContainerRequest{}, httpErrorf(http.StatusForbidden, "%s", "invalid API token") + } + user, err := conn.local.UserGetCurrent(ctx, arvados.GetOptions{}) + if err != nil { + return arvados.ContainerRequest{}, err + } + if len(aca.Scopes) == 0 || aca.Scopes[0] != "all" { + return arvados.ContainerRequest{}, httpErrorf(http.StatusForbidden, "token scope is not [all]") + } + if strings.HasPrefix(aca.UUID, conn.cluster.ClusterID) { + // Local user, submitting to a remote cluster. + // Create a new time-limited token. + local, ok := conn.local.(*localdb.Conn) + if !ok { + return arvados.ContainerRequest{}, httpErrorf(http.StatusInternalServerError, "bug: local backend is a %T, not a *localdb.Conn", conn.local) + } + aca, err = local.CreateAPIClientAuthorization(ctx, conn.cluster.SystemRootToken, rpc.UserSessionAuthInfo{UserUUID: user.UUID, + ExpiresAt: time.Now().UTC().Add(conn.cluster.Collections.BlobSigningTTL.Duration())}) + if err != nil { + return arvados.ContainerRequest{}, err + } + options.Attrs["runtime_token"] = aca.TokenV2() + } else { + // Remote user. Container request will use the + // current token, minus the trailing portion + // (optional container uuid). + options.Attrs["runtime_token"] = aca.TokenV2() + } + } + return be.ContainerRequestCreate(ctx, options) +} + +func (conn *Conn) ContainerRequestUpdate(ctx context.Context, options arvados.UpdateOptions) (arvados.ContainerRequest, error) { + return conn.chooseBackend(options.UUID).ContainerRequestUpdate(ctx, options) +} + +func (conn *Conn) ContainerRequestGet(ctx context.Context, options arvados.GetOptions) (arvados.ContainerRequest, error) { + return conn.chooseBackend(options.UUID).ContainerRequestGet(ctx, options) +} + +func (conn *Conn) ContainerRequestDelete(ctx context.Context, options arvados.DeleteOptions) (arvados.ContainerRequest, error) { + return conn.chooseBackend(options.UUID).ContainerRequestDelete(ctx, options) +} + func (conn *Conn) SpecimenList(ctx context.Context, options arvados.ListOptions) (arvados.SpecimenList, error) { return conn.generated_SpecimenList(ctx, options) } diff --git a/lib/controller/federation/generate.go b/lib/controller/federation/generate.go index ab5d9966a4..9ce7fdcb21 100644 --- a/lib/controller/federation/generate.go +++ b/lib/controller/federation/generate.go @@ -52,7 +52,7 @@ func main() { defer out.Close() out.Write(regexp.MustCompile(`(?ms)^.*package .*?import.*?\n\)\n`).Find(buf)) io.WriteString(out, "//\n// -- this file is auto-generated -- do not edit -- edit list.go and run \"go generate\" instead --\n//\n\n") - for _, t := range []string{"Container", "Specimen", "User"} { + for _, t := range []string{"Container", "ContainerRequest", "Specimen", "User"} { _, err := out.Write(bytes.ReplaceAll(orig, []byte("Collection"), []byte(t))) if err != nil { panic(err) diff --git a/lib/controller/federation/generated.go b/lib/controller/federation/generated.go index 8745f3b973..ab9db93a4d 100755 --- a/lib/controller/federation/generated.go +++ b/lib/controller/federation/generated.go @@ -58,6 +58,47 @@ func (conn *Conn) generated_ContainerList(ctx context.Context, options arvados.L return merged, err } +func (conn *Conn) generated_ContainerRequestList(ctx context.Context, options arvados.ListOptions) (arvados.ContainerRequestList, error) { + var mtx sync.Mutex + var merged arvados.ContainerRequestList + var needSort atomic.Value + needSort.Store(false) + err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) { + options.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor + cl, err := backend.ContainerRequestList(ctx, options) + if err != nil { + return nil, err + } + mtx.Lock() + defer mtx.Unlock() + if len(merged.Items) == 0 { + merged = cl + } else if len(cl.Items) > 0 { + merged.Items = append(merged.Items, cl.Items...) + needSort.Store(true) + } + uuids := make([]string, 0, len(cl.Items)) + for _, item := range cl.Items { + uuids = append(uuids, item.UUID) + } + return uuids, nil + }) + if needSort.Load().(bool) { + // Apply the default/implied order, "modified_at desc" + sort.Slice(merged.Items, func(i, j int) bool { + mi, mj := merged.Items[i].ModifiedAt, merged.Items[j].ModifiedAt + return mj.Before(mi) + }) + } + if merged.Items == nil { + // Return empty results as [], not null + // (https://github.com/golang/go/issues/27589 might be + // a better solution in the future) + merged.Items = []arvados.ContainerRequest{} + } + return merged, err +} + func (conn *Conn) generated_SpecimenList(ctx context.Context, options arvados.ListOptions) (arvados.SpecimenList, error) { var mtx sync.Mutex var merged arvados.SpecimenList diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go index 031166b291..a92fc71053 100644 --- a/lib/controller/federation_test.go +++ b/lib/controller/federation_test.go @@ -352,7 +352,13 @@ func (s *FederationSuite) localServiceReturns404(c *check.C) *httpserver.Server return s.localServiceHandler(c, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.URL.Path == "/arvados/v1/api_client_authorizations/current" { if req.Header.Get("Authorization") == "Bearer "+arvadostest.ActiveToken { - json.NewEncoder(w).Encode(arvados.APIClientAuthorization{UUID: arvadostest.ActiveTokenUUID, APIToken: arvadostest.ActiveToken}) + json.NewEncoder(w).Encode(arvados.APIClientAuthorization{UUID: arvadostest.ActiveTokenUUID, APIToken: arvadostest.ActiveToken, Scopes: []string{"all"}}) + } else { + w.WriteHeader(http.StatusUnauthorized) + } + } else if req.URL.Path == "/arvados/v1/users/current" { + if req.Header.Get("Authorization") == "Bearer "+arvadostest.ActiveToken { + json.NewEncoder(w).Encode(arvados.User{UUID: arvadostest.ActiveUserUUID}) } else { w.WriteHeader(http.StatusUnauthorized) } @@ -627,38 +633,78 @@ func (s *FederationSuite) TestCreateRemoteContainerRequest(c *check.C) { c.Check(strings.HasPrefix(cr.UUID, "zzzzz-"), check.Equals, true) } +// getCRfromMockRequest returns a ContainerRequest with the content of the +// request sent to the remote mock. This function takes into account the +// Content-Type and acts accordingly. +func (s *FederationSuite) getCRfromMockRequest(c *check.C) arvados.ContainerRequest { + + // Body can be a json formated or something like: + // cluster_id=zmock&container_request=%7B%22command%22%3A%5B%22abc%22%5D%2C%22container_image%22%3A%22123%22%2C%22...7D + // or: + // "{\"container_request\":{\"command\":[\"abc\"],\"container_image\":\"12...Uncommitted\"}}" + + var cr arvados.ContainerRequest + data, err := ioutil.ReadAll(s.remoteMockRequests[0].Body) + c.Check(err, check.IsNil) + + if s.remoteMockRequests[0].Header.Get("Content-Type") == "application/json" { + // legacy code path sends a JSON request body + var answerCR struct { + ContainerRequest arvados.ContainerRequest `json:"container_request"` + } + c.Check(json.Unmarshal(data, &answerCR), check.IsNil) + cr = answerCR.ContainerRequest + } else if s.remoteMockRequests[0].Header.Get("Content-Type") == "application/x-www-form-urlencoded" { + // new code path sends a form-encoded request body with a JSON-encoded parameter value + decodedValue, err := url.ParseQuery(string(data)) + c.Check(err, check.IsNil) + decodedValueCR := decodedValue.Get("container_request") + c.Check(json.Unmarshal([]byte(decodedValueCR), &cr), check.IsNil) + } else { + // mock needs to have Content-Type that we can parse. + c.Fail() + } + + return cr +} + func (s *FederationSuite) TestCreateRemoteContainerRequestCheckRuntimeToken(c *check.C) { // Send request to zmock and check that outgoing request has // runtime_token set with a new random v2 token. defer s.localServiceReturns404(c).Close() - // pass cluster_id via query parameter, this allows arvados-controller - // to avoid parsing the body req := httptest.NewRequest("POST", "/arvados/v1/container_requests?cluster_id=zmock", strings.NewReader(`{ - "container_request": { - "name": "hello world", - "state": "Uncommitted", - "output_path": "/", - "container_image": "123", - "command": ["abc"] - } -} -`)) + "container_request": { + "name": "hello world", + "state": "Uncommitted", + "output_path": "/", + "container_image": "123", + "command": ["abc"] + } + } + `)) req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveTokenV2) req.Header.Set("Content-type", "application/json") + // We replace zhome with zzzzz values (RailsAPI, ClusterID, SystemRootToken) + // SystemRoot token is needed because we check the + // https://[RailsAPI]/arvados/v1/api_client_authorizations/current + // https://[RailsAPI]/arvados/v1/users/current and + // https://[RailsAPI]/auth/controller/callback arvadostest.SetServiceURL(&s.testHandler.Cluster.Services.RailsAPI, "https://"+os.Getenv("ARVADOS_TEST_API_HOST")) s.testHandler.Cluster.ClusterID = "zzzzz" + s.testHandler.Cluster.SystemRootToken = arvadostest.SystemRootToken resp := s.testRequest(req).Result() c.Check(resp.StatusCode, check.Equals, http.StatusOK) - var cr struct { - arvados.ContainerRequest `json:"container_request"` - } - c.Check(json.NewDecoder(s.remoteMockRequests[0].Body).Decode(&cr), check.IsNil) - c.Check(strings.HasPrefix(cr.ContainerRequest.RuntimeToken, "v2/zzzzz-gj3su-"), check.Equals, true) - c.Check(cr.ContainerRequest.RuntimeToken, check.Not(check.Equals), arvadostest.ActiveTokenV2) + + cr := s.getCRfromMockRequest(c) + + // Runtime token must match zzzzz cluster + c.Check(cr.RuntimeToken, check.Matches, "v2/zzzzz-gj3su-.*") + // RuntimeToken must be different than the Original Token we originally did the request with. + c.Check(cr.RuntimeToken, check.Not(check.Equals), arvadostest.ActiveTokenV2) } func (s *FederationSuite) TestCreateRemoteContainerRequestCheckSetRuntimeToken(c *check.C) { @@ -670,54 +716,25 @@ func (s *FederationSuite) TestCreateRemoteContainerRequestCheckSetRuntimeToken(c // to avoid parsing the body req := httptest.NewRequest("POST", "/arvados/v1/container_requests?cluster_id=zmock", strings.NewReader(`{ - "container_request": { - "name": "hello world", - "state": "Uncommitted", - "output_path": "/", - "container_image": "123", - "command": ["abc"], - "runtime_token": "xyz" - } -} -`)) + "container_request": { + "name": "hello world", + "state": "Uncommitted", + "output_path": "/", + "container_image": "123", + "command": ["abc"], + "runtime_token": "xyz" + } + } + `)) req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveToken) req.Header.Set("Content-type", "application/json") resp := s.testRequest(req).Result() c.Check(resp.StatusCode, check.Equals, http.StatusOK) - var cr struct { - arvados.ContainerRequest `json:"container_request"` - } - c.Check(json.NewDecoder(s.remoteMockRequests[0].Body).Decode(&cr), check.IsNil) - c.Check(cr.ContainerRequest.RuntimeToken, check.Equals, "xyz") -} -func (s *FederationSuite) TestCreateRemoteContainerRequestRuntimeTokenFromAuth(c *check.C) { - // Send request to zmock and check that outgoing request has - // runtime_token set using the Auth token because the user is remote. + cr := s.getCRfromMockRequest(c) - defer s.localServiceReturns404(c).Close() - // pass cluster_id via query parameter, this allows arvados-controller - // to avoid parsing the body - req := httptest.NewRequest("POST", "/arvados/v1/container_requests?cluster_id=zmock", - strings.NewReader(`{ - "container_request": { - "name": "hello world", - "state": "Uncommitted", - "output_path": "/", - "container_image": "123", - "command": ["abc"] - } -} -`)) - req.Header.Set("Authorization", "Bearer "+arvadostest.ActiveTokenV2+"/zzzzz-dz642-parentcontainer") - req.Header.Set("Content-type", "application/json") - resp := s.testRequest(req).Result() - c.Check(resp.StatusCode, check.Equals, http.StatusOK) - var cr struct { - arvados.ContainerRequest `json:"container_request"` - } - c.Check(json.NewDecoder(s.remoteMockRequests[0].Body).Decode(&cr), check.IsNil) - c.Check(cr.ContainerRequest.RuntimeToken, check.Equals, arvadostest.ActiveTokenV2) + // After mocking around now making sure the runtime_token we sent is still there. + c.Check(cr.RuntimeToken, check.Equals, "xyz") } func (s *FederationSuite) TestCreateRemoteContainerRequestError(c *check.C) { diff --git a/lib/controller/handler.go b/lib/controller/handler.go index 6669e020fd..b04757ac33 100644 --- a/lib/controller/handler.go +++ b/lib/controller/handler.go @@ -100,6 +100,8 @@ func (h *Handler) setup() { mux.Handle("/arvados/v1/collections/", rtr) mux.Handle("/arvados/v1/users", rtr) mux.Handle("/arvados/v1/users/", rtr) + mux.Handle("/arvados/v1/container_requests", rtr) + mux.Handle("/arvados/v1/container_requests/", rtr) mux.Handle("/login", rtr) mux.Handle("/logout", rtr) } diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go index 7d8266a85c..d12e4fa33d 100644 --- a/lib/controller/handler_test.go +++ b/lib/controller/handler_test.go @@ -294,6 +294,8 @@ func (s *HandlerSuite) CheckObjectType(c *check.C, url string, token string, ski } resp2, err := client.Get(s.cluster.Services.RailsAPI.ExternalURL.String() + url + "/?api_token=" + token) c.Check(err, check.Equals, nil) + c.Assert(resp2.StatusCode, check.Equals, http.StatusOK, + check.Commentf("Wasn't able to get data from the RailsAPI at %q", url)) defer resp2.Body.Close() db, err := ioutil.ReadAll(resp2.Body) c.Check(err, check.Equals, nil) diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go index 0388f21bee..a574f39780 100644 --- a/lib/controller/integration_test.go +++ b/lib/controller/integration_test.go @@ -7,6 +7,7 @@ package controller import ( "bytes" "context" + "database/sql" "encoding/json" "fmt" "io" @@ -20,6 +21,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "git.arvados.org/arvados.git/lib/boot" "git.arvados.org/arvados.git/lib/config" @@ -209,6 +211,7 @@ func (s *IntegrationSuite) userClients(rootctx context.Context, c *check.C, conn FirstName: "Example", LastName: "User", Username: "example", + ExpiresAt: time.Now().Add(1 * time.Hour), }, }) c.Assert(err, check.IsNil) @@ -460,6 +463,7 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithFedToken(c *check.C) { c.Assert(err, check.IsNil) req.Header.Set("Content-Type", "application/json") err = ac2.DoAndDecode(&cr, req) + c.Assert(err, check.IsNil) c.Logf("err == %#v", err) c.Log("...get user with good token") @@ -486,10 +490,153 @@ func (s *IntegrationSuite) TestCreateContainerRequestWithFedToken(c *check.C) { req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", "OAuth2 "+ac2.AuthToken) resp, err = arvados.InsecureHTTPClient.Do(req) - if c.Check(err, check.IsNil) { - err = json.NewDecoder(resp.Body).Decode(&cr) + c.Assert(err, check.IsNil) + err = json.NewDecoder(resp.Body).Decode(&cr) + c.Check(err, check.IsNil) + c.Check(cr.UUID, check.Matches, "z2222-.*") +} + +func (s *IntegrationSuite) TestCreateContainerRequestWithBadToken(c *check.C) { + conn1 := s.conn("z1111") + rootctx1, _, _ := s.rootClients("z1111") + _, ac1, _, au := s.userClients(rootctx1, c, conn1, "z1111", true) + + tests := []struct { + name string + token string + expectedCode int + }{ + {"Good token", ac1.AuthToken, http.StatusOK}, + {"Bogus token", "abcdef", http.StatusUnauthorized}, + {"v1-looking token", "badtoken00badtoken00badtoken00badtoken00b", http.StatusUnauthorized}, + {"v2-looking token", "v2/" + au.UUID + "/badtoken00badtoken00badtoken00badtoken00b", http.StatusUnauthorized}, + } + + body, _ := json.Marshal(map[string]interface{}{ + "container_request": map[string]interface{}{ + "command": []string{"echo"}, + "container_image": "d41d8cd98f00b204e9800998ecf8427e+0", + "cwd": "/", + "output_path": "/", + }, + }) + + for _, tt := range tests { + c.Log(c.TestName() + " " + tt.name) + ac1.AuthToken = tt.token + req, err := http.NewRequest("POST", "https://"+ac1.APIHost+"/arvados/v1/container_requests", bytes.NewReader(body)) + c.Assert(err, check.IsNil) + req.Header.Set("Content-Type", "application/json") + resp, err := ac1.Do(req) + c.Assert(err, check.IsNil) + c.Assert(resp.StatusCode, check.Equals, tt.expectedCode) + } +} + +// We test the direct access to the database +// normally an integration test would not have a database access, but in this case we need +// to test tokens that are secret, so there is no API response that will give them back +func (s *IntegrationSuite) dbConn(c *check.C, clusterID string) (*sql.DB, *sql.Conn) { + ctx := context.Background() + db, err := sql.Open("postgres", s.testClusters[clusterID].super.Cluster().PostgreSQL.Connection.String()) + c.Assert(err, check.IsNil) + + conn, err := db.Conn(ctx) + c.Assert(err, check.IsNil) + + rows, err := conn.ExecContext(ctx, `SELECT 1`) + c.Assert(err, check.IsNil) + n, err := rows.RowsAffected() + c.Assert(err, check.IsNil) + c.Assert(n, check.Equals, int64(1)) + return db, conn +} + +// TestRuntimeTokenInCR will test several different tokens in the runtime attribute +// and check the expected results accessing directly to the database if needed. +func (s *IntegrationSuite) TestRuntimeTokenInCR(c *check.C) { + db, dbconn := s.dbConn(c, "z1111") + defer db.Close() + defer dbconn.Close() + conn1 := s.conn("z1111") + rootctx1, _, _ := s.rootClients("z1111") + userctx1, ac1, _, au := s.userClients(rootctx1, c, conn1, "z1111", true) + + tests := []struct { + name string + token string + expectAToGetAValidCR bool + expectedToken *string + }{ + {"Good token z1111 user", ac1.AuthToken, true, &ac1.AuthToken}, + {"Bogus token", "abcdef", false, nil}, + {"v1-looking token", "badtoken00badtoken00badtoken00badtoken00b", false, nil}, + {"v2-looking token", "v2/" + au.UUID + "/badtoken00badtoken00badtoken00badtoken00b", false, nil}, + } + + for _, tt := range tests { + c.Log(c.TestName() + " " + tt.name) + + rq := map[string]interface{}{ + "command": []string{"echo"}, + "container_image": "d41d8cd98f00b204e9800998ecf8427e+0", + "cwd": "/", + "output_path": "/", + "runtime_token": tt.token, + } + cr, err := conn1.ContainerRequestCreate(userctx1, arvados.CreateOptions{Attrs: rq}) + if tt.expectAToGetAValidCR { + c.Check(err, check.IsNil) + c.Check(cr, check.NotNil) + c.Check(cr.UUID, check.Not(check.Equals), "") + } + + if tt.expectedToken == nil { + continue + } + + c.Logf("cr.UUID: %s", cr.UUID) + row := dbconn.QueryRowContext(rootctx1, `SELECT runtime_token from container_requests where uuid=$1`, cr.UUID) + c.Check(row, check.NotNil) + var token sql.NullString + row.Scan(&token) + if c.Check(token.Valid, check.Equals, true) { + c.Check(token.String, check.Equals, *tt.expectedToken) + } + } +} + +// TestIntermediateCluster will send a container request to +// one cluster with another cluster as the destination +// and check the tokens are being handled properly +func (s *IntegrationSuite) TestIntermediateCluster(c *check.C) { + conn1 := s.conn("z1111") + rootctx1, _, _ := s.rootClients("z1111") + uctx1, ac1, _, _ := s.userClients(rootctx1, c, conn1, "z1111", true) + + tests := []struct { + name string + token string + expectedRuntimeToken string + expectedUUIDprefix string + }{ + {"Good token z1111 user sending a CR to z2222", ac1.AuthToken, "", "z2222-xvhdp-"}, + } + + for _, tt := range tests { + c.Log(c.TestName() + " " + tt.name) + rq := map[string]interface{}{ + "command": []string{"echo"}, + "container_image": "d41d8cd98f00b204e9800998ecf8427e+0", + "cwd": "/", + "output_path": "/", + "runtime_token": tt.token, + } + cr, err := conn1.ContainerRequestCreate(uctx1, arvados.CreateOptions{ClusterID: "z2222", Attrs: rq}) + c.Check(err, check.IsNil) - c.Check(cr.UUID, check.Matches, "z2222-.*") + c.Check(strings.HasPrefix(cr.UUID, tt.expectedUUIDprefix), check.Equals, true) + c.Check(cr.RuntimeToken, check.Equals, tt.expectedRuntimeToken) } } diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go index 4f0035edf9..d197675f8d 100644 --- a/lib/controller/localdb/conn.go +++ b/lib/controller/localdb/conn.go @@ -24,21 +24,24 @@ func NewConn(cluster *arvados.Cluster) *Conn { railsProxy := railsproxy.NewConn(cluster) var conn Conn conn = Conn{ - cluster: cluster, - railsProxy: railsProxy, - loginController: chooseLoginController(cluster, railsProxy), + cluster: cluster, + railsProxy: railsProxy, } + conn.loginController = chooseLoginController(cluster, &conn) return &conn } +// Logout handles the logout of conn giving to the appropriate loginController func (conn *Conn) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { return conn.loginController.Logout(ctx, opts) } +// Login handles the login of conn giving to the appropriate loginController func (conn *Conn) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) { return conn.loginController.Login(ctx, opts) } +// UserAuthenticate handles the User Authentication of conn giving to the appropriate loginController func (conn *Conn) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { return conn.loginController.UserAuthenticate(ctx, opts) } diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go index f4632751e3..4bf515fc3f 100644 --- a/lib/controller/localdb/login.go +++ b/lib/controller/localdb/login.go @@ -27,7 +27,7 @@ type loginController interface { UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) } -func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) loginController { +func chooseLoginController(cluster *arvados.Cluster, parent *Conn) loginController { wantGoogle := cluster.Login.Google.Enable wantOpenIDConnect := cluster.Login.OpenIDConnect.Enable wantSSO := cluster.Login.SSO.Enable @@ -43,7 +43,7 @@ func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) log case wantGoogle: return &oidcLoginController{ Cluster: cluster, - RailsProxy: railsProxy, + Parent: parent, Issuer: "https://accounts.google.com", ClientID: cluster.Login.Google.ClientID, ClientSecret: cluster.Login.Google.ClientSecret, @@ -54,7 +54,7 @@ func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) log case wantOpenIDConnect: return &oidcLoginController{ Cluster: cluster, - RailsProxy: railsProxy, + Parent: parent, Issuer: cluster.Login.OpenIDConnect.Issuer, ClientID: cluster.Login.OpenIDConnect.ClientID, ClientSecret: cluster.Login.OpenIDConnect.ClientSecret, @@ -63,13 +63,13 @@ func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) log UsernameClaim: cluster.Login.OpenIDConnect.UsernameClaim, } case wantSSO: - return &ssoLoginController{railsProxy} + return &ssoLoginController{Parent: parent} case wantPAM: - return &pamLoginController{Cluster: cluster, RailsProxy: railsProxy} + return &pamLoginController{Cluster: cluster, Parent: parent} case wantLDAP: - return &ldapLoginController{Cluster: cluster, RailsProxy: railsProxy} + return &ldapLoginController{Cluster: cluster, Parent: parent} case wantTest: - return &testLoginController{Cluster: cluster, RailsProxy: railsProxy} + return &testLoginController{Cluster: cluster, Parent: parent} case wantLoginCluster: return &federatedLoginController{Cluster: cluster} default: @@ -89,10 +89,16 @@ func countTrue(vals ...bool) int { return n } -// Login and Logout are passed through to the wrapped railsProxy; +// Login and Logout are passed through to the parent's railsProxy; // UserAuthenticate is rejected. -type ssoLoginController struct{ *railsProxy } +type ssoLoginController struct{ Parent *Conn } +func (ctrl *ssoLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) { + return ctrl.Parent.railsProxy.Login(ctx, opts) +} +func (ctrl *ssoLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { + return ctrl.Parent.railsProxy.Logout(ctx, opts) +} func (ctrl *ssoLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New("username/password authentication is not available"), http.StatusBadRequest) } @@ -135,9 +141,12 @@ func noopLogout(cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.L return arvados.LogoutResponse{RedirectLocation: target}, nil } -func createAPIClientAuthorization(ctx context.Context, conn *rpc.Conn, rootToken string, authinfo rpc.UserSessionAuthInfo) (resp arvados.APIClientAuthorization, err error) { +func (conn *Conn) CreateAPIClientAuthorization(ctx context.Context, rootToken string, authinfo rpc.UserSessionAuthInfo) (resp arvados.APIClientAuthorization, err error) { + if rootToken == "" { + return arvados.APIClientAuthorization{}, errors.New("configuration error: empty SystemRootToken") + } ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{rootToken}}) - newsession, err := conn.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ + newsession, err := conn.railsProxy.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ // Send a fake ReturnTo value instead of the caller's // opts.ReturnTo. We won't follow the resulting // redirect target anyway. diff --git a/lib/controller/localdb/login_ldap.go b/lib/controller/localdb/login_ldap.go index 6c430d69bb..49f557ae5b 100644 --- a/lib/controller/localdb/login_ldap.go +++ b/lib/controller/localdb/login_ldap.go @@ -21,8 +21,8 @@ import ( ) type ldapLoginController struct { - Cluster *arvados.Cluster - RailsProxy *railsProxy + Cluster *arvados.Cluster + Parent *Conn } func (ctrl *ldapLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { @@ -143,7 +143,7 @@ func (ctrl *ldapLoginController) UserAuthenticate(ctx context.Context, opts arva return arvados.APIClientAuthorization{}, errors.New("authentication succeeded but ldap returned no email address") } - return createAPIClientAuthorization(ctx, ctrl.RailsProxy, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ + return ctrl.Parent.CreateAPIClientAuthorization(ctx, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ Email: email, FirstName: attrs["givenname"], LastName: attrs["sn"], diff --git a/lib/controller/localdb/login_ldap_test.go b/lib/controller/localdb/login_ldap_test.go index bce1ecfcf2..b8ba6b4676 100644 --- a/lib/controller/localdb/login_ldap_test.go +++ b/lib/controller/localdb/login_ldap_test.go @@ -90,8 +90,8 @@ func (s *LDAPSuite) SetUpSuite(c *check.C) { s.cluster.Login.LDAP.SearchBase = "dc=example,dc=com" c.Assert(err, check.IsNil) s.ctrl = &ldapLoginController{ - Cluster: s.cluster, - RailsProxy: railsproxy.NewConn(s.cluster), + Cluster: s.cluster, + Parent: &Conn{railsProxy: railsproxy.NewConn(s.cluster)}, } s.db = arvadostest.DB(c, s.cluster) } diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go index 5f96da5624..b99a1c2aa5 100644 --- a/lib/controller/localdb/login_oidc.go +++ b/lib/controller/localdb/login_oidc.go @@ -22,7 +22,6 @@ import ( "time" "git.arvados.org/arvados.git/lib/controller/api" - "git.arvados.org/arvados.git/lib/controller/railsproxy" "git.arvados.org/arvados.git/lib/controller/rpc" "git.arvados.org/arvados.git/lib/ctrlctx" "git.arvados.org/arvados.git/sdk/go/arvados" @@ -46,7 +45,7 @@ const ( type oidcLoginController struct { Cluster *arvados.Cluster - RailsProxy *railsProxy + Parent *Conn Issuer string // OIDC issuer URL, e.g., "https://accounts.google.com" ClientID string ClientSecret string @@ -143,7 +142,7 @@ func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOp return loginError(err) } ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{ctrl.Cluster.SystemRootToken}}) - return ctrl.RailsProxy.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ + return ctrl.Parent.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ ReturnTo: state.Remote + "," + state.ReturnTo, AuthInfo: *authinfo, }) @@ -322,7 +321,7 @@ func OIDCAccessTokenAuthorizer(cluster *arvados.Cluster, getdb func(context.Cont // We want ctrl to be nil if the chosen controller is not a // *oidcLoginController, so we can ignore the 2nd return value // of this type cast. - ctrl, _ := chooseLoginController(cluster, railsproxy.NewConn(cluster)).(*oidcLoginController) + ctrl, _ := NewConn(cluster).loginController.(*oidcLoginController) cache, err := lru.New2Q(tokenCacheSize) if err != nil { panic(err) @@ -474,7 +473,7 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er } ctxlog.FromContext(ctx).WithField("HMAC", hmac).Debug("(*oidcTokenAuthorizer)registerToken: updated api_client_authorizations row") } else { - aca, err = createAPIClientAuthorization(ctx, ta.ctrl.RailsProxy, ta.ctrl.Cluster.SystemRootToken, *authinfo) + aca, err = ta.ctrl.Parent.CreateAPIClientAuthorization(ctx, ta.ctrl.Cluster.SystemRootToken, *authinfo) if err != nil { return err } diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go index 2447713a2c..5d116a9e8f 100644 --- a/lib/controller/localdb/login_pam.go +++ b/lib/controller/localdb/login_pam.go @@ -20,8 +20,8 @@ import ( ) type pamLoginController struct { - Cluster *arvados.Cluster - RailsProxy *railsProxy + Cluster *arvados.Cluster + Parent *Conn } func (ctrl *pamLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { @@ -87,7 +87,7 @@ func (ctrl *pamLoginController) UserAuthenticate(ctx context.Context, opts arvad "user": user, "email": email, }).Debug("pam authentication succeeded") - return createAPIClientAuthorization(ctx, ctrl.RailsProxy, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ + return ctrl.Parent.CreateAPIClientAuthorization(ctx, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ Username: user, Email: email, }) diff --git a/lib/controller/localdb/login_pam_test.go b/lib/controller/localdb/login_pam_test.go index e6b967c944..c5876bbfad 100644 --- a/lib/controller/localdb/login_pam_test.go +++ b/lib/controller/localdb/login_pam_test.go @@ -36,8 +36,8 @@ func (s *PamSuite) SetUpSuite(c *check.C) { s.cluster.Login.PAM.DefaultEmailDomain = "example.com" s.railsSpy = arvadostest.NewProxy(c, s.cluster.Services.RailsAPI) s.ctrl = &pamLoginController{ - Cluster: s.cluster, - RailsProxy: rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider), + Cluster: s.cluster, + Parent: &Conn{railsProxy: rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider)}, } } diff --git a/lib/controller/localdb/login_testuser.go b/lib/controller/localdb/login_testuser.go index 5852273529..c567a06683 100644 --- a/lib/controller/localdb/login_testuser.go +++ b/lib/controller/localdb/login_testuser.go @@ -17,8 +17,8 @@ import ( ) type testLoginController struct { - Cluster *arvados.Cluster - RailsProxy *railsProxy + Cluster *arvados.Cluster + Parent *Conn } func (ctrl *testLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { @@ -45,7 +45,7 @@ func (ctrl *testLoginController) UserAuthenticate(ctx context.Context, opts arva "username": username, "email": user.Email, }).Debug("test authentication succeeded") - return createAPIClientAuthorization(ctx, ctrl.RailsProxy, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ + return ctrl.Parent.CreateAPIClientAuthorization(ctx, ctrl.Cluster.SystemRootToken, rpc.UserSessionAuthInfo{ Username: username, Email: user.Email, }) diff --git a/lib/controller/localdb/login_testuser_test.go b/lib/controller/localdb/login_testuser_test.go index 7589088899..7a520428b6 100644 --- a/lib/controller/localdb/login_testuser_test.go +++ b/lib/controller/localdb/login_testuser_test.go @@ -41,8 +41,8 @@ func (s *TestUserSuite) SetUpSuite(c *check.C) { } s.railsSpy = arvadostest.NewProxy(c, s.cluster.Services.RailsAPI) s.ctrl = &testLoginController{ - Cluster: s.cluster, - RailsProxy: rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider), + Cluster: s.cluster, + Parent: &Conn{railsProxy: rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider)}, } s.db = arvadostest.DB(c, s.cluster) } diff --git a/lib/controller/router/response.go b/lib/controller/router/response.go index 543e25d0ce..d554ab930f 100644 --- a/lib/controller/router/response.go +++ b/lib/controller/router/response.go @@ -97,38 +97,18 @@ func (rtr *router) sendResponse(w http.ResponseWriter, req *http.Request, resp i } else if defaultItemKind != "" { item["kind"] = defaultItemKind } - items[i] = applySelectParam(opts.Select, item) + item = applySelectParam(opts.Select, item) + rtr.mungeItemFields(item) + items[i] = item } if opts.Count == "none" { delete(tmp, "items_available") } } else { tmp = applySelectParam(opts.Select, tmp) + rtr.mungeItemFields(tmp) } - // Format non-nil timestamps as rfc3339NanoFixed (by default - // they will have been encoded to time.RFC3339Nano, which - // omits trailing zeroes). - for k, v := range tmp { - if !strings.HasSuffix(k, "_at") { - continue - } - switch tv := v.(type) { - case *time.Time: - if tv == nil { - break - } - tmp[k] = tv.Format(rfc3339NanoFixed) - case time.Time: - tmp[k] = tv.Format(rfc3339NanoFixed) - case string: - t, err := time.Parse(time.RFC3339Nano, tv) - if err != nil { - break - } - tmp[k] = t.Format(rfc3339NanoFixed) - } - } w.Header().Set("Content-Type", "application/json") enc := json.NewEncoder(w) enc.SetEscapeHTML(false) @@ -160,3 +140,51 @@ func kind(resp interface{}) string { return "#" + strings.ToLower(s[1:]) }) } + +func (rtr *router) mungeItemFields(tmp map[string]interface{}) { + for k, v := range tmp { + if strings.HasSuffix(k, "_at") { + // Format non-nil timestamps as + // rfc3339NanoFixed (otherwise they would use + // the default time encoding, which omits + // trailing zeroes). + switch tv := v.(type) { + case *time.Time: + if tv == nil || tv.IsZero() { + tmp[k] = nil + } else { + tmp[k] = tv.Format(rfc3339NanoFixed) + } + case time.Time: + if tv.IsZero() { + tmp[k] = nil + } else { + tmp[k] = tv.Format(rfc3339NanoFixed) + } + case string: + if tv == "" { + tmp[k] = nil + } else if t, err := time.Parse(time.RFC3339Nano, tv); err != nil { + // pass through an invalid time value (?) + } else if t.IsZero() { + tmp[k] = nil + } else { + tmp[k] = t.Format(rfc3339NanoFixed) + } + } + } + // Arvados API spec says when these fields are empty + // they appear in responses as null, rather than a + // zero value. + switch k { + case "output_uuid", "output_name", "log_uuid", "description", "requesting_container_uuid", "container_uuid": + if v == "" { + tmp[k] = nil + } + case "container_count_max": + if v == float64(0) { + tmp[k] = nil + } + } + } +} diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go index 2944524344..9fb2a0d32b 100644 --- a/lib/controller/router/router.go +++ b/lib/controller/router/router.go @@ -168,6 +168,41 @@ func (rtr *router) addRoutes() { return rtr.backend.ContainerDelete(ctx, *opts.(*arvados.DeleteOptions)) }, }, + { + arvados.EndpointContainerRequestCreate, + func() interface{} { return &arvados.CreateOptions{} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.backend.ContainerRequestCreate(ctx, *opts.(*arvados.CreateOptions)) + }, + }, + { + arvados.EndpointContainerRequestUpdate, + func() interface{} { return &arvados.UpdateOptions{} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.backend.ContainerRequestUpdate(ctx, *opts.(*arvados.UpdateOptions)) + }, + }, + { + arvados.EndpointContainerRequestGet, + func() interface{} { return &arvados.GetOptions{} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.backend.ContainerRequestGet(ctx, *opts.(*arvados.GetOptions)) + }, + }, + { + arvados.EndpointContainerRequestList, + func() interface{} { return &arvados.ListOptions{Limit: -1} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.backend.ContainerRequestList(ctx, *opts.(*arvados.ListOptions)) + }, + }, + { + arvados.EndpointContainerRequestDelete, + func() interface{} { return &arvados.DeleteOptions{} }, + func(ctx context.Context, opts interface{}) (interface{}, error) { + return rtr.backend.ContainerRequestDelete(ctx, *opts.(*arvados.DeleteOptions)) + }, + }, { arvados.EndpointContainerLock, func() interface{} { diff --git a/lib/controller/rpc/conn.go b/lib/controller/rpc/conn.go index cd98b64718..455c20fa91 100644 --- a/lib/controller/rpc/conn.go +++ b/lib/controller/rpc/conn.go @@ -23,6 +23,8 @@ import ( "git.arvados.org/arvados.git/sdk/go/auth" ) +const rfc3339NanoFixed = "2006-01-02T15:04:05.000000000Z07:00" + type TokenProvider func(context.Context) ([]string, error) func PassthroughTokenProvider(ctx context.Context) ([]string, error) { @@ -120,6 +122,20 @@ func (conn *Conn) requestAndDecode(ctx context.Context, dst interface{}, ep arva delete(params, "limit") } } + + if authinfo, ok := params["auth_info"]; ok { + if tmp, ok2 := authinfo.(map[string]interface{}); ok2 { + for k, v := range tmp { + if strings.HasSuffix(k, "_at") { + // Change zero times values to nil + if v, ok3 := v.(string); ok3 && (strings.HasPrefix(v, "0001-01-01T00:00:00") || v == "") { + tmp[k] = nil + } + } + } + } + } + if len(tokens) > 1 { params["reader_tokens"] = tokens[1:] } @@ -286,6 +302,41 @@ func (conn *Conn) ContainerUnlock(ctx context.Context, options arvados.GetOption return resp, err } +func (conn *Conn) ContainerRequestCreate(ctx context.Context, options arvados.CreateOptions) (arvados.ContainerRequest, error) { + ep := arvados.EndpointContainerRequestCreate + var resp arvados.ContainerRequest + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} + +func (conn *Conn) ContainerRequestUpdate(ctx context.Context, options arvados.UpdateOptions) (arvados.ContainerRequest, error) { + ep := arvados.EndpointContainerRequestUpdate + var resp arvados.ContainerRequest + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} + +func (conn *Conn) ContainerRequestGet(ctx context.Context, options arvados.GetOptions) (arvados.ContainerRequest, error) { + ep := arvados.EndpointContainerRequestGet + var resp arvados.ContainerRequest + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} + +func (conn *Conn) ContainerRequestList(ctx context.Context, options arvados.ListOptions) (arvados.ContainerRequestList, error) { + ep := arvados.EndpointContainerRequestList + var resp arvados.ContainerRequestList + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} + +func (conn *Conn) ContainerRequestDelete(ctx context.Context, options arvados.DeleteOptions) (arvados.ContainerRequest, error) { + ep := arvados.EndpointContainerRequestDelete + var resp arvados.ContainerRequest + err := conn.requestAndDecode(ctx, &resp, ep, nil, options) + return resp, err +} + func (conn *Conn) SpecimenCreate(ctx context.Context, options arvados.CreateOptions) (arvados.Specimen, error) { ep := arvados.EndpointSpecimenCreate var resp arvados.Specimen @@ -402,11 +453,13 @@ func (conn *Conn) APIClientAuthorizationCurrent(ctx context.Context, options arv } type UserSessionAuthInfo struct { - Email string `json:"email"` - AlternateEmails []string `json:"alternate_emails"` - FirstName string `json:"first_name"` - LastName string `json:"last_name"` - Username string `json:"username"` + UserUUID string `json:"user_uuid"` + Email string `json:"email"` + AlternateEmails []string `json:"alternate_emails"` + FirstName string `json:"first_name"` + LastName string `json:"last_name"` + Username string `json:"username"` + ExpiresAt time.Time `json:"expires_at"` } type UserSessionCreateOptions struct { @@ -416,6 +469,7 @@ type UserSessionCreateOptions struct { func (conn *Conn) UserSessionCreate(ctx context.Context, options UserSessionCreateOptions) (arvados.LoginResponse, error) { ep := arvados.APIEndpoint{Method: "POST", Path: "auth/controller/callback"} + // if ExpiresAt is empty value then add 2 hour expiration var resp arvados.LoginResponse err := conn.requestAndDecode(ctx, &resp, ep, nil, options) return resp, err diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go index 341938354c..730185c196 100644 --- a/lib/crunchrun/crunchrun.go +++ b/lib/crunchrun/crunchrun.go @@ -621,7 +621,7 @@ func (runner *ContainerRunner) SetupMounts() (err error) { return fmt.Errorf("output path does not correspond to a writable mount point") } - if wantAPI := runner.Container.RuntimeConstraints.API; needCertMount && wantAPI != nil && *wantAPI { + if needCertMount && runner.Container.RuntimeConstraints.API { for _, certfile := range arvadosclient.CertFiles { _, err := os.Stat(certfile) if err == nil { @@ -1092,7 +1092,7 @@ func (runner *ContainerRunner) CreateContainer() error { }, } - if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI != nil && *wantAPI { + if runner.Container.RuntimeConstraints.API { tok, err := runner.ContainerToken() if err != nil { return err @@ -1269,7 +1269,7 @@ func (runner *ContainerRunner) updateLogs() { // CaptureOutput saves data from the container's output directory if // needed, and updates the container output accordingly. func (runner *ContainerRunner) CaptureOutput() error { - if wantAPI := runner.Container.RuntimeConstraints.API; wantAPI != nil && *wantAPI { + if runner.Container.RuntimeConstraints.API { // Output may have been set directly by the container, so // refresh the container record to check. err := runner.DispatcherArvClient.Get("containers", runner.Container.UUID, diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go index eb83bbd410..dbdaa6293d 100644 --- a/lib/crunchrun/crunchrun_test.go +++ b/lib/crunchrun/crunchrun_test.go @@ -1291,9 +1291,7 @@ func (s *TestSuite) TestSetupMounts(c *C) { cr.Container.Mounts = make(map[string]arvados.Mount) cr.Container.Mounts["/tmp"] = arvados.Mount{Kind: "tmp"} cr.Container.OutputPath = "/tmp" - - apiflag := true - cr.Container.RuntimeConstraints.API = &apiflag + cr.Container.RuntimeConstraints.API = true err := cr.SetupMounts() c.Check(err, IsNil) @@ -1305,7 +1303,7 @@ func (s *TestSuite) TestSetupMounts(c *C) { cr.CleanupDirs() checkEmpty() - apiflag = false + cr.Container.RuntimeConstraints.API = false } { diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go index 5a2cfb8800..a11872971a 100644 --- a/sdk/go/arvados/api.go +++ b/sdk/go/arvados/api.go @@ -41,6 +41,11 @@ var ( EndpointContainerDelete = APIEndpoint{"DELETE", "arvados/v1/containers/{uuid}", ""} EndpointContainerLock = APIEndpoint{"POST", "arvados/v1/containers/{uuid}/lock", ""} EndpointContainerUnlock = APIEndpoint{"POST", "arvados/v1/containers/{uuid}/unlock", ""} + EndpointContainerRequestCreate = APIEndpoint{"POST", "arvados/v1/container_requests", "container_request"} + EndpointContainerRequestUpdate = APIEndpoint{"PATCH", "arvados/v1/container_requests/{uuid}", "container_request"} + EndpointContainerRequestGet = APIEndpoint{"GET", "arvados/v1/container_requests/{uuid}", ""} + EndpointContainerRequestList = APIEndpoint{"GET", "arvados/v1/container_requests", ""} + EndpointContainerRequestDelete = APIEndpoint{"DELETE", "arvados/v1/container_requests/{uuid}", ""} EndpointUserActivate = APIEndpoint{"POST", "arvados/v1/users/{uuid}/activate", ""} EndpointUserCreate = APIEndpoint{"POST", "arvados/v1/users", "user"} EndpointUserCurrent = APIEndpoint{"GET", "arvados/v1/users/current", ""} @@ -175,6 +180,11 @@ type API interface { ContainerDelete(ctx context.Context, options DeleteOptions) (Container, error) ContainerLock(ctx context.Context, options GetOptions) (Container, error) ContainerUnlock(ctx context.Context, options GetOptions) (Container, error) + ContainerRequestCreate(ctx context.Context, options CreateOptions) (ContainerRequest, error) + ContainerRequestUpdate(ctx context.Context, options UpdateOptions) (ContainerRequest, error) + ContainerRequestGet(ctx context.Context, options GetOptions) (ContainerRequest, error) + ContainerRequestList(ctx context.Context, options ListOptions) (ContainerRequestList, error) + ContainerRequestDelete(ctx context.Context, options DeleteOptions) (ContainerRequest, error) SpecimenCreate(ctx context.Context, options CreateOptions) (Specimen, error) SpecimenUpdate(ctx context.Context, options UpdateOptions) (Specimen, error) SpecimenGet(ctx context.Context, options GetOptions) (Specimen, error) diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go index 265944e81d..3ff7c52055 100644 --- a/sdk/go/arvados/container.go +++ b/sdk/go/arvados/container.go @@ -65,6 +65,9 @@ type ContainerRequest struct { LogUUID string `json:"log_uuid"` OutputUUID string `json:"output_uuid"` RuntimeToken string `json:"runtime_token"` + ExpiresAt time.Time `json:"expires_at"` + Filters []Filter `json:"filters"` + ContainerCount int `json:"container_count"` } // Mount is special behavior to attach to a filesystem path or device. @@ -86,7 +89,7 @@ type Mount struct { // RuntimeConstraints specify a container's compute resources (RAM, // CPU) and network connectivity. type RuntimeConstraints struct { - API *bool + API bool `json:"api"` RAM int64 `json:"ram"` VCPUs int `json:"vcpus"` KeepCacheRAM int64 `json:"keep_cache_ram"` diff --git a/sdk/go/arvadostest/api.go b/sdk/go/arvadostest/api.go index 039d7ae116..df3e46febd 100644 --- a/sdk/go/arvadostest/api.go +++ b/sdk/go/arvadostest/api.go @@ -105,6 +105,26 @@ func (as *APIStub) ContainerUnlock(ctx context.Context, options arvados.GetOptio as.appendCall(ctx, as.ContainerUnlock, options) return arvados.Container{}, as.Error } +func (as *APIStub) ContainerRequestCreate(ctx context.Context, options arvados.CreateOptions) (arvados.ContainerRequest, error) { + as.appendCall(ctx, as.ContainerRequestCreate, options) + return arvados.ContainerRequest{}, as.Error +} +func (as *APIStub) ContainerRequestUpdate(ctx context.Context, options arvados.UpdateOptions) (arvados.ContainerRequest, error) { + as.appendCall(ctx, as.ContainerRequestUpdate, options) + return arvados.ContainerRequest{}, as.Error +} +func (as *APIStub) ContainerRequestGet(ctx context.Context, options arvados.GetOptions) (arvados.ContainerRequest, error) { + as.appendCall(ctx, as.ContainerRequestGet, options) + return arvados.ContainerRequest{}, as.Error +} +func (as *APIStub) ContainerRequestList(ctx context.Context, options arvados.ListOptions) (arvados.ContainerRequestList, error) { + as.appendCall(ctx, as.ContainerRequestList, options) + return arvados.ContainerRequestList{}, as.Error +} +func (as *APIStub) ContainerRequestDelete(ctx context.Context, options arvados.DeleteOptions) (arvados.ContainerRequest, error) { + as.appendCall(ctx, as.ContainerRequestDelete, options) + return arvados.ContainerRequest{}, as.Error +} func (as *APIStub) SpecimenCreate(ctx context.Context, options arvados.CreateOptions) (arvados.Specimen, error) { as.appendCall(ctx, as.SpecimenCreate, options) return arvados.Specimen{}, as.Error diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb index 8e3c3ac5e3..3d109c49ca 100644 --- a/services/api/app/controllers/user_sessions_controller.rb +++ b/services/api/app/controllers/user_sessions_controller.rb @@ -17,6 +17,7 @@ class UserSessionsController < ApplicationController raise "Local login disabled when LoginCluster is set" end + max_expires_at = nil if params[:provider] == 'controller' if request.headers['Authorization'] != 'Bearer ' + Rails.configuration.SystemRootToken return send_error('Invalid authorization header', status: 401) @@ -24,18 +25,27 @@ class UserSessionsController < ApplicationController # arvados-controller verified the user and is passing auth_info # in request params. authinfo = SafeJSON.load(params[:auth_info]) + max_expires_at = authinfo["expires_at"] else # omniauth middleware verified the user and is passing auth_info # in request.env. authinfo = request.env['omniauth.auth']['info'].with_indifferent_access end - begin - user = User.register(authinfo) - rescue => e - Rails.logger.warn "User.register error #{e}" - Rails.logger.warn "authinfo was #{authinfo.inspect}" - return redirect_to login_failure_url + if !authinfo['user_uuid'].blank? + user = User.find_by_uuid(authinfo['user_uuid']) + if !user + Rails.logger.warn "Nonexistent user_uuid in authinfo #{authinfo.inspect}" + return redirect_to login_failure_url + end + else + begin + user = User.register(authinfo) + rescue => e + Rails.logger.warn "User.register error #{e}" + Rails.logger.warn "authinfo was #{authinfo.inspect}" + return redirect_to login_failure_url + end end # For the benefit of functional and integration tests: @@ -72,7 +82,7 @@ class UserSessionsController < ApplicationController return send_error 'Invalid remote cluster id', status: 400 end remote = nil if remote == '' - return send_api_token_to(return_to_url, user, remote) + return send_api_token_to(return_to_url, user, remote, max_expires_at) end redirect_to @redirect_to end @@ -106,7 +116,7 @@ class UserSessionsController < ApplicationController # FIXME: if current_user has never authorized this app before, # ask for confirmation here! - return send_api_token_to(params[:return_to], current_user, params[:remote]) + return send_api_token_to(params[:return_to], current_user, params[:remote], params[:expires_at]) end p = [] p << "auth_provider=#{CGI.escape(params[:auth_provider])}" if params[:auth_provider] @@ -136,7 +146,7 @@ class UserSessionsController < ApplicationController end end - def send_api_token_to(callback_url, user, remote=nil) + def send_api_token_to(callback_url, user, remote=nil, token_expiration=nil) # Give the API client a token for making API calls on behalf of # the authenticated user @@ -146,11 +156,14 @@ class UserSessionsController < ApplicationController @api_client = ApiClient. find_or_create_by(url_prefix: api_client_url_prefix) end - - token_expiration = nil if Rails.configuration.Login.TokenLifetime > 0 - token_expiration = Time.now + Rails.configuration.Login.TokenLifetime + if token_expiration == nil + token_expiration = Time.now + Rails.configuration.Login.TokenLifetime + else + token_expiration = [token_expiration, Time.now + Rails.configuration.Login.TokenLifetime].min + end end + @api_client_auth = ApiClientAuthorization. new(user: user, api_client: @api_client, diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 6a0a58f08d..7d5bea8fae 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -855,6 +855,40 @@ class ArvadosModel < ApplicationRecord nil end + # Fill in implied zero/false values in database records that were + # created before #17014 made them explicit, and reset the Rails + # "changed" state so the record doesn't appear to have been modified + # after loading. + # + # Invoked by Container and ContainerRequest models as an after_find + # hook. + def fill_container_defaults_after_find + fill_container_defaults + set_attribute_was('runtime_constraints', runtime_constraints) + set_attribute_was('scheduling_parameters', scheduling_parameters) + clear_changes_information + end + + # Fill in implied zero/false values. Invoked by ContainerRequest as + # a before_validation hook in order to (a) ensure every key has a + # value in the updated database record and (b) ensure the attribute + # whitelist doesn't reject a change from an explicit zero/false + # value in the database to an implicit zero/false value in an update + # request. + def fill_container_defaults + self.runtime_constraints = { + 'api' => false, + 'keep_cache_ram' => 0, + 'ram' => 0, + 'vcpus' => 0, + }.merge(attributes['runtime_constraints'] || {}) + self.scheduling_parameters = { + 'max_run_time' => 0, + 'partitions' => [], + 'preemptible' => false, + }.merge(attributes['scheduling_parameters'] || {}) + end + # ArvadosModel.find_by_uuid needs extra magic to allow it to return # an object in any class. def self.find_by_uuid uuid diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index 5833c2251f..d01787cbc7 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -29,6 +29,7 @@ class Container < ArvadosModel serialize :command, Array serialize :scheduling_parameters, Hash + after_find :fill_container_defaults_after_find before_validation :fill_field_defaults, :if => :new_record? before_validation :set_timestamps before_validation :check_lock @@ -207,17 +208,16 @@ class Container < ArvadosModel # containers are suitable). def self.resolve_runtime_constraints(runtime_constraints) rc = {} - defaults = { - 'keep_cache_ram' => - Rails.configuration.Containers.DefaultKeepCacheRAM, - } - defaults.merge(runtime_constraints).each do |k, v| + runtime_constraints.each do |k, v| if v.is_a? Array rc[k] = v[0] else rc[k] = v end end + if rc['keep_cache_ram'] == 0 + rc['keep_cache_ram'] = Rails.configuration.Containers.DefaultKeepCacheRAM + end rc end diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb index 77536eee4f..837f2cdc70 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -30,14 +30,16 @@ class ContainerRequest < ArvadosModel serialize :command, Array serialize :scheduling_parameters, Hash + after_find :fill_container_defaults_after_find before_validation :fill_field_defaults, :if => :new_record? - before_validation :validate_runtime_constraints + before_validation :fill_container_defaults before_validation :set_default_preemptible_scheduling_parameter before_validation :set_container validates :command, :container_image, :output_path, :cwd, :presence => true validates :output_ttl, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 1000 } validate :validate_datatypes + validate :validate_runtime_constraints validate :validate_scheduling_parameters validate :validate_state_change validate :check_update_whitelist @@ -194,7 +196,7 @@ class ContainerRequest < ArvadosModel coll_name = "Container #{out_type} for request #{uuid}" trash_at = nil if out_type == 'output' - if self.output_name + if self.output_name and self.output_name != "" coll_name = self.output_name end if self.output_ttl > 0 @@ -312,28 +314,17 @@ class ContainerRequest < ArvadosModel end def set_default_preemptible_scheduling_parameter - c = get_requesting_container() - if self.state == Committed - # If preemptible instances (eg: AWS Spot Instances) are allowed, - # ask them on child containers by default. - if Rails.configuration.Containers.UsePreemptibleInstances and !c.nil? and - self.scheduling_parameters['preemptible'].nil? - self.scheduling_parameters['preemptible'] = true - end + if Rails.configuration.Containers.UsePreemptibleInstances && state == Committed && get_requesting_container() + self.scheduling_parameters['preemptible'] = true end end def validate_runtime_constraints case self.state when Committed - [['vcpus', true], - ['ram', true], - ['keep_cache_ram', false]].each do |k, required| - if !required && !runtime_constraints.include?(k) - next - end + ['vcpus', 'ram'].each do |k| v = runtime_constraints[k] - unless (v.is_a?(Integer) && v > 0) + if !v.is_a?(Integer) || v <= 0 errors.add(:runtime_constraints, "[#{k}]=#{v.inspect} must be a positive integer") end diff --git a/services/api/test/fixtures/container_requests.yml b/services/api/test/fixtures/container_requests.yml index ab0400a678..1c62605029 100644 --- a/services/api/test/fixtures/container_requests.yml +++ b/services/api/test/fixtures/container_requests.yml @@ -20,6 +20,7 @@ queued: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} running: uuid: zzzzz-xvhdp-cr4runningcntnr @@ -39,6 +40,7 @@ running: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} requester_for_running: uuid: zzzzz-xvhdp-req4runningcntr @@ -59,6 +61,7 @@ requester_for_running: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} running_older: uuid: zzzzz-xvhdp-cr4runningcntn2 @@ -78,6 +81,7 @@ running_older: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} completed: uuid: zzzzz-xvhdp-cr4completedctr @@ -99,6 +103,7 @@ completed: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} completed-older: uuid: zzzzz-xvhdp-cr4completedcr2 @@ -120,6 +125,7 @@ completed-older: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} completed_diagnostics: name: CWL diagnostics hasher @@ -365,6 +371,7 @@ requester: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} cr_for_requester: uuid: zzzzz-xvhdp-cr4requestercnt @@ -385,6 +392,7 @@ cr_for_requester: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} cr_for_requester2: uuid: zzzzz-xvhdp-cr4requestercn2 @@ -404,6 +412,7 @@ cr_for_requester2: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} running_anonymous_accessible: uuid: zzzzz-xvhdp-runninganonaccs @@ -423,6 +432,7 @@ running_anonymous_accessible: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} cr_for_failed: uuid: zzzzz-xvhdp-cr4failedcontnr @@ -442,6 +452,7 @@ cr_for_failed: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} canceled_with_queued_container: uuid: zzzzz-xvhdp-canceledqueuedc @@ -461,6 +472,7 @@ canceled_with_queued_container: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} canceled_with_locked_container: uuid: zzzzz-xvhdp-canceledlocekdc @@ -480,6 +492,7 @@ canceled_with_locked_container: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} canceled_with_running_container: uuid: zzzzz-xvhdp-canceledrunning @@ -499,6 +512,7 @@ canceled_with_running_container: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} running_to_be_deleted: uuid: zzzzz-xvhdp-cr5runningcntnr @@ -518,6 +532,7 @@ running_to_be_deleted: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} completed_with_input_mounts: uuid: zzzzz-xvhdp-crwithinputmnts @@ -539,18 +554,23 @@ completed_with_input_mounts: container_uuid: zzzzz-dz642-compltcontainer log_uuid: zzzzz-4zz18-logcollection01 output_uuid: zzzzz-4zz18-znfnqtbbv4spc3w - mounts: - /var/lib/cwl/cwl.input.json: - content: - input1: - basename: foo - class: File - location: "keep:fa7aeb5140e2848d39b416daeef4ffc5+45/foo" - input2: - basename: bar - class: File - location: "keep:fa7aeb5140e2848d39b416daeef4ffc5+45/bar" - /var/lib/cwl/workflow.json: "keep:f9ddda46bb293b6847da984e3aa735db+290" + mounts: { + "/var/lib/cwl/cwl.input.json": { + "kind": "json", + "content": { + "input1": { + "basename": "foo", + "class": "File", + "location": "keep:fa7aeb5140e2848d39b416daeef4ffc5+45/foo", + }, + "input2": { + "basename": "bar", + "class": "File", + "location": "keep:fa7aeb5140e2848d39b416daeef4ffc5+45/bar", + } + } + } + } uncommitted: uuid: zzzzz-xvhdp-cr4uncommittedc @@ -991,6 +1011,7 @@ cr_in_trashed_project: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} runtime_token: uuid: zzzzz-xvhdp-11eklkhy0n4dm86 @@ -1011,6 +1032,7 @@ runtime_token: runtime_constraints: vcpus: 1 ram: 123 + mounts: {} # Test Helper trims the rest of the file @@ -1026,6 +1048,7 @@ cr_<%=i%>_of_60: name: cr-<%= i.to_s %> output_path: test command: ["echo", "hello"] + mounts: {} <% end %> # Do not add your fixtures below this line as the rest of this file will be trimmed by test_helper diff --git a/services/api/test/functional/arvados/v1/container_requests_controller_test.rb b/services/api/test/functional/arvados/v1/container_requests_controller_test.rb index 95c477f411..e99af39c9c 100644 --- a/services/api/test/functional/arvados/v1/container_requests_controller_test.rb +++ b/services/api/test/functional/arvados/v1/container_requests_controller_test.rb @@ -24,7 +24,8 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase cr = JSON.parse(@response.body) assert_not_nil cr, 'Expected container request' - assert_equal sp, cr['scheduling_parameters'] + assert_equal sp['partitions'], cr['scheduling_parameters']['partitions'] + assert_equal false, cr['scheduling_parameters']['preemptible'] end test "secret_mounts not in #create responses" do @@ -62,6 +63,28 @@ class Arvados::V1::ContainerRequestsControllerTest < ActionController::TestCase assert_equal 'bar', req.secret_mounts['/foo']['content'] end + test "cancel with runtime_constraints and scheduling_params with default values" do + authorize_with :active + req = container_requests(:queued) + + patch :update, params: { + id: req.uuid, + container_request: { + state: 'Final', + priority: 0, + runtime_constraints: { + 'vcpus' => 1, + 'ram' => 123, + 'keep_cache_ram' => 0, + }, + scheduling_parameters: { + "preemptible"=>false + } + }, + } + assert_response :success + end + test "update without deleting secret_mounts" do authorize_with :active req = container_requests(:uncommitted) diff --git a/services/api/test/functional/user_sessions_controller_test.rb b/services/api/test/functional/user_sessions_controller_test.rb index d979208d38..129464cf1c 100644 --- a/services/api/test/functional/user_sessions_controller_test.rb +++ b/services/api/test/functional/user_sessions_controller_test.rb @@ -47,6 +47,31 @@ class UserSessionsControllerTest < ActionController::TestCase 1.second) end + [[0, 1.hour, 1.hour], + [1.hour, 2.hour, 1.hour], + [2.hour, 1.hour, 1.hour], + [2.hour, nil, 2.hour], + ].each do |config_lifetime, request_lifetime, expect_lifetime| + test "login with TokenLifetime=#{config_lifetime} and request has expires_at=#{ request_lifetime.nil? ? "nil" : request_lifetime }" do + Rails.configuration.Login.TokenLifetime = config_lifetime + expected_expiration_time = Time.now() + expect_lifetime + authorize_with :inactive + @request.headers['Authorization'] = 'Bearer '+Rails.configuration.SystemRootToken + if request_lifetime.nil? + get :create, params: {provider: 'controller', auth_info: {email: "foo@bar.com"}, return_to: ',https://app.example'} + else + get :create, params: {provider: 'controller', auth_info: {email: "foo@bar.com", expires_at: Time.now() + request_lifetime}, return_to: ',https://app.example'} + end + assert_response :redirect + api_client_auth = assigns(:api_client_auth) + assert_not_nil api_client_auth + assert_not_nil assigns(:api_client) + assert_in_delta(api_client_auth.expires_at, + expected_expiration_time, + 1.second) + end + end + test "login with remote param returns a salted token" do authorize_with :inactive api_client_page = 'http://client.example.com/home' diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index 90de800b2f..b2dde79956 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -153,7 +153,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr.reload - assert_equal({"vcpus" => 2, "ram" => 30}, cr.runtime_constraints) + assert ({"vcpus" => 2, "ram" => 30}.to_a - cr.runtime_constraints.to_a).empty? assert_not_nil cr.container_uuid c = Container.find_by_uuid cr.container_uuid @@ -164,7 +164,7 @@ class ContainerRequestTest < ActiveSupport::TestCase assert_equal({}, c.environment) assert_equal({"/out" => {"kind"=>"tmp", "capacity"=>1000000}}, c.mounts) assert_equal "/out", c.output_path - assert_equal({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}, c.runtime_constraints) + assert ({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}.to_a - c.runtime_constraints.to_a).empty? assert_operator 0, :<, c.priority assert_raises(ActiveRecord::RecordInvalid) do @@ -973,47 +973,16 @@ class ContainerRequestTest < ActiveSupport::TestCase end else cr.save! - assert_equal sp, cr.scheduling_parameters + assert (sp.to_a - cr.scheduling_parameters.to_a).empty? end end end - [ - 'zzzzz-dz642-runningcontainr', - nil, - ].each do |requesting_c| - test "having preemptible instances active on the API server, a committed #{requesting_c.nil? ? 'non-':''}child CR should not ask for preemptible instance if parameter already set to false" do - common_attrs = {cwd: "test", - priority: 1, - command: ["echo", "hello"], - output_path: "test", - scheduling_parameters: {"preemptible" => false}, - mounts: {"test" => {"kind" => "json"}}} - - Rails.configuration.Containers.UsePreemptibleInstances = true - set_user_from_auth :active - - if requesting_c - cr = with_container_auth(Container.find_by_uuid requesting_c) do - create_minimal_req!(common_attrs) - end - assert_not_nil cr.requesting_container_uuid - else - cr = create_minimal_req!(common_attrs) - end - - cr.state = ContainerRequest::Committed - cr.save! - - assert_equal false, cr.scheduling_parameters['preemptible'] - end - end - [ [true, 'zzzzz-dz642-runningcontainr', true], - [true, nil, nil], - [false, 'zzzzz-dz642-runningcontainr', nil], - [false, nil, nil], + [true, nil, false], + [false, 'zzzzz-dz642-runningcontainr', false], + [false, nil, false], ].each do |preemptible_conf, requesting_c, schedule_preemptible| test "having Rails.configuration.Containers.UsePreemptibleInstances=#{preemptible_conf}, #{requesting_c.nil? ? 'non-':''}child CR should #{schedule_preemptible ? '':'not'} ask for preemptible instance by default" do common_attrs = {cwd: "test", @@ -1068,11 +1037,11 @@ class ContainerRequestTest < ActiveSupport::TestCase end else cr = create_minimal_req!(common_attrs.merge({state: state})) - assert_equal sp, cr.scheduling_parameters + assert (sp.to_a - cr.scheduling_parameters.to_a).empty? if state == ContainerRequest::Committed c = Container.find_by_uuid(cr.container_uuid) - assert_equal sp, c.scheduling_parameters + assert (sp.to_a - c.scheduling_parameters.to_a).empty? end end end diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index 98e60e0579..4d85385242 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -23,6 +23,8 @@ class ContainerTest < ActiveSupport::TestCase command: ["echo", "hello"], output_path: "test", runtime_constraints: { + "api" => false, + "keep_cache_ram" => 0, "ram" => 12000000000, "vcpus" => 4, }, @@ -227,11 +229,12 @@ class ContainerTest < ActiveSupport::TestCase set_user_from_auth :active env = {"C" => "3", "B" => "2", "A" => "1"} m = {"F" => {"kind" => "3"}, "E" => {"kind" => "2"}, "D" => {"kind" => "1"}} - rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1} + rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1, "api" => true} c, _ = minimal_new(environment: env, mounts: m, runtime_constraints: rc) - assert_equal c.environment.to_json, Container.deep_sort_hash(env).to_json - assert_equal c.mounts.to_json, Container.deep_sort_hash(m).to_json - assert_equal c.runtime_constraints.to_json, Container.deep_sort_hash(rc).to_json + c.reload + assert_equal Container.deep_sort_hash(env).to_json, c.environment.to_json + assert_equal Container.deep_sort_hash(m).to_json, c.mounts.to_json + assert_equal Container.deep_sort_hash(rc).to_json, c.runtime_constraints.to_json end test 'deep_sort_hash on array of hashes' do @@ -561,6 +564,7 @@ class ContainerTest < ActiveSupport::TestCase assert_equal Container::Queued, c1.state reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token))) # See #14584 + assert_not_nil reused assert_equal c1.uuid, reused.uuid end @@ -571,6 +575,7 @@ class ContainerTest < ActiveSupport::TestCase assert_equal Container::Queued, c1.state reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token))) # See #14584 + assert_not_nil reused assert_equal c1.uuid, reused.uuid end @@ -581,6 +586,7 @@ class ContainerTest < ActiveSupport::TestCase assert_equal Container::Queued, c1.state reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token))) # See #14584 + assert_not_nil reused assert_equal c1.uuid, reused.uuid end -- 2.30.2