Merge lp:~jtv/juju-core/session-certificate into lp:~go-bot/juju-core/trunk

[Jeroen T. Vermeulen]

Create temporary cert file for each ManagementAPI.

For now, each high-level function in the Azure provider will create its own storage contexts and management-API objects as needed. In the case of the management API, that comes with a temporary file containing an Azure certificate. (There is another way to pass the certificate to go-curl, but we want to avoid the complications and especially the unknowns at this point).

Once done with your request(s) to the management, you release it. This cleans up the certificate file, but in the future it may also serve as a hook for connection pooling. We'll treat optimization as a separate problem — it seems easy but this sort of thing often brings out creative tendencies that aren't worth the time just at the moment. As long as we don't need any actual cleanup code for the storage-API side, we chose not to create a cleanup method there. This is all internal detail after all; it doesn't affect any exported APIs.

https://codereview.appspot.com/10447043

[Martin Packman (gz)]

Seems rietveld is still confused about the branch target here, looks
like it's comparing against an older trunk?

 From the diff in launchpad though, LGTM. Double check the merge makes
sense before landing. :)

https://codereview.appspot.com/10443043/

[Jeroen T. Vermeulen (jtv)]

mgz voted LGTM on the Rietveld merge proposal, but we created that after the LP one was already up and it seems a little confused.

[Jeroen T. Vermeulen (jtv)]

Continuing with a new Rietveld MP to replace the old one.

[Roger Peppe (rogpeppe)]

LGTM with some minor suggestions below.

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ.go#newcode153
environs/azure/environ.go:153: certFile, err :=
newTempCertFile([]byte(certData))
i hope we manage to avoid the need for these temporary files sooner
rather than later.

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ.go#newcode158
environs/azure/environ.go:158: // make sure we clean up that certificate
file.
rather than recovering, i think a better approach might be something
like this:
func (env *azureEnviron) getManagementAPI() (_ *azureManagementContext,
retErr) {
      ...
       certFile, err := newTempCertFile([]byte(certData))
       if err != nil {
           return nil, err
       }
       defer func() {
           if retErr != nil {
               certFile.Delete()
           }
       }()

which amounts to much the same thing except you don't
need to call certFile.Delete if NewManagementAPI fails.

To be honest though, we're not gaining that much
by deleting the file even when we panic. That
will only happen if NewManagementAPI itself panics,
not if any other goroutine panics.

I'd probably just delete the defer and leave the
Delete in the NewManagementAPI error case.
Anyone with access to read the cert file
probably has access to get the environment
config anyway, so it's not a huge security issue.

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ.go#newcode178
environs/azure/environ.go:178: // releaseManagementAPI frees up a
context object for interfacing with Azure's
// releaseManagementAPI frees up an Azure management context.
?

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10447043/diff/1/environs/azure/environ_test.go#newcode104
environs/azure/environ_test.go:104: }
i wouldn't mind seeing a test that checks that the temporary cert file
is deleted when NewManagementAPI fails.

https://codereview.appspot.com/10447043/

[Jeroen T. Vermeulen (jtv)]

LGTM votes from mgz and rogpeppe. Thanks!

review: Approve

[Roger Peppe (rogpeppe)]

On 20 June 2013 17:58, <email address hidden> wrote:
> Thanks Roger!
>
>
> On 2013/06/20 16:40:57, rog wrote:
>
>> i hope we manage to avoid the need for these temporary files sooner
>
> rather than
>>
>> later.
>
>
> Absolutely. There are several ways it might happen: by replacing
> libcurl with Go's built-in http library if it starts supporting this
> site, or by going through libcurl's in-memory mechanism for passing the
> certificates, or by storing the certificate somewhere slightly more
> durable perhaps. That's in descending order of desirability.
>
>
>
>> I'd probably just delete the defer and leave the
>> Delete in the NewManagementAPI error case.
>
>
> Done.
>
>
>
>> environs/azure/environ.go:178: // releaseManagementAPI frees up a
>
> context object
>>
>> for interfacing with Azure's
>> // releaseManagementAPI frees up an Azure management context.
>> ?
>
>
> I just made it say that it frees up the context object returned by
> getManagementAPI. Once. :)
>
>
>
>> i wouldn't mind seeing a test that checks that the temporary cert file
>
> is
>>
>> deleted when NewManagementAPI fails.
>
>
> There doesn't currently seem to be a failure path!

maybe just delete the error return from NewManagementAPI then
and code goes away :-)

=== modified file 'environs/azure/environ.go'
--- environs/azure/environ.go	2013-06-20 13:43:05 +0000
+++ environs/azure/environ.go	2013-06-20 17:02:46 +0000
@@ -129,21 +129,58 @@
 	panic("unimplemented")
 }
 
-// TODO: Temporarily deactivating this code.  Passing certificate in-memory
-// may require gwacl change.
-/*
+// azureManagementContext wraps two things: a gwacl.ManagementAPI (effectively
+// a session on the Azure management API) and a tempCertFile, which keeps track
+// of the temporary certificate file that needs to be deleted once we're done
+// with this particular session.
+// Since it embeds *gwacl.ManagementAPI, you can use it much as if it were a
+// pointer to a ManagementAPI object.  Just don't forget to release it after
+// use.
+type azureManagementContext struct {
+	*gwacl.ManagementAPI
+	certFile *tempCertFile
+}
+
 // getManagementAPI obtains a context object for interfacing with Azure's
 // management API.
 // For now, each invocation just returns a separate object.  This is probably
 // wasteful (each context gets its own SSL connection) and may need optimizing
 // later.
-func (env *azureEnviron) getManagementAPI() (*gwacl.ManagementAPI, error) {
+func (env *azureEnviron) getManagementAPI() (*azureManagementContext, error) {
 	snap := env.getSnapshot()
 	subscription := snap.ecfg.ManagementSubscriptionId()
-	cert := snap.ecfg.ManagementCertificate()
-	return gwacl.NewManagementAPI(subscription, cert)
-}
-*/
+	certData := snap.ecfg.ManagementCertificate()
+	certFile, err := newTempCertFile([]byte(certData))
+	if err != nil {
+		return nil, err
+	}
+	// After this point, if we need to leave prematurely, we should clean
+	// up that certificate file.
+	mgtAPI, err := gwacl.NewManagementAPI(subscription, certFile.Path())
+	if err != nil {
+		certFile.Delete()
+		return nil, err
+	}
+	context := azureManagementContext{
+		ManagementAPI: mgtAPI,
+		certFile:      certFile,
+	}
+	return &context, nil
+}
+
+// releaseManagementAPI frees up a context object obtained through
+// getManagementAPI.
+func (env *azureEnviron) releaseManagementAPI(context *azureManagementContext) {
+	// Be tolerant to incomplete context objects, in case we ever get
+	// called during cleanup of a failed attempt to create one.
+	if context == nil || context.certFile == nil {
+		return
+	}
+	// For now, all that needs doing is to delete the temporary certificate
+	// file.  We may do cleverer things later, such as connection pooling
+	// where this method returns a context to the pool.
+	context.certFile.Delete()
+}
 
 // getStorageContext obtains a context object for interfacing with Azure's
 // storage API.

=== modified file 'environs/azure/environ_test.go'
--- environs/azure/environ_test.go	2013-06-20 08:30:01 +0000
+++ environs/azure/environ_test.go	2013-06-20 17:02:46 +0000
@@ -68,16 +68,31 @@
 	testing.TestLockingFunction(&env.Mutex, func() { env.Config() })
 }
 
-// TODO: Temporarily deactivating this code.  Passing certificate in-memory
-// may require gwacl change.
-/*
 func (EnvironSuite) TestGetManagementAPI(c *C) {
 	env := makeEnviron(c)
-	management, err := env.getManagementAPI()
+	context, err := env.getManagementAPI()
 	c.Assert(err, IsNil)
-	c.Check(management, NotNil)
-}
-*/
+	defer env.releaseManagementAPI(context)
+	c.Check(context, NotNil)
+	c.Check(context.ManagementAPI, NotNil)
+	c.Check(context.certFile, NotNil)
+}
+
+func (EnvironSuite) TestReleaseManagementAPIAcceptsNil(c *C) {
+	env := makeEnviron(c)
+	env.releaseManagementAPI(nil)
+	// The real test is that this does not panic.
+}
+
+func (EnvironSuite) TestReleaseManagementAPIAcceptsIncompleteContext(c *C) {
+	env := makeEnviron(c)
+	context := azureManagementContext{
+		ManagementAPI: nil,
+		certFile:      nil,
+	}
+	env.releaseManagementAPI(&context)
+	// The real test is that this does not panic.
+}
 
 func (EnvironSuite) TestGetStorageContext(c *C) {
 	env := makeEnviron(c)