LogDog: Project READ access for user endpoints.

appengine/logdog/coordinator/project.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package coordinator
 
 import (
         "strings"
 
-        "github.com/luci/gae/service/datastore/meta"
         "github.com/luci/gae/service/info"
-        "github.com/luci/luci-go/common/config"
+        "github.com/luci/luci-go/appengine/logdog/coordinator/config"
+        luciConfig "github.com/luci/luci-go/common/config"
+        log "github.com/luci/luci-go/common/logging"
         "github.com/luci/luci-go/common/proto/logdog/svcconfig"
         "golang.org/x/net/context"
 )
 
-// projectNamespacePrefix is the datastore namespace prefix for project
-// namespaces.
-const projectNamespacePrefix = "luci."
+const (
+        // projectNamespacePrefix is the datastore namespace prefix for project
+        // namespaces.
+        projectNamespacePrefix = "luci."
+
+        // projectConfigWorkers is the number of workers that will pull project
+        // configs from the config service.
+        projectConfigWorkers = 16
+)
 
 // ProjectNamespace returns the AppEngine namespace for a given luci-config
 // project name.
-func ProjectNamespace(project config.ProjectName) string {
+func ProjectNamespace(project luciConfig.ProjectName) string {
         return projectNamespacePrefix + string(project)
 }
 
 // ProjectFromNamespace returns the current project installed in the supplied
 // Context's namespace.
 //
 // If the namespace does not have a project namespace prefix, this function
 // will return an empty string.
-func ProjectFromNamespace(ns string) config.ProjectName {
+func ProjectFromNamespace(ns string) luciConfig.ProjectName {
         if !strings.HasPrefix(ns, projectNamespacePrefix) {
                 return ""
         }
-        return config.ProjectName(ns[len(projectNamespacePrefix):])
+        return luciConfig.ProjectName(ns[len(projectNamespacePrefix):])
 }
 
 // CurrentProject returns the current project based on the currently-loaded
 // namespace.
 //
 // If there is no current namespace, or if the current namespace is not a valid
 // project namespace, an empty string will be returned.
-func CurrentProject(c context.Context) config.ProjectName {
+func CurrentProject(c context.Context) luciConfig.ProjectName {
         if ns, ok := info.Get(c).GetNamespace(); ok {
                 return ProjectFromNamespace(ns)
         }
         return ""
 }
 
 // CurrentProjectConfig returns the project-specific configuration for the
 // current project.
 //
 // If there is no current project namespace, or if the current project has no
 // configuration, config.ErrInvalidConfig will be returned.
 func CurrentProjectConfig(c context.Context) (*svcconfig.ProjectConfig, error) {
         return GetServices(c).ProjectConfig(c, CurrentProject(c))
 }
 
-// AllProjectsWithNamespaces scans current namespaces and returns those that
-// belong to LUCI projects.
-func AllProjectsWithNamespaces(c context.Context) ([]config.ProjectName, error) {
-        var projects []config.ProjectName
-        err := meta.NamespacesWithPrefix(c, projectNamespacePrefix, func(ns string) error {
-                if proj := ProjectFromNamespace(ns); proj != "" {
-                        projects = append(projects, proj)
-                }
-                return nil
-        })
+// ActiveUserProjects returns a full list of all config service projects with
+// a LogDog project configurations that the current user has READ access to.

[nodir, 2016/05/19 17:17:21]

s/a //

[dnj (Google), 2016/05/19 20:10:46]

Done.

+func ActiveUserProjects(c context.Context) (map[luciConfig.ProjectName]*svcconfig.ProjectConfig, error) {
+        allPcfgs, err := config.AllProjectConfigs(c)

[nodir, 2016/05/19 17:17:21]

I'd strongly recommend calling this function in a cron job and persisting
results to datastore; and then read from datastore here.

Then you can update your other functions for config loading to load from
datastore.
I am sorry I writing this only now. We should probably write a package that does
this sort of replication because all apps need it. If you want to do that, I can
review the code

[dnj (Google), 2016/05/19 20:10:46]

Agreed, and I'll put a TODO in, but this is not a performance bottleneck ATM.
This is an infrequently-called function at best, and memcached either way.

         if err != nil {
                 return nil, err
         }
-        return projects, nil
+
+        for project, pcfg := range allPcfgs {

[nodir, 2016/05/19 17:17:21]

add a TODO to make acl check concurrent. Auth groups can be large, so checks may
take time and we may have a lot of projects

[dnj (Google), 2016/05/19 20:10:46]

If this is actually a problem, I think we need to optimize this lookup. It is
the sort of thing that needs to happen fast.

+                // Verify user access. This includes loading the project's config.

[nodir, 2016/05/19 17:17:21]

it does not include that

[dnj (Google), 2016/05/19 20:10:46]

Done.

+                if err := IsProjectReader(c, pcfg); err != nil {
+                        delete(allPcfgs, project)

[nodir, 2016/05/19 17:17:21]

this makes me nervous. I'd rather create a separate map with restricted set and
add projects there explicitly. Anyway you will probably do the explicit addition
if you make this code parallel because you will send accessible projects to a
channel

[dnj (Google), 2016/05/19 20:10:46]

Agree about parallel construction.

I thought about this in response to your comment, but I think this is fine.
Delete is a reliable Go operation, and this logic is really simple: if there is
an error, remove the thing from the map. I am comfortable relying on this. Any
reconstruction around building another map will use the same checks and have
equivalent code paths.

+
+                        // If it is a membership error, prune this project and continue.
+                        // Otherwise, forward the error.
+                        if !IsMembershipError(err) {
+                                // No configuration for this project, the configuration is invalid, or
+                                // the user didn't have access. Remove it from the list.
+                                log.Fields{
+                                        log.ErrorKey: err,
+                                        "project":    project,
+                                }.Errorf(c, "Failed to check project.")
+                                return nil, err
+                        }
+                }
+        }
+        return allPcfgs, nil
 }