Load Cloudstack configuration from file #13926

mcanevet · 2017-04-25T07:51:56Z

This PR adds 2 new parameters to the CloudStack provider allowing to fetch credentials from a configuration file compatible with https://github.com/exoscale/cs CloudStack CLI.

With this patch, one can use this manifest to configure the CloudStack provider:

provider "cloudstack" {
  config = "${pathexpand("~/.cloudstack.ini")}"
  region = "my_region"
}

mcanevet · 2017-05-10T12:38:40Z

@svanharmelen could you please review this?

svanharmelen · 2017-05-10T21:55:21Z

@mcanevet I must admit that I'm not in favour of adding this customization.

The configuration file you refer to is an Exoscale specific config file that is not part of Apache CloudStack. There are more cloud providers out there that add their own magic sauce or add tools to standard Apache CloudStack, but of course this single provider cannot serve all those use different flavours. So my goal is to keep this provider as close the Apache CloudStack as possible.

As an example you can think about CloudMonkey. CloudMonkey also uses an ini style config file, but with a different layout (different fields). So now we already have 2 different formats. And I'm very sure there are more, but they are all customized. So why should we take one or the other? If anything I would argue that the CloudMonkey format is probably the most official one as that is hosted as the Apache CloudStack CloudMonkey project.

So I would very much stay away from adding something like this, sorry. Luckily there are already a couple of ways you can config the provider within TF itself (directly in the file, using cmdline vars, using a tfvars file or use env variables) so I hope one of those options can make it into your workflow instead.

mcanevet · 2017-05-11T07:03:06Z

@svanharmelen sounds legit.

However, I'm not a fan of any other way of configuring the provider:

hardcode credentials in the manifests or using tfvars file : we definitely don't want to commit credentials and each user has its own credentials,
Using cmdline vars or environment variables : way too dangerous, if one uses the wrong command line arguments or load the wrong environment variables, he may deploy something on the wrong region.

For the moment we are fetching credentials from pass-password datasource (#11381) using that kind of declaration (we could also use vault provider the same way):

data "pass_password" "cloudstack" {
  path = "foo/bar"
}

provider "cloudstack" {
  api_url    = "${data.pass_password.cloudstack.data["api_url"]}"
  api_key    = "${data.pass_password.cloudstack.data["api_key"]}"
  secret_key = "${data.pass_password.cloudstack.data["secret_key"]}"
}

Using that, we don't have to commit any credentials in the manifests and there is no risk of using wrong API keys because we hardcode the location of it in a shared password store.

BUT, all user of the team use the same API keys belonging to a specific user, not its personal API keys. This is really annoying, especially when a team member leaves and closes its Cloudstack account ; or when we want to look into Cloudstack logs to see who did what ; we definitely want to use one's personal API keys...

So the idea behind this PR is to be able to hardcode the location of a file in every team member's personal workstation containing personal API keys to access Cloudstack using this declaration:

provider "cloudstack" {
  config = "${pathexpand("~/.cloudstack.ini")}"
  region = "my_region"
}

Like we do for example for the AWS provider:

provider "aws" {
  profile = "foo"
  region = "eu-west-1"
}

I totally agree the Exoscale's Cloustack API client CLI is not the most used, but I guess it works for every Cloudstack deployment and to be honest I knew that it cloud be criticized because of that.

Would you be agree with a PR implementing the same thing but using CloudMonkey file format?

mcanevet · 2017-05-17T14:43:03Z

@svanharmelen I'm now using CloudMonkey's config file. Is it OK for you now?

svanharmelen

@mcanevet I can life with adding this feature (while still not a fan of it), but I do have some comments we need to address first...

One thing that is not in the comments but should also be added is better restrictions on the schema fields (the params). Mainly the ConflictsWith option should be added to make sure either the config param if used or the api URL/key/secret and never both...

Also since now all fields are optional the validation of the provider will not work anymore as no values at all would now also be valid.

So there needs to be a proper validation that will return any errors during runtime. Not nice, but for that one I don't see another option...

svanharmelen · 2017-05-18T08:31:29Z

builtin/providers/cloudstack/provider.go

 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/terraform"
+	"os/user"


Please use goimports to order the imports

svanharmelen · 2017-05-18T08:52:46Z

builtin/providers/cloudstack/provider.go

 )

 // Provider returns a terraform.ResourceProvider.
 func Provider() terraform.ResourceProvider {
+	usr, _ := user.Current()


Calling this without checking the error is asking for problems...

svanharmelen · 2017-05-18T08:56:24Z

builtin/providers/cloudstack/provider.go

+			"config": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  fmt.Sprintf("%s/.cloudmonkey/config", usr.HomeDir),


And here is your problem 😉 If the call to user.Current() failed, usr would be nil and calling usr.HomeDir would cause a panic.

svanharmelen · 2017-05-18T10:16:08Z

builtin/providers/cloudstack/provider.go

-				Required:    true,
-				DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_API_URL", nil),
+				Optional:    true,
+				DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_API_URL", ""),


Why would you change this from nil to ""?

I thought the validation would fail if neither the environment variable nor the parameter was set with nil, but I tested and looks to be ok.

svanharmelen · 2017-05-18T10:17:48Z

builtin/providers/cloudstack/provider.go

-				Required:    true,
-				DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_SECRET_KEY", nil),
+				Optional:    true,
+				DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_SECRET_KEY", ""),
 			},


Please put the new options below the secret_key, as the existing api_url, api_key and secret_key would be the main way to use this provider, and alternatively you can point to a config file. So let's order the parameters in that same way...

svanharmelen · 2017-05-18T10:23:42Z

builtin/providers/cloudstack/provider.go

+
+		section, err := config.GetSection(d.Get("profile").(string))
+		if err != nil {
+			return config, err


Please return a nil instead of the config.

svanharmelen · 2017-05-18T10:24:52Z

builtin/providers/cloudstack/provider.go

+		}
+	}
+
+	if apiURL == "" {


Why do you only check the URL? We need to check the key and secret as well right?

svanharmelen · 2017-05-18T10:28:04Z

builtin/providers/cloudstack/provider.go

+
+		if secretKey == "" {
+			secretKey = section.Key("secretkey").String()
+		}


Why do you only assign the config values if the variables are empty? Either all the values come from the config file, or they all come from the individual api_url, api_key and secret_key parameters. To prevent unexpected behaviour they should never be mixed...

I'm trying to mimic the aws provider behavior where you can override the parameters in the ~/.aws/credentials on provider instantiation.

As explained in the comment above, I don't think we should add this option here. Let's make this as clear as possible and either use one or the other...

svanharmelen · 2017-05-18T10:32:56Z

builtin/providers/cloudstack/provider.go

+	apiKey := d.Get("api_key").(string)
+	secretKey := d.Get("secret_key").(string)
+
+	if configFile := d.Get("config").(string); configFile != "" {


I think it's cleaner to check for this like so:

if configFile, ok := d.GetOk("config"); ok { config, err := ini.Load(configFile.(string)) <snip>

This would give an error if you did set the config param, but assigned it an empty string (the ini.Load() func would return an error in that case). I think that would be more clear for users.

svanharmelen · 2017-05-18T10:33:28Z

builtin/providers/cloudstack/provider.go

+			"profile": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "",


If the default is an empty string, then please don't set a default...

mcanevet · 2017-05-18T13:40:30Z

@svanharmelen I fixed most of your comment. I'm not sure we should mark parameters as conflicts as we should be able to override the config file we either the environment variables or hardcoded parameters, just like the aws provider does.

svanharmelen

See my additional comments...

Additionally I still miss the more strict ConflictsWith options, which I believe add value.

And a validation that checks if either both a config and profile are given, of the api url/key/secret trio.

Since we don't have the required options set anymore, we need to validate the given info ourselves.

svanharmelen · 2017-05-18T13:46:29Z

builtin/providers/cloudstack/provider.go

+	usr, err := user.Current()
+	if err != nil {
+		return nil
+	}


This is still dangerous as the calling side of this function does not expect nil, but assumes a *schema.Provider. So when you do this and the calling side calls anything on the returned value, it will again panic.

svanharmelen · 2017-05-18T13:52:30Z

builtin/providers/cloudstack/provider.go

+			"config": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  fmt.Sprintf("%s/.cloudmonkey/config", usr.HomeDir),


I more or less understand your take on this one, but I don't think we should mimic the AWS provider. This is a new feature that is not in any way something CloudStack users expect. I'm OK with adding it, but I would like to make sure the use of it is as clear as possible.

So either you use the file or the inline fields. But not a combination of both. I don't see the use case that would benefit from being able to do that. I only see the possible confusion this may cause users.

So please remove this default which will also solve the error handling thingy when calling user.Current() as that can then be deleted as well...

svanharmelen · 2017-05-18T13:53:42Z

builtin/providers/cloudstack/provider.go

+
+		if secretKey == "" {
+			secretKey = section.Key("secretkey").String()
+		}


As explained in the comment above, I don't think we should add this option here. Let's make this as clear as possible and either use one or the other...

svanharmelen · 2017-05-18T13:54:27Z

builtin/providers/cloudstack/provider.go

+	apiKey := d.Get("api_key").(string)
+	secretKey := d.Get("secret_key").(string)
+
+	if profile, ok := d.GetOk("profile"); ok {


I would revert this to checking the config field as that makes more sense (for people reading the code)

mcanevet · 2017-05-18T14:17:50Z

@svanharmelen I added ConflictsWith but somehow it looks like it does not work. I can mix parameters without Terraform complaining... Do you have any idea?

mcanevet · 2017-05-19T12:00:17Z

@svanharmelen do you have any idea for ConflictsWith?

svanharmelen · 2017-05-19T19:17:41Z

I've been pretty busy today with some other PR's. I'll have a look at this one again coming Monday! From the top of my head I have no idea why this wouldn't work. So have to take a closer look...

mcanevet · 2017-05-24T12:10:21Z

@svanharmelen did you find time to check this PR?

svanharmelen · 2017-05-24T13:20:10Z

@mcanevet I just downloaded and compiled your branch and it works as expected:

mcanevet · 2017-05-24T13:21:45Z

@svanharmelen cool, I must have do something wrong during my tests.
Could you please merge then?

svanharmelen · 2017-05-24T13:22:59Z

I'll merge it and then add a small validation func to improve the UX just bit. Thanks!

ghost · 2020-04-12T02:00:08Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

tombuildsstuff added enhancement provider/cloudstack labels Apr 25, 2017

mcanevet force-pushed the cloudstack_config branch from c2ad232 to 00f78e9 Compare May 17, 2017 14:42

svanharmelen suggested changes May 18, 2017

View reviewed changes

mcanevet force-pushed the cloudstack_config branch from 00f78e9 to d893b14 Compare May 18, 2017 13:30

svanharmelen suggested changes May 18, 2017

View reviewed changes

Load Cloudstack configuration from file

fcfe465

mcanevet force-pushed the cloudstack_config branch from 3fe02a4 to fcfe465 Compare May 18, 2017 14:11

Add ConflictsWith

2d89a4d

svanharmelen approved these changes May 24, 2017

View reviewed changes

svanharmelen merged commit e4bd7e2 into hashicorp:master May 24, 2017

mcanevet deleted the cloudstack_config branch May 24, 2017 13:30

ghost locked and limited conversation to collaborators Apr 12, 2020

Load Cloudstack configuration from file #13926

Load Cloudstack configuration from file #13926

Conversation

mcanevet commented Apr 25, 2017

mcanevet commented May 10, 2017

svanharmelen commented May 10, 2017

mcanevet commented May 11, 2017

mcanevet commented May 17, 2017

svanharmelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcanevet commented May 18, 2017

svanharmelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcanevet commented May 18, 2017

mcanevet commented May 19, 2017

svanharmelen commented May 19, 2017

mcanevet commented May 24, 2017

svanharmelen commented May 24, 2017

mcanevet commented May 24, 2017

svanharmelen commented May 24, 2017

ghost commented Apr 12, 2020