juju: cache API endpoints and credentials

juju/api.go

 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
 package juju
 
 import (
         "fmt"
         "io"
         "time"
 
@@ skipping 15 matching lines @@
 // changed by tests.
 var (
         apiOpen              = api.Open
         apiClose             = (*api.State).Close
         providerConnectDelay = 2 * time.Second
 )
 
 // apiState wraps an api.State, redefining its Close method
 // so we can abuse it for testing purposes.
 type apiState struct {
-        st            *api.State
-        connectedInfo *api.Info
-        // fromEnviron is true if the connection was made using the
-        // environment, rather than the local .jenv connection info.
-        fromEnviron bool

[rog, 2014/01/15 13:31:09]

We could lose this field and rename connectedInfo to cacheInfo;
then the code could cache the info only if cacheInfo is non-nil.

[dimitern, 2014/01/15 14:24:03]

Done.

+        st *api.State
+        // If cachedInfo is non-nil, it indicates that the info has been
+        // newly retrieved, and should be cached in the config store.
+        cachedInfo *api.Info
 }
 
 func (st apiState) Close() error {
         return apiClose(st.st)
 }
 
 // APIConn holds a connection to a juju environment and its
 // associated state through its API interface.
 type APIConn struct {
         Environ environs.Environ
@@ skipping 117 matching lines @@
                 // Delay the config connection until we've spent
                 // some time trying to connect to the cached info.
                 delay = providerConnectDelay
         } else {
                 logger.Debugf("no cached API connection settings found")
         }
         try.Start(func(stop <-chan struct{}) (io.Closer, error) {
                 return apiConfigConnect(info, envs, envName, stop, delay)
         })
         try.Close()
-        val, err := try.Result()
+        val0, err := try.Result()
         if err != nil {
                 if ierr, ok := err.(*infoConnectError); ok {
                         // lose error encapsulation:
                         err = ierr.error
                 }
                 return nil, err
         }

[rog, 2014/01/15 13:31:09]

Please do the dynamic type conversion once only.

I would usually do something like:

     val0, err := try.Result()
     if err != nil {
         ...
     }
     val := val0.(apiState)
     if info != nil && val.cacheInfo != nil {
         ...
     }
     return val.st

[dimitern, 2014/01/15 14:24:03]

Done.

-        connectedInfo := val.(apiState).connectedInfo
-        connectedState := val.(apiState).st
-        connectedFromEnviron := val.(apiState).fromEnviron
-
-        if connectedFromEnviron && info != nil {
-                // Cache the successful connection info for future use, but only if we

[rog, 2014/01/15 13:31:09]

How about putting the body of this into its own function?

say:

func cacheInfo(info configstore.EnvironInfo, apiInfo *api.Info) error

[dimitern, 2014/01/15 14:24:03]

Done.

-                // connected using the environment config.
-                info.SetAPIEndpoint(configstore.APIEndpoint{
-                        Addresses: connectedInfo.Addrs,
-                        CACert:    string(connectedInfo.CACert),
-                })
-                _, username, err := names.ParseTag(connectedInfo.Tag, names.UserTagKind)
+        val := val0.(apiState)
+
+        if val.cachedInfo != nil && info != nil {
+                // Cache the connection settings only if we used the
+                // environment config, but any errors are just logged
+                // as warnings, because they're not fatal.
+                err = cacheAPIInfo(info, val.cachedInfo)
                 if err != nil {
-                        logger.Warningf("not caching API connection settings: invalid API user tag: %v", err)
-                        return connectedState, nil
-                }
-                info.SetAPICredentials(configstore.APICredentials{
-                        User:     username,
-                        Password: connectedInfo.Password,
-                })
-                if err := info.Write(); err != nil {
-                        // Not fatal, just the cache won't be updated.
-                        logger.Warningf("cannot cache API connection settings: %v", err)
-                }
-                logger.Debugf("updated API connection settings cache")
-        }
-        return connectedState, nil
+                        logger.Warningf(err.Error())
+                } else {
+                        logger.Debugf("updated API connection settings cache")
+                }
+        }
+        return val.st, nil
 }
 
 func errorImportance(err error) int {
         if err == nil {
                 return 0
         }
         if errors.IsNotFoundError(err) {
                 // An error from an actual connection attempt
                 // is more interesting than the fact that there's
                 // no environment info available.
@@ skipping 22 matching lines @@
         apiInfo := &api.Info{
                 Addrs:    endpoint.Addresses,
                 CACert:   []byte(endpoint.CACert),
                 Tag:      names.UserTag(info.APICredentials().User),
                 Password: info.APICredentials().Password,
         }
         st, err := apiOpen(apiInfo, api.DefaultDialOpts())
         if err != nil {
                 return apiState{}, &infoConnectError{err}
         }
-        return apiState{st, apiInfo, false}, err
+        return apiState{st, nil}, err
 }
 
 // apiConfigConnect looks for configuration info on the given environment,
 // and tries to use an Environ constructed from that to connect to
 // its endpoint. It only starts the attempt after the given delay,
 // to allow the faster apiInfoConnect to hopefully succeed first.
 // It returns nil if there was no configuration information found.
 func apiConfigConnect(info configstore.EnvironInfo, envs *environs.Environs, envName string, stop <-chan struct{}, delay time.Duration) (apiState, error) {
         var cfg *config.Config
         var err error
@@ skipping 18 matching lines @@
         }
         apiInfo, err := environAPIInfo(environ)
         if err != nil {
                 return apiState{}, err
         }
         st, err := apiOpen(apiInfo, api.DefaultDialOpts())
         // TODO(rog): handle errUnauthorized when the API handles passwords.
         if err != nil {
                 return apiState{}, err
         }
-        return apiState{st, apiInfo, true}, nil
+        return apiState{st, apiInfo}, nil
 }
 
 func environAPIInfo(environ environs.Environ) (*api.Info, error) {
         _, info, err := environ.StateInfo()
         if err != nil {
                 return nil, err
         }
         info.Tag = "user-admin"
         password := environ.Config().AdminSecret()
         if password == "" {
                 return nil, fmt.Errorf("cannot connect without admin-secret")
         }
         info.Password = password
         return info, nil
 }
+
+// cacheAPIInfo updates the local environment settings (.jenv file)
+// with the provided apiInfo, assuming we've just successfully
+// connected to the API server.
+func cacheAPIInfo(info configstore.EnvironInfo, apiInfo *api.Info) error {
+        info.SetAPIEndpoint(configstore.APIEndpoint{
+                Addresses: apiInfo.Addrs,
+                CACert:    string(apiInfo.CACert),
+        })
+        _, username, err := names.ParseTag(apiInfo.Tag, names.UserTagKind)
+        if err != nil {
+                return fmt.Errorf("not caching API connection settings: invalid API user tag: %v", err)
+        }
+        info.SetAPICredentials(configstore.APICredentials{
+                User:     username,
+                Password: apiInfo.Password,
+        })
+        if err := info.Write(); err != nil {
+                return fmt.Errorf("cannot cache API connection settings: %v", err)
+        }
+        return nil
+}