-
Notifications
You must be signed in to change notification settings - Fork 123
Add credentials.tfrc.json support for TFE authentication (#228) #265
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
base: main
Are you sure you want to change the base?
Add credentials.tfrc.json support for TFE authentication (#228) #265
Conversation
Read TFE token from ~/.terraform.d/credentials.tfrc.json as fallback when TFE_TOKEN environment variable is not set. This allows users who have run `terraform login` to use the MCP server without additional configuration. Priority chain: HTTP Header → Environment Variable → credentials.tfrc.json Co-Authored-By: Claude Opus 4.5 <[email protected]>
Changelog WarningPlease add a changelog entry to |
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes 1 out of 2 committers have signed the CLA.
Vishnu Ravindra seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
jrhouston
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Vishnu 😄
Looks good, just a couple of test/error handling comments.
pkg/client/credentials_test.go
Outdated
| {"standard https", "https://app.terraform.io", "app.terraform.io"}, | ||
| {"with port", "https://tfe.example.com:8443", "tfe.example.com"}, | ||
| {"with path", "https://app.terraform.io/api/v2", "app.terraform.io"}, | ||
| {"http scheme", "http://localhost:8080", "localhost"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH – since extractHostname is a private function that just adds some sugar on top of url.Parse, I don't see much value in these tests. They are just re-testing the url.Parse() function which already has a comprehensive suite of tests that look exactly like this. The logic we've introduced here is returning "" for empty input or invalid URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've trimmed the test cases down to only the edge cases
|
|
||
| // ReadCredentialsFile reads the Terraform CLI credentials file and returns | ||
| // the token for the specified hostname. Returns empty string if not found. | ||
| func ReadCredentialsFile(hostname string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works but the function fails silently, and there's a few real failure modes we'd probably want to at least warn the user about in the logs:
- The path / file simply doesn't exist
- We tried to read the file but don't have permissions to do so.
- We read the file successfully but couldn't parse it.
- We read and parse the file successfully but the hostname we've configured doesn't exist in the file.
Right now if I try to use the credentials file but misconfigure it in some way, the server boots up OK but now I have a task of trying to figure out why it doesn't like my credentials file.
Fixes #228
~/.terraform.d/credentials.tfrc.jsonterraform logincan now use the MCP server without settingTFE_TOKENenvironmentvariable
New credential resolution chain:
HTTP Header → Environment Variable → credentials.tfrc.json → Error
Changes
pkg/client/credentials.gopkg/client/credentials_test.gopkg/client/tfe_client.goTest plan
go test ./pkg/client/...)go test ./...)credentials file