provider/kubernetes: Add secret resource #12960

mrooding · 2017-03-22T13:31:55Z

First resource contribution to the new kubernetes provider to get the hang of it.

I've tested it using a dev build in combination with the google provider for both a string literal secret and a secret read from file.

ctavan · 2017-03-24T12:01:04Z

@mrooding awesome contribution, I was just about to start implementing this as well when I discovered your PR!

I have also successfully tested it using a dev build where I cherry-picked your commit on top of the current master. I have successfully created secrets which hold ssl certificates for an https-ingress.

As opposed to configmaps, secrets however have an optional type, see https://github.com/kubernetes/community/blob/master/contributors/design-proposals/secrets.md#secret-api-resource, here's how a kubectl get ingress looks like:

NAME               TYPE                DATA      AGE
new-tls-ingress    Opaque              2         12m
old-tls-ingress    kubernetes.io/tls   2         1h

The old-tls-ingress was created manually, the new-tls-ingress was created using your resource.

While the Kubernetes ingress doesn't seem to care about the type of the secret (it accepts the new-tls-ingress secret of type Opaque!) I guess it would be nice to allow for configuring, what do you think?

ctavan · 2017-03-24T16:39:20Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+		return err
+	}
+
+	d.Set("data", secret.StringData)


I think when reading secret.Data has to be used instead of StringData since StringData is "provided as a write-only convenience method" as described in https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L4060

ctavan · 2017-03-24T16:45:40Z

While further testing your code I found that terraform kept updating the secrets with each run due to the issue I commented on in the code. I've fixed that issue in ctavan@6341110

I have furthermore implemented the secret type in ctavan@6a393bb

I suppose some more tests would be appropriate?!

ctavan · 2017-03-24T18:10:10Z

@radeksimko I assume we have to include the changes from #12945 here as well, I suppose?

mrooding · 2017-03-24T19:04:05Z

Seems like we were both already working on the fix ;) I have fixed both but was unable to respond sooner due to a meeting.

I also ran into the same bug today. However, in my tests, it wasn't enough to set d.Set("data", secret.Data) since the data format didn't match. I fixed it by transforming the map of byte arrays to a map of strings. Were you able to actually get it to work without this conversion?

My test cases:

resource "kubernetes_secret" "db-password" {
  metadata {
    name = "my-secret"
  }

  data {
    db_password = "P4ssw0rd"
  }

  type = "Opaque"
}

resource "kubernetes_secret" "docker-config" {
  metadata {
    name = "docker-config"
  }

  data {
    ".dockercfg" = "${file("/Users/mrooding/.docker/config.json")}"
  }

  type = "kubernetes.io/dockercfg"
}

In my documentation entry I added the link to the GO API documentation since I couldn't find a real documentation page on Kubernetes about secret types. Yours contains very specific information about 1 type of secret which might be to specific. What do you think?

I'll take a look at #12945 later on.

I'm all up for adding more resources so it might be a good idea to align with @radeksimko before we
waste time implementing the same resources?

ctavan · 2017-03-27T09:21:11Z

@mrooding your patch works equally well for me and I agree that your documentation link is definitely better (I found it pretty hard to figure out anything about that feature without reading the code).

So to sum up: Your current implementation works perfectly for me, thanks a lot!

I will for now also refrain from implementing anything else on this feature to prevent double work ;). So I guess I leave that issue up to you and @radeksimko now…

radeksimko · 2017-03-28T17:07:29Z

Hi folks,
thanks @mrooding for opening this PR.

I was reading up a bit on Kubernetes & Google Cloud access control management (IAM, service accounts etc.) and I'm not entirely convinced https://www.terraform.io/docs/providers/google/r/google_service_account.html is fully interchangeable with Kubernetes Service Account(s), but I may be wrong. I think we'll need to add a few more resources related to security/access (to make secrets "more secret").

I understand the user can specify the service account for launching a pod, which may in turn limit the access to secrets. I feel like we should add a warning to the docs of this resource either way:

the resource will by default (without the use of service accounts, roles, policies etc.) create secret which is available to any pod in the specified (or default) namespace - at least I think that's the default behaviour ❓ 🤔

Also, we should note that (as any other attributes of other resources in other providers) the secrets will be stored in plain-text in the state file which may increase the risk of exposure.

~~I will prepare a separate docs page you can link to explaining that problem~~
Please link to it like this:

~> **Note:** All arguments including the secret data will be stored in the raw state as plain-text.
[Read more about sensitive data in state](/docs/state/sensitive-data.html).

I will review the code in a moment.

radeksimko

I ran the attached acceptance tests and some of them were failing, but I think I explained the reasons in the review.

The code otherwise looks pretty good, so if you can look at my comments, it would be great. 😃

radeksimko · 2017-03-28T17:09:45Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+			"data": {
+				Type:        schema.TypeMap,
+				Description: "A map of the secret data.",
+				Optional:    true,


I think this field should be marked as Sensitive: true to avoid unintentional exposure of secrets in the plan/apply output.

radeksimko · 2017-03-28T17:12:24Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+				Type:        schema.TypeString,
+				Description: "Type of secret",
+				Optional:    true,
+				ForceNew:    true,


To avoid the few failures in attached acceptance tests and make the behaviour generally consistent we should make Opaque the default value here, i.e.

Default: "Opaque",

The other way to avoid diffs would be to mark it as Computed, but I think Default is ok here as the default value is simple static value which is unlikely to be changed.

radeksimko · 2017-03-28T17:14:41Z

builtin/providers/kubernetes/resource_kubernetes_secret_test.go

+					resource.TestCheckResourceAttrSet("kubernetes_secret.test", "metadata.0.uid"),
+					resource.TestCheckResourceAttr("kubernetes_secret.test", "data.%", "2"),
+					resource.TestCheckResourceAttr("kubernetes_secret.test", "data.username", "first"),
+					resource.TestCheckResourceAttr("kubernetes_secret.test", "data.password", "second"),


I think there's a typo. These should be admin/password.

radeksimko · 2017-03-28T17:16:06Z

builtin/providers/kubernetes/resource_kubernetes_secret_test.go

+		if len(expected) == 0 && len(m.Data) == 0 {
+			return nil
+		}
+		if !reflect.DeepEqual(m.Data, expected) {


I don't think this will work without an explicit conversion from map[string][]byte to map[string]string.

radeksimko · 2017-03-28T17:21:35Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+	data := map[string]string{}
+	for k, v := range secret.Data {
+		data[k] = string(v)
+	}


It would be nice to separate out this conversion logic into a function which we can then reuse in tests, as mentioned below, e.g. byteMapToStringMap(m map[string][]byte) map[string]string. By using such function it will also become more obvious what's happening here as on first sight it may not be obvious we're getting bytes.

radeksimko · 2017-03-28T17:25:50Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+	}
+
+	log.Printf("[INFO] Updating secret: %#v", secret)
+	out, err := conn.CoreV1().Secrets(namespace).Update(&secret)


I know this is probably going to be no-op in case of this resource since it doesn't get any internal K8S annotations by default, but would you mind changing it to Patch? That way we can keep the update behaviour consistent across all K8S resources + we can avoid stamping 👣 over any internal annotations that may appear in the future. 😃

Feel free to leverage what I have done for kubernetes_config_map:

terraform/builtin/providers/kubernetes/resource_kubernetes_config_map.go

Lines 80 to 94 in ac87865

ops := patchMetadata("metadata.0.", "/metadata/", d)

if d.HasChange("data") {

oldV, newV := d.GetChange("data")

diffOps := diffStringMap("/data/", oldV.(map[string]interface{}), newV.(map[string]interface{}))

ops = append(ops, diffOps...)

}

data, err := ops.MarshalJSON()

if err != nil {

return fmt.Errorf("Failed to marshal update operations: %s", err)

}

log.Printf("[INFO] Updating config map %q: %v", name, string(data))

out, err := conn.CoreV1().ConfigMaps(namespace).Patch(name, pkgApi.JSONPatchType, data)

if err != nil {

return fmt.Errorf("Failed to update Config Map: %s", err)

}

radeksimko · 2017-03-28T17:30:10Z

website/source/docs/providers/kubernetes/r/secret.html.markdown

+
+The resource provides mechanisms to inject containers with sensitive information, such as passwords, while keeping containers agnostic of Kubernetes.
+Secrets can be used to store sensitive information either as individual properties or coarse-grained entries like entire files or JSON blobs.
+


As this is sensitive topic, I feel we should take an extra step to educate the user and perhaps link to security properties and risks related to this resource?
https://kubernetes.io/docs/user-guide/secrets/#security-properties

radeksimko · 2017-03-28T17:34:47Z

website/source/docs/providers/kubernetes/r/secret.html.markdown

+
+* `data` - (Optional) A map of the secret data.
+* `metadata` - (Required) Standard secret's metadata. More info: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#metadata
+* `type` - (Optional) The secret type. Defaults to `Opaque`. More info: https://godoc.org/k8s.io/kubernetes/pkg/api#SecretType


It's shame these types aren't documented in the official docs, but how do you feel about linking to this document instead? https://github.com/kubernetes/community/blob/master/contributors/design-proposals/secrets.md#proposed-design - I think it's a bit more condensed and to-the-point compared to the godoc link.

mrooding · 2017-03-29T14:58:11Z

Thank you for the detailed feedback @radeksimko. I think that I've covered all the requested changes. I've also ran the integration test to make sure everything is working as expected.

The patch implementation had a bit of a challenge in the sense that the PatchOperation values have to be base64 encoded. I did not want to add this to patch_operations.go and eventually added it to the resourceKubernetesSecretUpdate function.

Let me know what you think!

ctavan · 2017-03-30T07:08:14Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+				Default:     "Opaque",
+				Optional:    true,
+				ForceNew:    true,
+				Sensitive:   true,


Ooops, that Sensitive: true ended up in the wrong place: Should be part of the data schema, not the type schema.

Good catch. Just updated it!

radeksimko

Just a few nitpicks left, but this otherwise looks pretty much ready for merge.

Would you mind addressing them? If not, I'm happy to pull this in and make these small changes myself afterwards.

Thanks 👍

radeksimko · 2017-03-30T07:53:55Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+		return fmt.Errorf("Failed to marshal update operations: %s", err)
+	}
+
+	log.Printf("[INFO] Updating secret %q: %v", name, string(data))


The casting here isn't necessary as PatchOperations (ops) have String() method, but it doesn't hurt.

radeksimko · 2017-03-30T07:55:08Z

builtin/providers/kubernetes/resource_kubernetes_secret.go

+
+	namespace, name := idParts(d.Id())
+
+	log.Printf("[INFO] Deleting secret: %#v", name)


I think %q or %s would be more appropriate in this case, but it's rather a nitpick.

radeksimko · 2017-03-30T08:03:45Z

website/source/docs/providers/kubernetes/r/secret.html.markdown

+Read more about security properties and risks involved with using Kubernetes secrets: https://kubernetes.io/docs/user-guide/secrets/#security-properties
+
+**Note:** All arguments including the secret data will be stored in the raw state as plain-text.
+[Read more about sensitive data in state](/docs/state/sensitive-data.html).


To make the two paragraphs (previous one starting with Read more about security properties ...) stand out, would you mind prefixing each with ~> ?

That makes the paragraph turn into this yellow rectangle:

mrooding · 2017-03-30T08:12:05Z

Updated once again! 👍

radeksimko · 2017-03-30T08:24:34Z

Great 👍 🎉
Pulling in, thanks for all the work!

ghost · 2020-04-14T02:28:14Z

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.

radeksimko added provider/kubernetes new-resource labels Mar 22, 2017

ctavan reviewed Mar 24, 2017

View reviewed changes

radeksimko requested changes Mar 28, 2017

View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Mar 28, 2017

radeksimko mentioned this pull request Mar 28, 2017

docs/state: Sensitive data in state file #13132

Merged

ctavan reviewed Mar 30, 2017

View reviewed changes

radeksimko approved these changes Mar 30, 2017

View reviewed changes

kubernetes: Add secret resource

8e9538c

radeksimko merged commit c2b657a into hashicorp:master Mar 30, 2017

mrooding deleted the add-kubernetes-secret-resource branch March 30, 2017 08:27

ghost locked and limited conversation to collaborators Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/kubernetes: Add secret resource #12960

provider/kubernetes: Add secret resource #12960

mrooding commented Mar 22, 2017

ctavan commented Mar 24, 2017

ctavan Mar 24, 2017

ctavan commented Mar 24, 2017

ctavan commented Mar 24, 2017

mrooding commented Mar 24, 2017 •

edited

Loading

ctavan commented Mar 27, 2017 •

edited

Loading

radeksimko commented Mar 28, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

radeksimko Mar 28, 2017

mrooding commented Mar 29, 2017

ctavan Mar 30, 2017

mrooding Mar 30, 2017

radeksimko left a comment

radeksimko Mar 30, 2017

radeksimko Mar 30, 2017

radeksimko Mar 30, 2017

mrooding commented Mar 30, 2017

radeksimko commented Mar 30, 2017

ghost commented Apr 14, 2020

	ops := patchMetadata("metadata.0.", "/metadata/", d)
	if d.HasChange("data") {
	oldV, newV := d.GetChange("data")
	diffOps := diffStringMap("/data/", oldV.(map[string]interface{}), newV.(map[string]interface{}))
	ops = append(ops, diffOps...)
	}
	data, err := ops.MarshalJSON()
	if err != nil {
	return fmt.Errorf("Failed to marshal update operations: %s", err)
	}
	log.Printf("[INFO] Updating config map %q: %v", name, string(data))
	out, err := conn.CoreV1().ConfigMaps(namespace).Patch(name, pkgApi.JSONPatchType, data)
	if err != nil {
	return fmt.Errorf("Failed to update Config Map: %s", err)
	}


		The resource provides mechanisms to inject containers with sensitive information, such as passwords, while keeping containers agnostic of Kubernetes.
		Secrets can be used to store sensitive information either as individual properties or coarse-grained entries like entire files or JSON blobs.


		namespace, name := idParts(d.Id())

		log.Printf("[INFO] Deleting secret: %#v", name)

provider/kubernetes: Add secret resource #12960

provider/kubernetes: Add secret resource #12960

Conversation

mrooding commented Mar 22, 2017

ctavan commented Mar 24, 2017

Choose a reason for hiding this comment

ctavan commented Mar 24, 2017

ctavan commented Mar 24, 2017

mrooding commented Mar 24, 2017 • edited Loading

ctavan commented Mar 27, 2017 • edited Loading

radeksimko commented Mar 28, 2017 • edited Loading

radeksimko 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

mrooding commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko 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

mrooding commented Mar 30, 2017

radeksimko commented Mar 30, 2017

ghost commented Apr 14, 2020

mrooding commented Mar 24, 2017 •

edited

Loading

ctavan commented Mar 27, 2017 •

edited

Loading

radeksimko commented Mar 28, 2017 •

edited

Loading