xenserver / terraform-provider-xenserver

XenServer provider for Terraform
Other
19 stars 5 forks source link

host, username, and password provider attributes cannot be set from environment as documented #58

Closed jrandall closed 2 weeks ago

jrandall commented 2 months ago

It appears that the current version of the provider has marked the three provider attributes (host, username, and password) as required such that terraform will not allow them to be unspecified in the provider block (https://github.com/xenserver/terraform-provider-xenserver/blob/master/xenserver/provider.go#L55-L70).

The inline documentation says that these values can be set using the environment variables XENSERVER_HOST, XENSERVER_USERNAME, and XENSERVER_PASSWORD and there is logic to set the values from the environment if available, then override them with the provider block attributes, then finally check if any are unset and provide useful error messages to the user: https://github.com/xenserver/terraform-provider-xenserver/blob/master/xenserver/provider.go#L115-L158

However, I believe that code path cannot be reached because the Required: true on the attributes in the provider schema means that terraform will insist on all three being set before even invoking the provider. Unfortunately, because the logic order is for non-null values in the provider attributes to override the environment, the possible workaround to just set those to nonsense values and put the real values in the environment does not work either.

I believe the simplest fix for this is to update the schema and mark all three provider attributes as Required: false (or omit it) and set Optional: true since they are optional with respect to the provider configuration block, and rely on the logic already present in the code to handle the case if they cannot be found either from the environment or the provider attributes.

e.g. this might look something like this:

            "host": schema.StringAttribute{
                MarkdownDescription: "The base URL of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_HOST**.",
                Optional: true,
            },
            "username": schema.StringAttribute{
                MarkdownDescription: "The user name of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_USERNAME**.",
                Optional: true,
            },
            "password": schema.StringAttribute{
                MarkdownDescription: "The password of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_PASSWORD**.",
                Optional:  true,
                Sensitive: true,
            },

Alternatively, if you would like to retain the idea that specifying these logical values is in fact required, you could retain the Required: true but also set DefaultFunc to a function that performs the GetEnv lookup (and then remove the logic in https://github.com/xenserver/terraform-provider-xenserver/blob/master/xenserver/provider.go#L115-L158).

e.g. this might look something like this:

            "host": schema.StringAttribute{
                MarkdownDescription: "The base URL of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_HOST**.",
                Required: true,
                DefaultFunc: func() (any, error) {
                    if v := os.Getenv("XENSERVER_HOST"); v != "" {
                        return v, nil
                    } 
                    return nil, nil
                }
            },
            "username": schema.StringAttribute{
                MarkdownDescription: "The user name of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_USERNAME**.",
                Required: true,
                DefaultFunc: func() (any, error) {
                    if v := os.Getenv("XENSERVER_USERNAME"); v != "" {
                        return v, nil
                    } 
                    return nil, nil
                }
            },
            "password": schema.StringAttribute{
                MarkdownDescription: "The password of target XenServer host." + "<br />" +
                    "Can be set by using the environment variable **XENSERVER_PASSWORD**.",
                Required:  true,
                Sensitive: true,
                DefaultFunc: func() (any, error) {
                    if v := os.Getenv("XENSERVER_PASSWORD"); v != "" {
                        return v, nil
                    } 
                    return nil, nil
                }
            },
acefei commented 1 month ago

@jrandall Thank you for your information. It seems that the example you provided is for Terraform SDK V2. In any case, we will consider update the logic.

acefei commented 2 weeks ago

This was fixed in v0.2.0.