Create temporary cert file for each ManagementAPI.

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://code.launchpad.net/~jtv/juju-core/session-certificate/+merge/170611

Requires: https://code.launchpad.net/~jtv/juju-core/create-gwacl-sessions/+merge/170032

(do not edit description out of merge proposal)

[jtv.canonical, 2013-06-20 16:07:18]

Please take a look.

[rog, 2013-06-20 16:40:57]

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#newc...
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#newc...
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#newc...
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...
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.

[jtv.canonical, 2013-06-20 16:58:34]

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!

[rog, 2013-06-20 17:24:45]

On 20 June 2013 17:58,  <jtv.canonical@gmail.com> 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 :-)