Fixing issues with workspace_key_prefix for s3 backend #16932

ghost · 2017-12-15T22:55:05Z

I believe this will fix #16383 and #16387

ghost · 2017-12-16T02:21:01Z

@jbardin @apparentlymart
Ran into this bug today and I think I've got a fix here. I added some tests for this functionality and cleaned up a bit. However, I haven't manually tested this. I'm hoping this could be released in a minor version.

jbardin

Thanks for submitting the PR @rv-jmaggio. The implementation shown here will fail the full backend test set, so there's a few issues that need to be sorted out.

jbardin · 2017-12-18T18:25:11Z

backend/remote-state/s3/backend_state.go

@@ -51,6 +58,12 @@ func (b *Backend) keyEnv(key string) string {
 		return ""
 	}

+	parts = strings.Split(parts[1], "/")


If the key names are allowed to have / characters, you're going to want SplitN with 3 here.

jbardin · 2017-12-18T18:28:06Z

backend/remote-state/s3/backend_test.go

+		t.Fatal(err)
+	}
+
+	if err := testGetWorkspaceForKey(b1, "root/userA/ws1/tfstate", "ws1"); err != nil {


If the prefix is root/userA and the key is ws1/tfstate, ws1 can't be the workspace name.

jbardin · 2017-12-18T18:31:41Z

backend/remote-state/s3/backend_test.go

+		t.Fatal(err)
+	}
+
+	if err := testGetWorkspaceForKey(b1, "root/userA/ws2/tfstate", "ws2"); err != nil {


Same as above for this check and the one below. The tests are checking the output of getWorkspaceForKey, but that doesn't match how it's used by the backend.

jbardin · 2017-12-18T18:33:18Z

backend/remote-state/s3/backend_test.go

+
+	if err := testGetWorkspaceForKey(b2, "root/userA/ws2/env1/tfstate", "ws2"); err != nil {
+		t.Fatal(err)
+	}


Once of the new backend configs needs to be run through the TestBackend helper, which would have failed with either b1 or b2.

ghost · 2017-12-18T21:33:38Z

@jbardin I updated from your feedback. I did some testing in a playground and you are definitely correct on SplitN. Evidently I was a bit confused when I wrote my test cases, so I made them a bit more clear. I think this should be good to go now. I am happy to contribute to a tool I use nearly every day 😃

jbardin

Don't forget these are all run as acceptance tests, so you need to set TF_ACC=1!

jbardin · 2017-12-18T21:58:13Z

backend/remote-state/s3/backend_state.go

+func getWorkspaceForKey(key string, b *Backend) string {
+	if b.workspaceKeyPrefix == "" {
+		parts := strings.Split(key, "/")
+		if len(parts) > 1 && parts[1] == key {


I'm not sure I understand what the intent here was, but you can't split key, then still have parts[1] == key. It's also going to fail for the same reason as before, since we need to be able to support / in the key for compatibility.

Ah yes the intention was to check b.keyName, not key. Thanks

jbardin · 2017-12-18T21:59:37Z

backend/remote-state/s3/backend_test.go

+}
+
+func testGetWorkspaceForKey(b *Backend, key string, expected string) error {
+	if getWorkspaceForKey(key, b) != expected {


This should record the computed value, so the error can show both the expected value and what you received.

jbardin · 2017-12-18T22:12:47Z

backend/remote-state/s3/backend_test.go

@@ -249,6 +249,63 @@ func TestBackendExtraPaths(t *testing.T) {
 	}
 }

+func TestKeyEnv(t *testing.T) {


Though the rest of the ACC tests pass again, this test still fails.

jbardin · 2017-12-18T22:23:59Z

backend/remote-state/s3/backend_test.go

+	if err := testGetWorkspaceForKey(b2, "env:/ws3/tfstate", "ws3"); err != nil {
+		t.Fatal(err)
+	}
+


You're not creating this bucket for the backend tests, though it currently fails earlier.

jbardin · 2017-12-18T22:28:55Z

backend/remote-state/s3/backend_test.go

+func TestKeyEnv(t *testing.T) {
+	testACC(t)
+	bucketName := fmt.Sprintf("terraform-remote-s3-test-%x", time.Now().Unix())
+	keyName := "tfstate"


These test cases are good, but let's go back to having a keyName with a / to make sure it's still supported after we introduce them into the prefix.

ghost · 2017-12-19T14:35:37Z

@jbardin
Ah, thanks for the pointer on TF_ACC=1.

So I made those changes, but I was getting an error when running backend.TestBackend(t, b0, nil)
--- FAIL: TestKeyEnv (23.36s) backend_test.go:339: creating S3 bucket terraform-remote-s3-test-5a39211b-0 in us-west-2 backend_test.go:339: creating S3 bucket terraform-remote-s3-test-5a39211e-1 in us-west-2 backend_test.go:339: creating S3 bucket terraform-remote-s3-test-5a392121-2 in us-west-2 backend_test.go:308: bad: []string{"default"} FAIL

It seems like when workspace_key_prefix is set to "" then the workspace folder default isn't being created? Not exactly sure what to make of this.

ghost · 2017-12-19T14:39:38Z

Ah actually it seems to be related to the implementation for empty string workspace_key_prefix. I think I can get that fixed.

edit: Maybe not, after making some adjustments it is still failing with the same error

ghost · 2017-12-19T15:05:50Z

I think there may be an issue with this implementation for a workspace_key_prefix set to ""

https://github.com/hashicorp/terraform/blob/master/backend/remote-state/s3/backend_state.go#L180

ghost · 2017-12-20T18:45:10Z

@jbardin I had to make 2 more changes to the backend_state implementation to allow for empty string prefix. I ran the acceptance tests (actually in acceptance test mode) and they all passed.

jbardin · 2017-12-21T23:24:17Z

Hi @rv-jmaggio,

Thanks for the update!
This is looking good. I'll do a final review after we return from the break and aim to get this into the next release.

jbardin

LGTM 👍 Thanks for putting in the extra work to get this in!

rv-jmaggio added 2 commits December 15, 2017 17:50

Fixing issues with workspace_key_prefix

0889c11

refactor and add a test

7f8d686

apparentlymart requested a review from jbardin December 18, 2017 15:07

apparentlymart added backend/s3 bug labels Dec 18, 2017

jbardin requested changes Dec 18, 2017

View reviewed changes

clarifying tests and using SplitN in implementation

b02a1c8

jbardin requested changes Dec 18, 2017

View reviewed changes

Fixing implementation for empty string and making acceptance test work

bef64cf

Changing prefix for empty workspace prefix

b313ce8

jbardin approved these changes Jan 5, 2018

View reviewed changes

jbardin merged commit 7c703f2 into hashicorp:master Jan 5, 2018

jbardin mentioned this pull request Jan 5, 2018

Empty string in workspace_key_prefix breaks workspace functionality #16387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing issues with workspace_key_prefix for s3 backend #16932

Fixing issues with workspace_key_prefix for s3 backend #16932

ghost commented Dec 15, 2017

ghost commented Dec 16, 2017 •

edited by ghost

Loading

jbardin left a comment

jbardin Dec 18, 2017

jbardin Dec 18, 2017

jbardin Dec 18, 2017

jbardin Dec 18, 2017

ghost commented Dec 18, 2017

jbardin left a comment

jbardin Dec 18, 2017

ghost Dec 19, 2017

jbardin Dec 18, 2017

jbardin Dec 18, 2017

jbardin Dec 18, 2017

jbardin Dec 18, 2017

ghost commented Dec 19, 2017 •

edited by ghost

Loading

ghost commented Dec 19, 2017 •

edited by ghost

Loading

ghost commented Dec 19, 2017

ghost commented Dec 20, 2017

jbardin commented Dec 21, 2017

jbardin left a comment

Fixing issues with workspace_key_prefix for s3 backend #16932

Fixing issues with workspace_key_prefix for s3 backend #16932

Conversation

ghost commented Dec 15, 2017

ghost commented Dec 16, 2017 • edited by ghost Loading

jbardin 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

ghost commented Dec 18, 2017

jbardin 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

ghost commented Dec 19, 2017 • edited by ghost Loading

ghost commented Dec 19, 2017 • edited by ghost Loading

ghost commented Dec 19, 2017

ghost commented Dec 20, 2017

jbardin commented Dec 21, 2017

jbardin left a comment

Choose a reason for hiding this comment

ghost commented Dec 16, 2017 •

edited by ghost

Loading

ghost commented Dec 19, 2017 •

edited by ghost

Loading

ghost commented Dec 19, 2017 •

edited by ghost

Loading