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

Add manta remote state backend #8830

Closed
wants to merge 6 commits into from

Conversation

watters
Copy link
Contributor

@watters watters commented Sep 13, 2016

I tried to match what the other backends offered in terms of documentation and testing, but any/all feedback is welcome.

I've marked this [WIP] for now while I continue to exercise it, but I wanted to invite any feedback early on.

@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

Hi @watters! I strongly approve of this PR, obviously! I'll look at getting it tested and merged into Terraform shortly.

@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

Hi @watters - I've made a quick review of this, but can't figure out via the various layers of indirection which environment variables need to be set to make the tests run here? Can you shed some light on this (and maybe fix up the "MANTA_USER and friends" comment to reflect that 😆)? Thanks!

@watters
Copy link
Contributor Author

watters commented Sep 14, 2016

@jen20 thanks so much for taking a look at this so soon!

I've updated the skip message in the tests to be explicit about all three Manta environment variables that must be set.

I also added a link to the Manta documentation for setting up the environment in the website documentation.

When running make test TEST=./state/remote TESTARGS=-v with the MANTA_USER, MANTA_KEY_ID and MANTA_URL environment variables set, the tests pass and I can see the calls to Manta being made when I inspect the traffic.

Let me know if there are any other changes you'd like to see.

@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

Hi @watters - those changes look good to me. Running the tests I'm still seeing "Error getting Manta credentials: An error occurred while parsing the key: asn1: structure error: length too large" however - the key is in the agent and available unencrypted on disk, though it's unclear where it would be loaded from? FWIW, the manta command line tools work correctly with the same environment variables set.

@watters
Copy link
Contributor Author

watters commented Sep 14, 2016

I'll see if I can repro.

That error is coming from the underlying auth bits in github.com/joyent/gosign, which I didn't mess with—yet (as discussed elsewhere, the underlying libraries in play can be unwieldy).

keyName, which defaults to $HOME/.ssh/id_rsa currently cannot be overridden for the tests, which is probably something that needs solving.

…kend

- expose MANTA_KEY_MATERIAL environment variable (previously specified via
  keyName parameter) and make it required; no defaults

- stop relying on some of the Joyent go libraries' underlying environment
  variable config magic (it doesn't appear to play nice if you have
  different values in MANTA_USER and SDC_ACCOUNT)

- explicitly check for passphrase-protected SSH keys, which are currently
  unsupported, and generate a more helpful error (borrowed from Packer's
  solution to the same problem):
  https://github.com/mitchellh/packer/blob/master/common/ssh/key.go#L27

- update documentation to reflect these changes
return nil, fmt.Errorf("Error reading key material from %s: %s",
keyMaterial, err)
} else {
block, _ := pem.Decode(keyBytes)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, this is working now.

@watters
Copy link
Contributor Author

watters commented Sep 14, 2016

@jen20 I was able to reproduce the error message you described when using a passphrase-protected 4096-bit SSH key. The underlying auth code chokes on passphrase-protected keys and currently only supports keys specified by file path or decrypted into an environment variable.

I've exposed the key path via environment the variable MANTA_KEY_MATERIAL, which will allow you to specify a key that can be used during the tests (previously it defaulted to ~/.ssh/id_rsa), but it must align with the MANTA_KEY_ID and, of course, be associated with your account.

Hopefully, this gets the tests passing for you. I'd already determined before submitting this PR that Joyent's Go library for Manta probably needed a re-write, and now I'm only even more convinced.

@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

That explains a lot - I have a separate unencrypted key for testing with HashiCorp's Joyent account rather than my personal (encrypted) key, so ~/.ssh/id_rsa producing that error message make sense. I'll test it with MANTA_KEY_MATERIAL set now. The Go library for Manta is a bit janky right now - I suspect it was written before the idioms for Go had become apparent. I'll look at merging this now though and we can sort that out in future.

Copy link
Contributor

@jen20 jen20 left a comment

Choose a reason for hiding this comment

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

LGTM!

return nil, fmt.Errorf("Error reading key material from %s: %s",
keyMaterial, err)
} else {
block, _ := pem.Decode(keyBytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, this is working now.

"Failed to read key material '%s': no key found", keyMaterial)
}

if block.Headers["Proc-Type"] == "4,ENCRYPTED" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great - we should roll it out into the Triton provider as well.

@jen20 jen20 changed the title [WIP] Add manta remote state backend Add manta remote state backend Sep 14, 2016
@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

Example test run:

=== RUN   TestMantaClient_impl
--- PASS: TestMantaClient_impl (0.00s)
=== RUN   TestMantaClient
2016/09/14 20:37:13 [DEBUG] New state was assigned lineage "aace983a-9729-4997-9ca0-121b4e2f698c"
2016/09/14 20:37:13 [TRACE] Preserving existing state lineage "aace983a-9729-4997-9ca0-121b4e2f698c"
--- PASS: TestMantaClient (7.28s)

@jen20
Copy link
Contributor

jen20 commented Sep 14, 2016

Hi @watters! I've run the tests and reviewed this and it seems fine. I've rebased to master and squashed some of the commits (leaving the vendoring separate). Finally, I've fixed up the documentation sidebar for remote state to include the Manta option. This has landed in 72a341b and b4eb63d, though GitHub sadly doesn't reflect that here! Thanks for your work!

@jen20 jen20 closed this Sep 14, 2016
@watters
Copy link
Contributor Author

watters commented Sep 14, 2016

Thank you!

@watters watters mentioned this pull request Sep 28, 2016
@ghost
Copy link

ghost commented Apr 22, 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 Apr 22, 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.

2 participants