impl/prod: Embed AppEngine SDK into Context.

impl/prod/context.go

Index: impl/prod/context.go
diff --git a/impl/prod/context.go b/impl/prod/context.go
index 10ba7d1d36433213957863ea0fa3b309f8020563..4e49cf94d371ceec9120f0651126d32935770f30 100644
--- a/impl/prod/context.go
+++ b/impl/prod/context.go
@@ -32,26 +32,30 @@ var (
 	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
@@ -64,11 +68,23 @@ func setupAECtx(c, aeCtx context.Context) context.Context {
 //   - 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
@@ -77,67 +93,69 @@ func Use(c context.Context, r *http.Request) context.Context {
 //
 // 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, http.DefaultClient will be used.
+func UseRemote(host string, client *http.Client) (context.Context, error) {
+	if client == nil {
+		client = http.DefaultClient
+	}
+
+	c, err := remote_api.NewRemoteContext(host, client)
+	if err != nil {
+		return nil, err
+	}
+	return setupAECtx(c), nil
+}
+
+// RemoteAPIClientForHost generates an authenticated HTTP client suitable for
+// use with UseRemote for the specified host.
 //
-// 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 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".
+//	- 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 == 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
-			}
+//	- 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
 	}
 
-	aeCtx, err := remote_api.NewRemoteContext(host, client)
-	if err != nil {
-		return
+	if aeCtx == nil {
+		aeCtx = context.Background()
 	}
-	*inOutCtx = setupAECtx(*inOutCtx, aeCtx)
-	return nil
+	return gOAuth.DefaultClient(aeCtx, RemoteAPIScopes...)
 }
 
 // prodState is the current production state.