add `secretmanager` resources and `secret_version` data source #3007

megan07 · 2020-01-22T20:55:28Z

Fixes hashicorp/terraform-provider-google#5168

Added secretmanager service with secret and secret_version resources and secret_version data source. I have not yet added the ability to enable/disable the secret_version resource.

Release Note Template for Downstream PRs (will be copied)

secretmanager: added `secret` and `secret_version` resources and `secret_version` datasource

modular-magician · 2020-01-22T21:01:11Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-22T21:05:11Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, fda1c6d.

Pull request statuses

No diff detected in terraform-google-conversion.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1669
depends: hashicorp/terraform-provider-google#5473

modular-magician · 2020-01-22T22:25:31Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-22T22:30:45Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, cebf43e.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

slevenick · 2020-01-23T18:10:53Z

products/secretmanager/terraform.yaml

+      - !ruby/object:Provider::Terraform::Examples
+        name: "secret_version_basic"
+        primary_resource_id: "secret-version-basic"
+        primary_resource_name: "fmt.Sprintf(\"test-secret-version-basic%s\", context[\"random_suffix\"])"


I don't think you need this field if there is no IAM policy for this resource

templates/terraform/custom_expand/bool_to_object.go.erb

slevenick · 2020-01-23T18:17:14Z

third_party/terraform/data_sources/data_source_secretmanager_secret_version.go.erb

+	parts := secretVersionRegex.FindStringSubmatch(version["name"].(string))
+	// should return [full string, project number, secret name, version number]
+	if len(parts) != 4 {
+		return fmt.Errorf("secret name, %s, does not match format, projects/{{project}}/secrets/{{secret}}/versions/{{versions}}", version["name"].(string))


{{versions}} -> {{version}}

slevenick · 2020-01-23T18:19:45Z

third_party/terraform/tests/data_source_secretmanager_secret_version_test.go.erb

+}
+
+data "google_secretmanager_secret_version" "basic" {
+	provider = google-beta


Mixed tabs + spaces in this file

slevenick · 2020-01-23T18:21:32Z

..._party/terraform/website/docs/d/datasource_google_secretmanager_secret_version.html.markdown

+
+The following attributes are exported:
+
+* `secret_data` - The secret data. No larger than 64KiB. A base64-encoded string.


Why do we have a data source that exposes secret_data when we are using ignore_read on the field in the TF resource? This field corresponds to SecretVersion.payload.data right?

Ah, read the ds more closely. We make a separate call to :access to retrieve the data?

Yea. I wasn't exactly sure how to handle this in the resource, and it's the same case with :enable and :disable, they're all separate calls. I'm going to spend some more time looking into it, but if you have any suggestions, please let me know!

slevenick · 2020-01-23T18:32:05Z

third_party/terraform/data_sources/data_source_secretmanager_secret_version.go.erb

+
+	parts := secretVersionRegex.FindStringSubmatch(version["name"].(string))
+	// should return [full string, project number, secret name, version number]
+	if len(parts) != 4 {


There is no way this should fail, right?

version["name"] is coming back from the API, not user input. If it isn't in the correct format then something has gone horribly wrong and the user cannot fix it

I believe so. Does this mean we don't want the error here? I just had it in as a just-in-case since we access parts[3] later.

It's probably fine. It would be confusing for a user to see this error and think that they need to fix it, and a panic might be more obvious that the error is in the provider (or API), but I don't have strong opinions and checking before array access seems like a good idea

slevenick · 2020-01-23T18:36:53Z

third_party/terraform/data_sources/data_source_secretmanager_secret_version.go.erb

+	versionNum := d.Get("version")
+
+	if versionNum != "" {
+		url, err = replaceVars(d, config, "{{SecretmanagerBasePath}}projects/{{project}}/secrets/{{secret}}/versions/{{version}}")


These will always pick up the default project config rather than the project contained in secret if it is a long name?

If the config looks like:

provider { project = "my-other-project" } "google_secret_version" "my-secret" { secret = "projects/my-project1/secrets/my-secret" }

We will pick up project as "my-other-project" I believe

Ah, yes, that makes sense. Should I d.Set("project", fv.Project) up by line 65 then?

But then we will override an explicit project set on the secret_version itself. Like:

"google_secret_version" "my-secret" { project = "my-other-project" secret = "projects/my-project1/secrets/my-secret" }

But that might be ok? Maybe check d.Get("project") and if set & not equal to fv.Project then throw an error. Otherwise d.Set("project", fv.Project)?

slevenick · 2020-01-23T18:44:26Z

products/secretmanager/terraform.yaml

+# limitations under the License.
+--- !ruby/object:Provider::Terraform::Config
+overrides: !ruby/object:Overrides::ResourceOverrides
+  Secret: !ruby/object:Overrides::Terraform::ResourceOverride


I think you will need to add custom import_formats due to the URL containing secret_id instead of name. If you look at https://github.com/terraform-providers/terraform-provider-google-beta/pull/1669/files#diff-cc818fd7ea3159b3e18bf79d36939d33R283

I think this will cause imports to fail.

I would also add handwritten import tests because we don't generate them for beta resources

slevenick · 2020-01-23T18:48:24Z

products/secretmanager/terraform.yaml

+        custom_flatten: templates/terraform/custom_flatten/object_to_bool.go.erb
+        custom_expand: templates/terraform/custom_expand/bool_to_object.go.erb
+
+  SecretVersion: !ruby/object:Overrides::Terraform::ResourceOverride


How will import work for this resource if the important part (payload.data) is ignore_read?

I'm not sure. I just wrote the test, and it doesn't work. Hopefully, if I can figure out how to deal with the :access, :enable and :disable on this resource, it will solve this as well.

Possibly a custom flatten on payload.data that calls the :access endpoint?

We have delete_url that you can specify on a resource, but I'm not sure how the :enable and :disable actually work for this API

slevenick · 2020-01-23T19:04:16Z

products/secretmanager/api.yaml

+                      description: |
+                        The canonical IDs of the location to replicate data. For example: "us-east1".
+  - !ruby/object:Api::Resource
+    name: SecretVersion


Is this updatable? Looking at the API it doesn't look like it

modular-magician · 2020-01-23T21:48:04Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-23T22:01:17Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff
Ansible: Diff
TF Conversion: Diff
Inspec: Diff

modular-magician · 2020-01-23T22:05:58Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 70e7f73.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

rileykarson · 2020-01-23T23:02:10Z

products/secretmanager/api.yaml

+# limitations under the License.
+
+--- !ruby/object:Api::Product
+name: Secretmanager


Suggested change

name: Secretmanager

name: SecretManager

I think google_secret_manager_ is more in line with the naming convention we tend to use on new products. Is there something I'm missing that would make us prefer the secretmanager slug?

Thank you! I had that originally then changed my mind after looking at a different service. I'll change it back, as I like this better too. Thanks!

slevenick · 2020-01-25T00:44:22Z

templates/terraform/custom_expand/bool_to_object.go.erb

+	# See the License for the specific language governing permissions and
+	# limitations under the License.
+-%>
+func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {


FYI I copied this & object_to_bool into another PR that is in master now, so when you rebase they should already exist

megan07 · 2020-01-28T21:17:36Z

provider/terraform.rb

@@ -94,7 +94,7 @@ def updatable?(resource, properties)

    def force_new?(property, resource)
      !property.output &&
-        (property.input || (resource.input && property.update_url.nil? &&
+        (property.input || (resource.input && property.update_url.nil? && property.input.nil? &&


I added this in here so that if a resource is input: true, a child property can overwrite that. In this case, I added an Update method in the resource_definition that would allow enabled to be updated (even though SecretVersion has no update method, only :enable or :disable), when the resource technically can't be updated. This is an assumption on my part that an explicit input: false on a child property can override this, but I'm not familiar with Ruby, or with all resources in the project to know if this will be problematic or not.

Is there a reason to not mark the individual fields that cannot be updated as input: true and leave the enabled as the only one that is not input or output?

I think this would accomplish the same thing without weird input negation via overridden properties

I tried that, however then it seems to generate the Update function, and in the generated Update function it makes a Put request, which isn't an option on the object. So I needed an Update function that only called the expand function (because its in that function that I call the :enable or :disable. Does that make sense?

Ah yeah, and you can only have a single endpoint as the update_url on a field. In general you can have custom update_url and update_verb or something, but I don't think that will work in this case because it has to be :enable or :disable depending on the state

modular-magician · 2020-01-28T21:18:38Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

megan07 · 2020-01-28T21:21:45Z

...party/terraform/website/docs/d/datasource_google_secret_manager_secret_version.html.markdown

+
+* `destroy_time` - The time at which the Secret was destroyed. Only present if state is DESTROYED.
+
+* `state` - The current state of the SecretVersion.


Should I change this to be enabled to be consistent with the resource? Or since it's read-only, does it matter?

I'd be consistent with the resource, but I don't think it's a big deal?

modular-magician · 2020-01-28T21:45:45Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

slevenick · 2020-01-29T21:08:40Z

provider/terraform.rb

@@ -94,7 +94,7 @@ def updatable?(resource, properties)

    def force_new?(property, resource)
      !property.output &&
-        (property.input || (resource.input && property.update_url.nil? &&
+        (property.input || (resource.input && property.update_url.nil? && property.input.nil? &&


Is there a reason to not mark the individual fields that cannot be updated as input: true and leave the enabled as the only one that is not input or output?

I think this would accomplish the same thing without weird input negation via overridden properties

slevenick · 2020-01-29T21:19:05Z

templates/terraform/custom_flatten/secret_version_access.go.erb

+	accessRes, err = sendRequest(config, "GET", project, url, nil)
+	if err != nil {
+		// if this secret version is disabled, the api will return an error, as the value cannot be accessed
+		if strings.Contains(err.Error(), "is not in ENABLED state") && d.Get("enabled").(bool) == false {


What would happen if the API error message changed at some point in the future? Trying to read or do any operations on a disabled secret version would throw an error, right?

Is there a way we can check the state of enabled and if it is disabled, not even make this call?

Though it might entail depending on the order of the calls of the flatten functions, and I'm not sure that's a change we want to make

Yes, that's correct. The error bubbles up to the flattenedProp loop, which is less than ideal because it panics when it tries to cast it into a string, but yea - I suppose I can add a check in there to see if it's an error so that it errors nicely in that case.
I tried checking the state of enabled, but yea, it was an order deal, I think. I'll run through again and see specifically what case that was in. I did a lot of testing/moving around code with this one, so it could be that after I fixed something else, this might work now.

I would try to get this flatten call to happen after we flatten the state so that we can check d.Get("state") and not make the :access call if state is disabled. Maybe moving the fields around within api.yaml can fix it?

That worked! Thanks!

modular-magician · 2020-01-30T20:17:49Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

slevenick · 2020-01-30T20:38:12Z

third_party/terraform/data_sources/data_source_secret_manager_secret_version.go.erb

+
+	// Get the payload
+	url = fmt.Sprintf("%s:access", url)
+	resp, err := sendRequest(config, "GET", project, url, nil)


This will error if the secret version is disabled. Do we want to check the state here and only read the payload if it is enabled?

Or throw an error saying that we can't retrieve the payload if it is disabled

Good question. I updated it so that secret_data would just return "" on disabled, but I can see both ways.
In one way, throwing an error would stay consistent with the API, but in another way (and I'm not sure of a use case for this) if the user wanted to see the other information associated with the version, it would still retrieve that information.
I could go either way.

I'd lean towards not throwing an error if we know how to prevent it. The downside of returning "" is pretty minimal

…secret_data when disabled

modular-magician · 2020-01-30T21:57:48Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff
Terraform Beta: Diff

modular-magician · 2020-01-31T20:48:04Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 135 files changed, 3146 insertions(+), 3199 deletions(-))
Terraform Beta: Diff ( 164 files changed, 4164 insertions(+), 6720 deletions(-))

sethvargo · 2020-01-31T22:04:57Z

products/secretmanager/terraform.yaml

+      payload.data: !ruby/object:Overrides::Terraform::PropertyOverride
+        name: secret_data
+        sensitive: true
+        custom_expand: templates/terraform/custom_expand/base64.go.erb


@megan07 @slevenick - the secret should not need to be base64-encoded. That feels like unnecessary user friction.

…leCloudPlatform#3007)

googlebot added the cla: yes label Jan 22, 2020

megan07 requested a review from slevenick January 22, 2020 20:58

modular-magician added downstream-generated terraform-provider-google-beta terraform-provider-google labels Jan 22, 2020

luisgreen approved these changes Jan 23, 2020

View reviewed changes

slevenick requested changes Jan 23, 2020

View reviewed changes

slevenick reviewed Jan 23, 2020

View reviewed changes

rileykarson reviewed Jan 23, 2020

View reviewed changes

slevenick mentioned this pull request Jan 24, 2020

Add support for new beta non-rfc fields in dns managed zone #3026

Merged

slevenick reviewed Jan 25, 2020

View reviewed changes

megan07 added 9 commits January 28, 2020 12:52

add secret manager secret and secret versions

edd431c

add secret_data to secret version datasource

ca65949

add new lines at end of files

8ac272f

update datasource docs

9110c6e

move package outside unless

ec70c21

fix comments from review (round 1)

8d71ccf

update secret tests

1fd9039

update to secret_manager

8310904

add enable/disable/access to secret version resource

21b1eaf

final updates

3a04762

megan07 force-pushed the megan_secrets_manager branch from 70e7f73 to 3a04762 Compare January 28, 2020 21:12

megan07 commented Jan 28, 2020

View reviewed changes

address last of review comments

c9b906e

megan07 requested a review from slevenick January 28, 2020 22:18

slevenick reviewed Jan 29, 2020

View reviewed changes

switch state and payload in api.yaml

dfb8c8c

slevenick reviewed Jan 30, 2020

View reviewed changes

update datasource from state to enabled, and return empty string for …

b9c025d

…secret_data when disabled

slevenick self-requested a review January 31, 2020 17:33

slevenick approved these changes Jan 31, 2020

View reviewed changes

throw error if datasource version is disabled

f363d1c

megan07 merged commit dcadfcf into GoogleCloudPlatform:master Jan 31, 2020

sethvargo reviewed Jan 31, 2020

View reviewed changes

nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020

add secret_manager resources and secret_version data source (Goog…

8409367

…leCloudPlatform#3007)


		The following attributes are exported:

		* `secret_data` - The secret data. No larger than 64KiB. A base64-encoded string.


		* `destroy_time` - The time at which the Secret was destroyed. Only present if state is DESTROYED.

		* `state` - The current state of the SecretVersion.

add secretmanager resources and secret_version data source #3007

add secretmanager resources and secret_version data source #3007

Conversation

megan07 commented Jan 22, 2020 • edited Loading

modular-magician commented Jan 22, 2020

Diff report:

modular-magician commented Jan 22, 2020

Pull request statuses

New Pull Requests

modular-magician commented Jan 22, 2020

Diff report:

modular-magician commented Jan 22, 2020

Pull request statuses

New Pull Requests

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

modular-magician commented Jan 23, 2020

Diff report:

modular-magician commented Jan 23, 2020

Diff report:

modular-magician commented Jan 23, 2020

Pull request statuses

New Pull Requests

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

modular-magician commented Jan 28, 2020

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jan 28, 2020

Diff report:

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

modular-magician commented Jan 30, 2020

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jan 30, 2020

Diff report:

modular-magician commented Jan 31, 2020

Diff report:

Choose a reason for hiding this comment

add `secretmanager` resources and `secret_version` data source #3007

add `secretmanager` resources and `secret_version` data source #3007

megan07 commented Jan 22, 2020 •

edited

Loading