impl/prod: Embed AppEngine SDK into Context.

impl/prod/context.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package prod
 
 import (
         "fmt"
         "net/http"
         "net/http/cookiejar"
@@ skipping 14 matching lines @@
         "https://www.googleapis.com/auth/cloud.platform",
 }
 
 type key int
 
 var (
         prodStateKey  = "contains the current *prodState"
         probeCacheKey = "contains the current *infoProbeCache"
 )
 
-// AEContext retrieves the raw "google.golang.org/appengine" compatible Context.
+// getAEContext retrieves the raw "google.golang.org/appengine" compatible
+// Context.
 //
-// It also transfers deadline of `c` to AE context, since deadline is used for
-// RPCs. Doesn't transfer cancelation ability though (since it's ignored by GAE
-// anyway).
-func AEContext(c context.Context) context.Context {
+// This is an independent Context chain from `c`. In an attempt to maintain user
+// expectations, the deadline of `c` is transferred to the returned Context,
+// RPCs. Cancelation is not transferred.
+func getAEContext(c context.Context) context.Context {
         ps := getProdState(c)
         return ps.context(c)
 }
 
-func setupAECtx(c, aeCtx context.Context) context.Context {
+func setupAECtx(c context.Context) context.Context {
+        // Retain the base AppEngine Context. This will allow us to switch between
+        // transactional and non-transactional Contexts as needed.
         c = withProdState(c, prodState{
-                ctx:      aeCtx,
-                noTxnCtx: aeCtx,
+                ctx:      c,
+                noTxnCtx: c,
         })
         return useModule(useMail(useUser(useURLFetch(useRDS(useMC(useTQ(useGI(useLogging(c)))))))))
 }
 
 // Use adds production implementations for all the gae services to the
-// context.
+// context. The implementations are all backed by the real AppEngine SDK
+// functionality,
 //
 // The services added are:
 //   - github.com/luci-go/common/logging
 //   - github.com/luci/gae/service/datastore
 //   - github.com/luci/gae/service/info
 //   - github.com/luci/gae/service/mail
 //   - github.com/luci/gae/service/memcache
 //   - github.com/luci/gae/service/module
 //   - github.com/luci/gae/service/taskqueue
 //   - github.com/luci/gae/service/urlfetch
 //   - github.com/luci/gae/service/user
 //
-// These can be retrieved with the <service>.Get functions.
-//
-// The implementations are all backed by the real appengine SDK functionality,
+// In addition, Use installs Google AppEngine SDK Context values into the
+// returned Context, allowing it to be used with raw Google AppEngine SDK APIs.
+// THIS IS VERY DANGEROUS, as the AppEngine SDK is NOT aware of luci/gae
+// services and DOES NOT coordinate state with them. Dependency on AppEngine SDK
+// calls undermines a lot of value that luci/gae adds, and should be used
+// CAUTIOUSLY and EXPERTLY. Some examples of pitfalls:

[iannucci, 2016/10/28 18:40:05]

as I mentioned on the other CL, I would stress that it's only harmful when
mixing and matching services that luci/gae supports. Accessing unsupported
services is fine.

The other alternative is that we could just add wrappers for the rest of the
services.

+//      - AppEngine SDK is not aware of the luci/gae transaction state of the
+//        Context. AppEngine calls to a Context that is in a luci/gae transaction
+//        will not operate on that transaction, and luci/gae calls to a Context that
+//        is in an AppEngine SDK transaction state will not operate on that
+//        transaction.
+//      - luci/gae filters are completely bypassed when accessing AppEngine SDK
+//        directly. This means that operations such as `dscache` will not be
+//        consulted or updated, leading to potential inconsistent state and memory
+//        corruption issues.

[iannucci, 2016/10/28 18:40:05]

data corruption. not really memory corruption.

 func Use(c context.Context, r *http.Request) context.Context {
-        return setupAECtx(c, appengine.NewContext(r))
+        return setupAECtx(appengine.WithContext(c, r))

[iannucci, 2016/10/28 18:40:05]

why is setupAECtx still needed at all?

[dnj, 2016/10/29 00:10:59]

UseRemote still uses it.

 }
 
 // UseRemote is the same as Use, except that it lets you attach a context to
 // a remote host using the Remote API feature. See the docs for the
 // prerequisites.
 //
 // docs: https://cloud.google.com/appengine/docs/go/tools/remoteapi
 //
-// inOutCtx will be replaced with the new, derived context, if err is nil,
-// otherwise it's unchanged and continues to be safe-to-use.
-//
-// If client is nil, this will use create a new client, and will try to be
-// clever about it:
-//   * If you're creating a remote context FROM AppEngine, this will use
-//     urlfetch.Transport. This can be used to allow app-to-app remote_api
-//     control.
-//
-//   * If host starts with "localhost", this will create a regular http.Client
-//     with a cookiejar, and call the _ah/login API to log in as an admin with
-//     the user "admin@example.com".
-//
-//   * Otherwise, it will create a Google OAuth2 client with the following scopes:
-//       - "https://www.googleapis.com/auth/appengine.apis"
-//       - "https://www.googleapis.com/auth/userinfo.email"
-//       - "https://www.googleapis.com/auth/cloud.platform"
-func UseRemote(inOutCtx *context.Context, host string, client *http.Client) (err error) {
+// If client is nil, http.DefaultClient will be used.
+func UseRemote(host string, client *http.Client) (context.Context, error) {
         if client == nil {
-                aeCtx := AEContext(*inOutCtx)
-
-                if strings.HasPrefix(host, "localhost") {
-                        transp := http.DefaultTransport
-                        if aeCtx != nil {
-                                transp = urlfetch.Get(*inOutCtx)
-                        }
-
-                        client = &http.Client{Transport: transp}
-                        client.Jar, err = cookiejar.New(nil)
-                        if err != nil {
-                                return
-                        }
-                        u := fmt.Sprintf("http://%s/_ah/login?%s", host, url.Values{
-                                "email":  {"admin@example.com"},
-                                "admin":  {"True"},
-                                "action": {"Login"},
-                        }.Encode())
-
-                        var rsp *http.Response
-                        rsp, err = client.Get(u)
-                        if err != nil {
-                                return
-                        }
-                        defer rsp.Body.Close()
-                } else {
-                        if aeCtx == nil {
-                                aeCtx = context.Background()
-                        }
-                        client, err = gOAuth.DefaultClient(aeCtx, RemoteAPIScopes...)
-                        if err != nil {
-                                return
-                        }
-                }
+                client = http.DefaultClient
         }
 
-        aeCtx, err := remote_api.NewRemoteContext(host, client)
+        c, err := remote_api.NewRemoteContext(host, client)
         if err != nil {
-                return
+                return nil, err
         }
-        *inOutCtx = setupAECtx(*inOutCtx, aeCtx)
-        return nil
+        return setupAECtx(c), nil
+}
+
+// RemoteAPIClientForHost generates an authenticated HTTP client suitable for
+// use with UseRemote for the specified host.
+//
+//      - If you're creating a remote context FROM AppEngine, this will use
+//        urlfetch.Transport. This can be used to allow app-to-app remote_api
+//        control. This is detected when a luci/gae urlfetch service is present in
+//        the supplied Context.
+//
+//      - If host starts with "localhost", this will create a regular http.Client
+//        with a cookiejar, and call the _ah/login API to log in as an admin with
+//        the user "admin@example.com".
+//
+//      - Otherwise, it will create a Google OAuth2 client with the following
+//        scopes:
+//        - "https://www.googleapis.com/auth/appengine.apis"
+//        - "https://www.googleapis.com/auth/userinfo.email"
+//        - "https://www.googleapis.com/auth/cloud.platform"
+func RemoteAPIClientForHost(c context.Context, host string) (*http.Client, error) {
+        aeCtx := getAEContext(c)
+
+        if strings.HasPrefix(host, "localhost") {
+                transp := http.DefaultTransport
+                if aeCtx != nil {
+                        transp = urlfetch.Get(aeCtx)
+                }
+
+                client := &http.Client{Transport: transp}
+
+                var err error
+                if client.Jar, err = cookiejar.New(nil); err != nil {
+                        return nil, err
+                }
+                u := fmt.Sprintf("http://%s/_ah/login?%s", host, url.Values{
+                        "email":  {"admin@example.com"},
+                        "admin":  {"True"},
+                        "action": {"Login"},
+                }.Encode())
+
+                var rsp *http.Response
+                if rsp, err = client.Get(u); err != nil {
+                        return nil, err
+                }
+                defer rsp.Body.Close()
+                return client, nil
+        }
+
+        if aeCtx == nil {
+                aeCtx = context.Background()
+        }
+        return gOAuth.DefaultClient(aeCtx, RemoteAPIScopes...)
 }
 
 // prodState is the current production state.
 type prodState struct {
         // ctx is the current derived GAE context.
         ctx context.Context
 
         // noTxnCtx is a Context maintained alongside ctx. When a transaction is
         // entered, ctx will be updated, but noTxnCtx will not, allowing extra-
         // transactional Context access.
@@ skipping 26 matching lines @@
         aeCtx := ps.ctx
         if aeCtx == nil {
                 return nil
         }
 
         if deadline, ok := c.Deadline(); ok {
                 aeCtx, _ = context.WithDeadline(aeCtx, deadline)
         }
         return aeCtx
 }