Merge terraform-softlayer into terraform-ibmcloud (steps)

[renier]

This is a list of things that need to be done, in rough order, in order to merge the terraform-softlayer provider development effectively into the terraform-ibmcloud space:

• Re-enable SoftLayer username/apikey for infra resources
• Enable lazy validation of provider credentials for softlayer-only users
• Commit access for @renier, @minsikl, @rbmateescu, @tflowcamdev on new provider repository
• Merge diverging changes from resources in ibm provider over to softlayer provider: storage resources, addition of notes.
• Fix automatic reading of ~/.softlayer for credentials
• Copy over resources from terraform-provider-softlayer after making the resource name changes.
• Port documentation from terraform-softlayer to middleman format
• Deprecate terraform-provider-softlayer repository (move over issues)

@albee-jhoney

[vkalangu]

Access granted to @renier, @minsikl, @rbmateescu, @tflowcamdev

[ashishth09]

@renier,

Regarding Enable lazy validation of provider credentials for softlayer-only users

I have 2 things on mind to tell users about missing credentials. Let me know what you think.

• Show error in the first given chance: In a resource CRUD operation we are retrieving the client, via invoking some function which sits outside resource CRUD logic implementation for example in config.go. We can do os.Exit() in that function which is shared across the resource implementation. This way we don't touch the resource implementations and it make code changes to just one file plus error is shown immediately. But using os.Exit() is frowned upon in general programming, I guess.

• Return error if during some resource CRUD operation you realize not enough authentication credentials have been provided by the user. This involves changing the whole code base, would touch every resource file. So if user gives bluemix_api_key and not softlayer_api_key and tries to provision vm as well as some cloud foundry application then in this case, cloud foundry application creation will proceed, will go to completion and then terraform will spit out the error which we return in the SL resource CRUD implementation.

To me looks like 2nd option is more robust just that it touches all the resource, all the CRUD functions. I think I could minimize them by just putting them in the Read/Create.. as those are the 2 things which are done on a fresh resource.

[albee-jhoney]

@ashishth09
'Lazy validation of provider credentials':

• provider: all credentials are optional
• resource: verify the existence of the required credentials as a pre-req

In my understanding, the option 2 translates to

• Provider implementation will create & return valid (softlayer, bluemix) clientsession, only if the respective credentials are provided. (else, return null for Session & an error)
  ** func (sess clientSession) SoftLayerSession() (*slsession.Session, error)
  ** func (sess clientSession) BluemixSession() (*bxsession.Session, error)
• Resource implementation can do one of the following
  ** check & handle error from the SoftLayerSession() or BluemixSession() (require to touch every resource file)
  ** Ignore the error, and pass the null Session object to the go-client, for it to handle & report the error (not a good option).

[ashishth09]

@albee-jhoney ,

Absolutely, even I think the same. It would require touching all files but that is the best solution.

** Ignore the error, and pass the null Session object to the go-client, for it to handle & report the error (not a good option).

This would end up crashing terraform so not a good option as you mentioned.

[renier]

@ashishth09 @albee-jhoney

Return error if during some resource CRUD operation you realize not enough authentication credentials have been provided by the user. This involves changing the whole code base, would touch every resource file. So if user gives bluemix_api_key and not softlayer_api_key and tries to provision vm as well as some cloud foundry application then in this case, cloud foundry application creation will proceed, will go to completion and then terraform will spit out the error which we return in the SL resource CRUD implementation.
To me looks like 2nd option is more robust just that it touches all the resource, all the CRUD functions. I think I could minimize them by just putting them in the Read/Create.. as those are the 2 things which are done on a fresh resource.

Returning an error is the only option. A plugin should never call os.Exit(). Let's talk about what our lazy-evaluation means, then.

First, it can and should be done in a way that does not involve "changing the whole code base" (CTWCB).

The first and easiest option is to simply let the API return the error due to insufficient credentials. Why is this not a good option? The API tells the user what the problem is instead of the provider telling the user what the problem is. In both cases, the user would see an error. But if you try to do this in the provider code, it would cause CTWCB, which we are trying to avoid.

The second and less easy option which stays away from CTWCB, is to update the bluemix-go and softlayer-go code to check for non-empty credentials. Any call going through the lower-level client library would result in an error if credentials are empty or insufficient. You would not need any provider changes in this case. You would still return an error.

• Ignore the error, and pass the null Session object to the go-client, for it to handle & report the error (not a good option).

This would end up crashing terraform so not a good option as you mentioned.

This should be filed as a bug in the go-client in any case. The go-client should never panic if being passed a null. It should just return an error.

[ashishth09]

@renier,

The first and easiest option is to simply let the API return the error due to insufficient credentials. Why is this not a good option?

It is 👍 We need to check if the error returned by the SDK is good enough. For example some may return Access Denied or Authentication failure etc. The error returned by the clients won't specifically tell which provider parameters are missing though. They can be inferred and they would be close. If not then we should make them.

The second and less easy option which stays away from CTWCB, is to update the bluemix-go and softlayer-go code to check for non-empty credentials.

bluemix-go unlike softlayer-go is checking that and hence we know about the credentials issues in the provider configuration itself when we create our client. Now the CTWCB arised for us because unlike softlayer we don't pass around session but the API interface references which we create during the provider configuration and use ready-made in the resource CRUD. Since we configured them in the provider , we know if anything goes wrong the call would never come to the resource CRUD (since we return with error right away). Hence we never expect them to have nil value. It will blow if we don't check for their nil value in the whole code base in every resource CRUD now or do some others changes to retrieve their value.

This would end up crashing terraform so not a good option as you mentioned.

This was meant in reference to our API interfaces above. Assuming someone doesn't configure the bluemix_api_key and we assume not to not configure bluemix apis, then nil is the value we would get in the CRUD at this point.

[renier]

@ashishth09 My suggestion is to start small (let the API return the error) and document it. Users (in the aggregate) will give you an idea of what more needs to be done there. But at that point, you've already made most of the progress.

[ashishth09]

@renier, I guess so. At this point we need to figure out a way for the bluemix api interfaces first.
For SoftLayer we don't have the API client problem and it throws relevant error. If not the most accurate pointing exactly what is missing. But we should be fine initially.

[mihasya]

Just thought I'd chime in and let y'all know that while switching from https://github.com/softlayer/terraform-provider-softlayer to this build of the plugin, I was bit by the fact that the new plugin no longer reads ~/.softlayer and spent a bit of time chasing a wild goose. I thought I didn't have permissions to create VLANs, but eventually recognized that I was getting 401s, not 403s, and figured it out from there. Would be nice if that file was still respected or there was an alternative so we don't have to push environment variables into automated builds.

Thanks for all the good work on this!

[ashishth09]

@mihasya , Thanks for the feedback 👍 We will look into it.