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

backend/kubernetes: Add Kubernetes as a backend #19525

Merged
merged 4 commits into from
Jun 8, 2020

Conversation

dramich
Copy link
Contributor

@dramich dramich commented Dec 1, 2018

This pr will add Kubernetes as a backend

Details:
The state will be stored gzipped in a kubernetes secret. The secret name is terrastate-<workspace>-<key>. If issues arise with locking or state the secret name is in the errors.
Locking is handled in the same secret used to hold this state. This is done so we can use references to ensure secrets aren't getting changed as we do state changes.
This pulls in the k8s.io/client-go along with all its friends.
Added docs for the website

Example of the secret

apiVersion: v1
data:
  lockInfo: <If lock is held, would be here>
  terrastate: <base64 encoded state>
kind: Secret
metadata:
  creationTimestamp: 2018-11-30T23:38:53Z
  labels:
    terraKey: example-key
    terraWorkspace: default
    terrastate: "true"
  name: terrastate-<workspace>-<key>
  namespace: default
type: Opaque

Open Questions:
How to run the tests? Is there a k8s infra already available in the builds?

Issue: #19371

Test output:

TF_K8S_TEST=true go test -v
=== RUN   TestBackend_impl
--- PASS: TestBackend_impl (0.00s)
=== RUN   TestBackend
--- PASS: TestBackend (7.86s)
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:44: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
=== RUN   TestBackendLocks
--- PASS: TestBackendLocks (2.25s)
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:57: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:61: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:66: TestBackend: testing state locking for *kubernetes.Backend
    backend_test.go:67: TestBackend: testing state locking for *kubernetes.Backend
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
=== RUN   TestRemoteClient_impl
--- PASS: TestRemoteClient_impl (0.00s)
=== RUN   TestRemoteClient
--- PASS: TestRemoteClient (0.62s)
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    client_test.go:20: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
=== RUN   TestRemoteClientLocks
--- PASS: TestRemoteClientLocks (0.45s)
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    client_test.go:36: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    client_test.go:40: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
=== RUN   TestForceUnlock
--- PASS: TestForceUnlock (2.06s)
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    client_test.go:61: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    client_test.go:65: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
    backend_test.go:72: TestBackendConfig on *kubernetes.Backend with configs.synthBody{Filename:"<TestWrapConfig>", Values:map[string]cty.Value{"key":cty.StringVal("test-state")}}
PASS
ok  	github.com/hashicorp/terraform/backend/remote-state/kubernetes	13.269s

@dramich dramich force-pushed the k8sback branch 2 times, most recently from c7b959b to f6b4be6 Compare December 4, 2018 16:49
@dramich
Copy link
Contributor Author

dramich commented Dec 18, 2018

@apparentlymart Can you (or someone) take a look at the build failure? I can't repo the failure locally and I am unsure how it relates to my changes. Thanks.

@mvladev
Copy link

mvladev commented Feb 28, 2019

@dramich

How to run the tests? Is there a k8s infra already available in the builds?

You don't need lots of infra for this kind of test. Simply running etcd and kube-apiserver should be sufficient.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 15, 2019

CLA assistant check
All committers have signed the CLA.

@chiefy
Copy link

chiefy commented Mar 28, 2019

Any chance of getting this in 0.12?

@apparentlymart
Copy link
Contributor

The v0.12.0 release cycle is already active and so there will only be bug fixes merged until it is released.

We will review other proposals once v0.12 is released and stable. Sorry for the delay!

@Bobonium
Copy link

Bobonium commented Jun 3, 2019

First of all I love it and it's exactly what I need now that the kubernetes-provider has proper support for ingress and affinity.
Some feedback in regard to the way it's currently implemented

  • the name of the secret is created from the key provided in the backend config and is simply a hash of said key
    why is the name hashed at all? the same key will always result in the same hash but it's unnecessary hard to manually have a look at the stored states like this

  • the secret is labelled with terraKey, terraWorkspace and terrastate
    terrastate seems to be hardcoded to true which makes it redundant as the secret already starts with the name terrastate
    terraWorkspace and terraKey are both also redundant as you know these values from the namespace and suffix from the secret name
    personally what would've helped me as a label would've been the information how the data is encoded, i.e. encoding: gzip so that the user instantly knows how to decode the secret to see the state file

  • what happens when the size limit of 1.5MB is reached?
    I didn't try it and it should be quite hard to reach, especially given the fact that the data is gzipped, but what would happen if the size limit of 1.5MB is reached?
    My expectation would be that the file will not be correctly written in this case even though state changes happened.
    Would it make sense to automatically index and split the secret as necessary in case the state gets too big? It could look like terrastate-KEY-INDEX aka terrastate-test-1 followed by terrastate-test-2 and so on. To make the lookup easier we could label the secrets with the total number of files

  • load_config_file why is the default false when the kubernetes provider has true as default
    As a user I'd expect that the kubernetes backend configuration is exactly like the kubernetes provider configuration, with the difference beeing that a namespace and key are required

Nonetheless I have to say these changes make terraform finally a viable option for bare metal clusters (especially now that version 1.7 of the kubernetes provider has been released as well), and I hope that a release with this feature included will happen in the near future now that 0.12.0 has been released.

Also as it makes testing for others easier here's a simple main.tf that makes use of the backend (~/.kube/config has to exist and the context has to be correctly configured beforehand)

terraform {
  backend "kubernetes" {
    load_config_file = true
    key = "asdf"
  }
}

provider "kubernetes" {
  version     = ">=1.7"
}

resource "kubernetes_namespace" "test" {
  metadata {
    name = "this-is-a-test"
  }
}

here's the secret terrastate-wvp4otqr26 that is created from the key name asdf

apiVersion: v1
data:
  lockInfo: ""
  terrastate: H4sIAAAAAAAA/4SSUY6jMAyG3zlFlOemJW0oTK+yGiFDTCcaSFBiKq2q3n2VdBnoTrfzgmL/X+zfDteMMX5BH4yz/MTUJsaE3kPn/FAvCs+3cr/NeQICegM9P7Eihb2xCGeMlCoLBaqqBB50I8ojKFFo1YpC77WGtyqXUt1ruInGiQI/sestJTwGN/kWY+pXxhhj1/RljA9Op+oDWDijTgWSQL/HJHxODXqLhKG2MGAYocWFiqlIEQZasqN3F6PRR2U+b5dCC2hsILBrY2tzCQntBw6wWle+WctA5E0zUSqxvhir62TtwwRhggDxYHKeHwk0EDwY+G5j7matIyDjbGxnp77ffIfOaNEDYT0vh/8fejLSX6CHBvsXXb42/2q8RM6vv/7lDvKoimdwwL6re2M/I7WD0ewucvf18GH3c7vpvvayylsAuRfV8dAIKfFNNKorRV7l+b6U+qi7jv9z+/YQv2fPlPl0V2P0nt2yPwEAAP//Vzdyu20DAAA=
kind: Secret
metadata:
  creationTimestamp: "2019-06-03T19:18:10Z"
  labels:
    terraKey: asdf
    terraWorkspace: default
    terrastate: "true"
  name: terrastate-wvp4otqr26
  namespace: default
  resourceVersion: "31651"
  selfLink: /api/v1/namespaces/default/secrets/terrastate-wvp4otqr26
  uid: 5381969c-8634-11e9-b4f7-0800271d6dff
type: Opaque

and just to be complete here's the base64 -d | gunzip of the secret

{
  "version": 4,
  "terraform_version": "0.12.0",
  "serial": 5,
  "lineage": "4754a488-e3db-76a4-5d4c-5d2dda980114",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "kubernetes_namespace",
      "name": "test",
      "provider": "provider.kubernetes",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "id": "this-is-a-test",
            "metadata": [
              {
                "annotations": null,
                "generate_name": "",
                "generation": 0,
                "labels": null,
                "name": "this-is-a-test",
                "resource_version": "31645",
                "self_link": "/api/v1/namespaces/this-is-a-test",
                "uid": "780caa12-863b-11e9-b4f7-0800271d6dff"
              }
            ]
          }
        }
      ]
    }
  ]
}

@dramich dramich force-pushed the k8sback branch 3 times, most recently from a0e49aa to 5f1e983 Compare June 12, 2019 23:43
@dramich
Copy link
Contributor Author

dramich commented Jun 12, 2019

@Bobonium Thanks for the feedback. To address your points:

  1. The hash was the workspace and the key. This was done to stop the issue of having values that are not valid for k8s secret names. But to your point it makes it harder to find your state if manual editing is needed. I have updated the naming to be terrastate-<workspace>-<key> and running this through k8s helpers to validate the input before attempting to create the secret.

  2. The labels are there for ease of use. In the code they are used in secret list calls so we don't get back ALL secrets in a namespace. They are also useful for an end user as they can also list secrets that just belong to a workspace or a key or some combination. I have added an annotation stating that the secret is gzipped as that really isn't what a label is for.

  3. If someone hits the 1mb limit of a k8s secret the backend will do the same thing consul does, error out.

  4. I agree, this has been updated so that load_config_file is the default.

@Bobonium
Copy link

@dramich thanks for considering my two cents on the matter, it's absolutely awesome.

@apparentlymart now that terraform 0.12.X has been released, can we expect this to be reviewed/merged in the near future or is there any to help to get this merged soon? I'd like to go back to the official release but I (and probably everyone else who's using the kubernetes provider and found this PR) just can't go back without this feature

@jbardin
Copy link
Member

jbardin commented Jun 18, 2019

Hi @dramich,

Thanks for the PR! This looks good on a first pass. For backends we are still manually running the tests which require external resources. You can add the verbose test output for this package into the initial comment of the PR (you can strip the logs and just have the test output itself if you want).

@dramich
Copy link
Contributor Author

dramich commented Jun 18, 2019

Hey @jbardin, I have appended the initial comment with the test output, let me know if you need anything else.

@dramich
Copy link
Contributor Author

dramich commented Jul 8, 2019

@jbardin Hello, any updates on this?

@jbardin
Copy link
Member

jbardin commented Jul 8, 2019

Hi @dramich,

We're currently evaluating how we want to handle backend contributions, and the ensuing support of the backend code. While we would like to be able to separate them from the core code, there is a lot of work to do before that can happen.

I think the biggest concern with this particular backend is the >300k lines of dependency code it brings along; which is a significant surface area, and also impacts all terraform releases with a 30% increase in binary size.

Do you know if this could be done with a simpler client implementation of any sort, which doesn't rely on the bulk of the k8s.io packages?

@dramich
Copy link
Contributor Author

dramich commented Jul 9, 2019

Hey @jbardin,

I understand your concerns. I have updated the PR to use a different client which has significantly cut down the PR size while still keeping some of the niceties of dealing with the kubeconfig and some help from clients. The tests are still passing as well.

Let me know what you think. (I will squash commits after you take a look)

@jbardin
Copy link
Member

jbardin commented Jul 18, 2019

Thanks for the extra effort here @dramich!

@alexsomesan, could I get another 👀 from someone more familiar with k8s? Thanks!

@alexsomesan
Copy link
Member

Sure, I'm having a look.

@jbardin jbardin requested a review from alexsomesan July 18, 2019 18:58
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Overall the K8s client code looks good and clean.

I've dropped a couple of comments, but most importantly I'd like to understand the handling of the result of Create calls (which aren't atomic).

if !k8serrors.IsAlreadyExists(err) {
return "", err
}
// The secret was created between the get and create, grab it and keep going
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this.
It's a known fact that the Create operation in K8s' client-go is not atomic. And so it makes sense to check whether "our" Create call actually produced the object we're later manipulating.
So what confuses me is why is the next line just reading the already existing object (supposedly produced outside of this client) and moving on using that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we attempt to create the secret and it already exists, it just needs to be fetched and then fall back to the same flow as if we found the secret the first time we tried to get it.

}

// Overriding with static configuration
cfg.UserAgent = fmt.Sprintf("HashiCorp/1.0 Terraform/%s", terraform.VersionString())
Copy link
Member

Choose a reason for hiding this comment

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

There's a "TODO" comment around terraform.VersionString() calling for future use-cases to just directly use the Version string in the github.com/hashicorp/terraform/version package (unless I'm misunderstanding it).

Maybe it's worth doing that. @jbardin might know more about the lifespans of these APIs.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Alex. That's probably copied from other backends, which haven't been updated. Those strings have been moved around as we've refactored, but we haven't gotten to the point of actually removing the old bits. It can simply be switched out for version.String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use version.String

@pselle
Copy link
Contributor

pselle commented Apr 22, 2020

@jrhouston We've recently updated the Contributing guide to clarify that coverage won't necessarily block a PR: https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md#pr-checks

So no, that is not blocking here.

Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

LGTM.
I think we should test the locking process a bit closer.
I have an uneasy feeling it might not behave quite atomically.

DefaultFunc: schema.EnvDefaultFunc("KUBE_NAMESPACE", "default"),
Description: "Namespace to store the secret in.",
},
"in_cluster_config": {
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we should align this change in the providers too, but being a breaking change we have to plan it for the next major release.

@jbardin jbardin self-assigned this May 15, 2020
return "", err
}

if li != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@alexsomesan

This unfortunately does not provide a safe lock. The big clue is that the lock is handled in multiple operations, but "check then create" (or the reverse) cannot work with multiple concurrent clients. Some form of a single test-and-set operation is needed to create a lock (we don't require blocking locks for mutual exclusion, so more advanced primitives are not required)

Multiple clients can reach this point with the expectation that their lock info is going to be saved. The following Update call will then update the secret for both with the last client's info being saved, but both clients assuming they have a lock.

@alexsomesan, the comments above lead me to believe that kubernetes secrets may not have the ability to use this storage as a lock, but I'm not familiar with the data model of the secrets storage to know for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbardin That is not how the k8s API works. If you hold an object, in this case a secret, which has a version ID, and then attempt to make an update to that secret, the k8s API will validate you have the most recent version of the secret and if you do not, reject your update request. The lock is being enforced by the API by assuring the client is only able to make updates to the most recent object.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @dramich, I'll look into that. Testing this has consistently resulted in multiple clients obtaining locks concurrently, so I'll have to investigate further.

Copy link
Contributor

@jrhouston jrhouston May 27, 2020

Choose a reason for hiding this comment

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

Just to briefly update on this; I realized the client-go exposes the same locking mechanism used by kubernetes' controller-manager to do leader election, so I'm reworking this PR to use that.

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

Sorry, but this cannot be merged in the current state, because the lock implementation is not safe. We will have to review if this is possible to implement with kubernetes secrets.

@jrhouston
Copy link
Contributor

jrhouston commented Jun 5, 2020

OK - so after doing a bunch of sleuthing in the k8s code I have reworked the backend to use the Lease resource from the coordination/v1 API as the lock mechanism.

Initially I looked to see if it was feasible to take the leaderelection or resourcelock packages (this is what kube-controller-manager uses internally) from client-go and just use those, but they seem to assume that you are going to use them in a long running service, so I am using the coordination API directly here.

What this now does is creates a Lease resource and uses the holderIdentity field as the lock before writing any state to a Secret. As @dramich alluded to in his comment, we are able to use this as a lock because the ResourceVersion (which, in turn is the resource's version in etcd) field allows the API client's Update call to implement compare-and-swap. If multiple clients try to update the Lease with themselves as the holder only one will succeed and other will recieve an error. This behaviour is not super obvious or well documented so I have included some references below.

For my own piece of mind, I created a soak test function alongside the usual locking test. This creates 1000 concurrent backend clients and fails if more than one succeeds in getting a lock. It is worth noting that if the API server of the cluster is not using a strongly consistent store like etcd, i.e k3s which allows you to use sqlite and postgres IIRC, then this locking mechanism may not be reliable.

running TF_K8S_TEST=true go test -count=1 ./... in the package directory and then kubectl get lease --watch in another terminal will show the lock mechanism being used.

References

https://kubernetes.io/blog/2016/01/simple-leader-election-with-kubernetes/
https://medium.com/michaelbi-22303/deep-dive-into-kubernetes-simple-leader-election-3712a8be3a99
Managing Kubernetes
kubernetes/kubernetes#2115 (comment)

backend/remote-state/kubernetes/backend_test.go Outdated Show resolved Hide resolved
backend/remote-state/kubernetes/backend_test.go Outdated Show resolved Hide resolved
backend/remote-state/kubernetes/backend_test.go Outdated Show resolved Hide resolved
backend/remote-state/kubernetes/backend_test.go Outdated Show resolved Hide resolved
backend/remote-state/kubernetes/client.go Outdated Show resolved Hide resolved
backend/remote-state/kubernetes/client.go Outdated Show resolved Hide resolved
@jrhouston
Copy link
Contributor

I have updated the Unlock function to catch those error cases, and changed the soak test to create 100 clients that each make 100 attempts on the lock and error if Unlock() produces a failure.

@jrhouston jrhouston requested a review from jbardin June 5, 2020 22:29
@sftim
Copy link

sftim commented Jun 6, 2020

I have reworked the backend to use the Lease resource from the coordination/v1 API as the lock mechanism

would you be interested / willing in writing a blog article for https://k8s.io/blog/abut that @jrhouston ? Or if not writing it yourself, you could sketch out how you'd like the article to look and I could help author something (I'm part of Kubernetes SIG Docs). What do you think?

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

Thanks @jrhouston!

@jrhouston
Copy link
Contributor

@sftim Sure! I'll drop you a message on the kubernetes slack and we can chat.

@jbardin jbardin merged commit 7800ef6 into hashicorp:master Jun 8, 2020
@jbardin
Copy link
Member

jbardin commented Jun 9, 2020

Thanks @dramich and all, sorry this took so long to include. This is now merged, and we will be shipping this with the next 0.13 beta release 🚀

@ghost
Copy link

ghost commented Jul 9, 2020

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.

@ghost ghost locked and limited conversation to collaborators Jul 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.