tokenRead will renew resource token instead of access token

[peric]

Resolves #422. Check issue for more info.

[tyrannosaurus-becks]

Hi @peric , thanks for submitting this PR.

I think your expectation about how renewal should work is reasonable. It does make more sense to renew the client token than to renew the accessor.

Looks like a test needs to be updated. This one does pass currently on master.

GOROOT=/usr/local/go #gosetup
GOPATH=/home/tbex/go #gosetup
/usr/local/go/bin/go test -c -o /tmp/___resource_token_test_go github.com/terraform-providers/terraform-provider-vault/vault #gosetup
/usr/local/go/bin/go tool test2json -t /tmp/___resource_token_test_go -test.v -test.run "^TestResourceToken_basic|TestResourceToken_import|TestResourceToken_full|TestResourceToken_lookup|TestResourceToken_expire|TestResourceToken_renew$" #gosetup
=== RUN   TestResourceToken_basic
--- PASS: TestResourceToken_basic (0.06s)
=== RUN   TestResourceToken_import
--- PASS: TestResourceToken_import (0.05s)
=== RUN   TestResourceToken_full
--- PASS: TestResourceToken_full (0.05s)
=== RUN   TestResourceToken_lookup
--- PASS: TestResourceToken_lookup (0.04s)
=== RUN   TestResourceToken_expire
--- PASS: TestResourceToken_expire (11.18s)
=== RUN   TestResourceToken_renew
--- FAIL: TestResourceToken_renew (21.18s)
    testing.go:538: Step 2 error: Expected a non-empty plan, but got an empty plan!
FAIL

Process finished with exit code 1

[tyrannosaurus-becks]

I just added code to master that will automate running this test whenever you push up a new commit. If you pull master, merge it, and push it up, you'll be able to see the test failure in CircleCI. Might make development a little easier.

[peric]

Hi @tyrannosaurus-becks, thanks a lot for setting up this.

I'm struggling to find out how to fix this test and I'm not getting anywhere. I understand that empty plan is received even though an non-empty one is expected, but I don't get why does it come to this. I have an idea to put couple of outputs around the code, but then I need to push every time, wait for the tests (cannot run them locally) and I will ruin the commit history.

Do you have any idea how to fix those tests? If not, I can maybe create another WIP PR just to add couple of outputs in my code and to run the tests.

Thanks in advance.

[tyrannosaurus-becks]

Hi @peric ! I'm not at all concerned about the commit history. If you need to push a lot to try things, feel free. If you feel comfortable when you're done, you can still PR it as-is, or, sometime I clean up my own commits by doing:

git pull origin master // get a fresh copy of master
git checkout my-branch // get onto my branch
git merge master // merge my branch to master
git reset origin/master // this takes all the different files and makes them un-added and un-committed locally
git add . // re-add them
git commit -m 'pretty commit message' // rewrite history locally
git push -u origin my-branch --force // rewrite history remotely

This is a bit risky so might be good to try once or twice on a test branch checked out off your real one.

[tyrannosaurus-becks]

Also! Regarding your request for help with the tests. I'm afraid I don't know offhand why the tests are failing. What I can do is link you to these docs and hope they might help you wade through it.

[peric]

@bhuisgen any way we can get your help on this? I'm pinging you because you added the resource_token code and tests.

But I'll anyway try to figure out those tests in the meantime, I have finally dedicated some time to this now.

[peric]

@tyrannosaurus-becks According to what I have read in Expecting errors or non-empty plans, ExpectNonEmptyPlan was actually undesirable and it was there because of the misconfiguration.

My fix was just to remove it, and in my opinion, this PR is now ready to be merged.

[tyrannosaurus-becks]

@peric thanks for working on this more!

I looked at the ExpectNonEmptyPlan attribute, and the docs you linked, and I have a couple of questions:

• Why does this code impact that test?
• What was the resulting state file before?
• What is the resulting state file now?
• How are they different?

Just trying to fully understand the behavior changes from this PR so I am aware of how this will impact other folks using the provider. Thanks again!

[peric]

@tyrannosaurus-becks here is how I see this. I will try to give you all the answers below🤞

Previous solution for renewing token looked like this:

renewed, err := client.Auth().Token().RenewSelf(increment)
if err != nil {
    log.Printf("[DEBUG] Error renewing token, removing from state")
    d.SetId("")
    return nil
}

What happened here is that a call to RenewSelf always errored and resulted in a:

Error making API request.

URL: PUT http://localhost:8200/v1/auth/token/renew-self
Code: 400. Errors:

* lease is not renewable

I believe this is because we were trying to renew a wrong token - not the one we are interested in, but the one that is currently authenticated.

Because of that, ResourceData ID was always set to empty (d.SetId("")) and this indicates to Terraform the item no longer exists, which always resulted in a state change even though the token was actually still there and valid. I believe this is also the reason why ExpectNonEmptyPlan was there, even though this seems to be some kind of workaround.

With the new solution, we are renewing the actual token, where the only changes should be lease_duration etc:

renewed, err := client.Auth().Token().Renew(id, increment)
if err != nil {
    log.Printf("[DEBUG] Error renewing token, removing from state")
    d.SetId("")
    return nil
}

Hope it is more clear now. I am definitely sure the previous solution didn't work as it should and it resulted in some non-expected changes, but it would be also nice if someone can confirm this.

[tyrannosaurus-becks]

@peric thank you for that explanation! It really helps. I'm going to go ahead and approve this PR and merge it. The next release for this provider will likely be about a month out, which will give me and others the opportunity to play with it a little more before it's out. I greatly appreciate your time and attention on this.

[peric]

@tyrannosaurus-becks thanks for taking care of this. I will use the compiled version of master until this is officially released.