Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

d/ssm_parameter: Return a default value if parameter cannot be found in AWS (optional) #3945

Closed
wants to merge 4 commits into from

Conversation

DanielRis
Copy link

@DanielRis DanielRis commented Mar 28, 2018

I tried to make it comparable with the behavior of d/consul_keys (https://www.terraform.io/docs/providers/consul/d/keys.html) but with an option to enable/disable this behavior to avoid any compatibility issues

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSsmParameterDataSource'
TF_ACC=1 go test ./aws -v -run=TestAccAWSSsmParameterDataSource -timeout 120m
=== RUN   TestAccAWSSsmParameterDataSource_basic
--- PASS: TestAccAWSSsmParameterDataSource_basic (85.00s)
=== RUN   TestAccAWSSsmParameterDataSource_defaultValue
--- PASS: TestAccAWSSsmParameterDataSource_defaultValue (37.30s)
=== RUN   TestAccAWSSsmParameterDataSource_fullPath
--- PASS: TestAccAWSSsmParameterDataSource_fullPath (53.61s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       178.397s

Use case description

I only want to build a AWS resource if a parameter was configured by operations or terraform.

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 28, 2018
@radeksimko radeksimko added service/ssm Issues and PRs that pertain to the ssm service. enhancement Requests to existing resources that expand the functionality or scope. labels Mar 28, 2018
@AndHei
Copy link
Contributor

AndHei commented Jun 1, 2018

@radeksimko this is super useful! please merge!

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. size/XXL Managed by automation to categorize the size of a PR. labels Jun 1, 2018
Optional: true,
Default: "",
},
<<<<<<< HEAD

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielRis Have you noticed the merge conflicts in several files?

@@ -29,7 +29,8 @@ The following arguments are supported:

* `name` - (Required) The name of the parameter.
* `with_decryption` - (Optional) Whether to return decrypted `SecureString` value. Defaults to `true`.

* `with_default` - (Optional) Whether to return a default value if no parameter can be found. Defaults to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with_default is not needed. Don't provide default if you don't need to get a default value back.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I'm unsure how this can be implemented without breaking the actual behavior of failing if a SSM parameter cannot be found in SSM. Thats the reason why i created this "with_default" parameter.

If I skip "with_default" and set the Default value of "default" to be an empty string, I currently have no idea how to catch that in my code without returning a default value every time the parameter cannot be found. Any idea?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I figured it out by my self.

@DanielRis DanielRis force-pushed the ssm-default-value branch from 4374f1c to c1eb34d Compare June 2, 2018 21:02
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jun 2, 2018
@tbondarchuk
Copy link

@antonbabenko Hi, any news on getting this merged?

@jakowicz
Copy link

It would be super useful if this could get merged, please.

@ixalon
Copy link

ixalon commented Jul 23, 2018

Hi @DanielRis - great PR, we could really do with this! Any chance you could please fix the conflict?

@antonbabenko
Copy link
Contributor

@bflad Could you tell what is missing here other than fixing conflicting file?

We can't wait for the next Terraform Gardering event to get this merged, can we? :)

@ghost ghost added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Oct 1, 2018
@DanielRis
Copy link
Author

I updated the PR to reflect that latest change that was implemented into this Data Resource. The lookup method was changed from GetParameters to GetParameter which caused the merge conflicts.

TF_ACC=1 go test ./aws -v -run=TestAccAWSSsmParameterDataSource -timeout 120m === RUN TestAccAWSSsmParameterDataSource_basic --- PASS: TestAccAWSSsmParameterDataSource_basic (20.08s) === RUN TestAccAWSSsmParameterDataSource_defaultValue --- PASS: TestAccAWSSsmParameterDataSource_defaultValue (10.68s) === RUN TestAccAWSSsmParameterDataSource_fullPath --- PASS: TestAccAWSSsmParameterDataSource_fullPath (12.14s) PASS ok github.com/DanielRis/terraform-provider-aws/aws 42.918s

@DanielRis
Copy link
Author

DanielRis commented Oct 13, 2018

I did some additional tests and I still have not found a good way to determine if a property has been set in a configuration.

data "aws_ssm_parameter" "foo" {
  name  = "foo"
  default = "bar"
}

works fine.

data "aws_ssm_parameter" "foo" {
  name  = "foo"
  default = ""
}

does not.

Is there any good way to check if a property is set in the configuration, independent of the value it has? I thought GetOkExist will do the job but this does not work for empty string values.

@bflad
Copy link
Contributor

bflad commented Oct 17, 2018

Hi Folks 👋 It would be helpful to understand some of the expected use cases for this functionality.

Outside of the consul_keys data source, the general Terraform design pattern for a data source requires that its referenced API resource (an SSM parameter in this case), actually exists or returns an error. Allowing this data source to be able to purposefully workaround that behavior (and also generate failing API calls) might not be the best way to handle this situation. It could also be confusing for operators that this data source works differently compared to others.

None of this is to say that we won't accept a change like this; its just a different class of problem and it would be nice to know some of the reasoning behind why the behavior change should be allowed. 😄

@gdlx
Copy link
Contributor

gdlx commented Feb 25, 2019

Hi @bflad, here is my use case:

We're using blue/green deployment and the active environment name is currently stored in Consul.
As we're only using Consul for the key/value store, we'd like to migrate to SSM parameters.
Everything works fine on an existing (already initialized) platform, but the lack of SSM parameter default value prevent us from initializing a new one:

The resource aws_ssm_parameter can store a value but it depends on the value returned by the data aws_ssm_parameter...which falls into error when the param isn't set.

The consul_keys we're (still) using has a default option, which allows us to return a value when the key is not set yet. This way, a new (test/dev) platform can be initialized seamlessly. In the current state of aws_ssm_parameter we'd have to initialize it manually, which is not acceptable.

You'll find some other cases here: hashicorp/terraform#16380

@AndHei @aliusmiles @jakowicz @ixalon : Can you add your use cases to help merging this PR ?

Thanks !

@sharmaansh21
Copy link
Contributor

@gdlx I am also facing the same issue :-(

@gdlx
Copy link
Contributor

gdlx commented Mar 6, 2019

@sharmaanshul2102 Can you please explain your use case as requested by @bflad ? This will help merging or fixing the PR.

Thanks !

@sharmaansh21
Copy link
Contributor

sharmaansh21 commented Mar 6, 2019

resource "aws_ssm_parameter" "test" {
  name        = "test"
  type        = "String"
  value       = "${var.test_version}"
  description = "Stores the latest version value of the test service."
  tags        = "${merge(local.common_tags, map("Name", "test"))}"
  overwrite   = true

  lifecycle {
    ignore_changes = ["value"]
  }
}

data "aws_ssm_parameter" "test" {
  name = "test"
}

It tries to read data and fails because aws_ssm_parameter doesn't exist in first run :-(

@tbondarchuk
Copy link

We are using SSM Parameters to store config variables in order to:

  1. Allow external configuration: update SSM parameter in AWS console then run Jenkins job to deploy environment

  2. Store environment specific variables: instance_type_ssm_path = "/${var.environment}/instance_type", with /dev/instance_type=t2.micro and /prod/instance_type=c5.large

Having default value for data "aws_ssm_parameter" allow us to set some common default values directly in terraform code without creating a lot of SSM parameters, and then create parameters only if needed.

@aeschright aeschright requested a review from a team June 25, 2019 19:23
@nywilken
Copy link
Contributor

Hey folks thanks for shedding a little light on the need. Unfortunately, given that the provided usage cases are specific to solving a particular build/deploy process we are unable to accept this change, as it is not a use case that is valid to the provider.

Data sources are expected to require existing infrastructure. Changing the behaviour of this one data source would make it inconsistent with the other data sources in the provider, and in most other providers (minus the Consul provider). In general, Terraform prefers the use of explicit configuration that represents the true state of the reference infrastructure over conditionally defined defaults.

If you still feel like this is something worth discussing I recommend starting a discussion on our community forum https://discuss.hashicorp.com/c/terraform-providers as a next step.

@nywilken nywilken closed this Jul 17, 2019
@ghost
Copy link

ghost commented Nov 2, 2019

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ssm Issues and PRs that pertain to the ssm service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.