Closed pdecat closed 2 years ago
Hi @sullivtr, I submitted this one as draft to gather interest for having this feature into your provider and hopefully get early feedback about design choices.
Thanks for this @pdecat. I’ll look over this sometime today and provide feedback.
I have a couple questions but I’ll need to review the code first 😊
Just to be sure (and for posterity on this PR), the provider's login_query will execute upon every terraform run, is that correct? If not, we will need to make sure the auth headers cannot go stale.
the provider's login_query will execute upon every terraform run, is that correct?
That's correct. There is no preserved state for provider configuration.
There is no preserved state for provider configuration
Oh, when I wrote this, it made me doubt so I had to double check and turns out I was wrong :facepalm: :
cat terraform.tfstate
{
"version": 4,
"terraform_version": "1.1.2",
"serial": 3,
"lineage": "5f334abd-d37c-d16e-bbaf-31303bdfe587",
"outputs": {},
"resources": [
{
"mode": "data",
"type": "graphql_query",
"name": "login",
"provider": "provider[\"registry.terraform.io/sullivtr/graphql\"].login",
"instances": [
{
"schema_version": 0,
"attributes": {
"id": "2977850100",
"query": "mutation Login($apiKey: String!) {loginAPI(apiKey: $apiKey) {accessToken}}",
"query_response": "{\"data\":{\"loginAPI\":{\"accessToken\":\"******\"}}}\n",
"query_variables": {
"apiKey": "******"
}
},
"sensitive_attributes": []
}
]
},
{
"mode": "data",
"type": "graphql_query",
"name": "query_req",
"provider": "provider[\"registry.terraform.io/sullivtr/graphql\"]",
"instances": [
{
"schema_version": 0,
"attributes": {
"id": "1737093455",
"query": "query queryReq {\n dataSources {\n datasetName\n cloudType\n }\n}\n",
"query_response": "{\"data\":{\"dataSources\":[{\"datasetName\":\"AWS_CUR\",\"cloudType\":\"AWS\"},{\"datasetName\":\"GCP_BILLING_EXPORT\",\"cloudType\":\"GCP\"}]}}\n",
"query_variables": null
},
"sensitive_attributes": []
}
]
}
]
}
I just lost my desired benefit from doing this...
Or not, I was checking the wrong state :sweat_smile:
cat terraform.tfstate
{
"version": 4,
"terraform_version": "1.1.2",
"serial": 4,
"lineage": "5f334abd-d37c-d16e-bbaf-31303bdfe587",
"outputs": {},
"resources": [
{
"mode": "data",
"type": "graphql_query",
"name": "query_req",
"provider": "provider[\"registry.terraform.io/sullivtr/graphql\"]",
"instances": [
{
"schema_version": 0,
"attributes": {
"id": "1737093455",
"query": "query queryReq {\n dataSources {\n datasetName\n cloudType\n }\n}\n",
"query_response": "{\"data\":{\"dataSources\":[{\"datasetName\":\"AWS_CUR\",\"cloudType\":\"AWS\"},{\"datasetName\":\"GCP_BILLING_EXPORT\",\"cloudType\":\"GCP\"}]}}\n",
"query_variables": null
},
"sensitive_attributes": []
}
]
}
]
}
Or not, I was checking the wrong state 😅
cat terraform.tfstate { "version": 4, "terraform_version": "1.1.2", "serial": 4, "lineage": "5f334abd-d37c-d16e-bbaf-31303bdfe587", "outputs": {}, "resources": [ { "mode": "data", "type": "graphql_query", "name": "query_req", "provider": "provider[\"registry.terraform.io/sullivtr/graphql\"]", "instances": [ { "schema_version": 0, "attributes": { "id": "1737093455", "query": "query queryReq {\n dataSources {\n datasetName\n cloudType\n }\n}\n", "query_response": "{\"data\":{\"dataSources\":[{\"datasetName\":\"AWS_CUR\",\"cloudType\":\"AWS\"},{\"datasetName\":\"GCP_BILLING_EXPORT\",\"cloudType\":\"GCP\"}]}}\n", "query_variables": null }, "sensitive_attributes": [] } ] } ] }
Awesome. I think it would be good to have an E2E test case for this. This can be done by adding a dummy endpoint on the E2E graphql server that mimics a token request/response, then have an E2E flow that exercises it over a couple TF lifecycles. I haven’t documented the tests all that well so let me know if you run into issues there 😊
Thanks! I'll implement the E2E test case ASAP.
Merging #59 (df89e9a) into master (37f8088) will increase coverage by
0.85%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 86.48% 87.34% +0.85%
==========================================
Files 3 3
Lines 74 79 +5
==========================================
+ Hits 64 69 +5
- Misses 6 9 +3
+ Partials 4 1 -3
Impacted Files | Coverage Δ | |
---|---|---|
graphql/data_source_graphql_query.go | 89.28% <ø> (-3.31%) |
:arrow_down: |
graphql/provider.go | 100.00% <100.00%> (ø) |
|
graphql/query_executor.go | 66.66% <100.00%> (+4.76%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 37f8088...df89e9a. Read the comment docs.
Submitted the new implementation supporting only OAuth 2.0 authentication scheme.
Configuration of the provider from a user's perspective now looks like this:
provider "graphql" {
url = "https://apps.cloudhealthtech.com/graphql"
oauth2_login_query = "mutation Login($apiKey: String!) {loginAPI(apiKey: $apiKey) {accessToken}}"
oauth2_login_query_variables = {
"apiKey" = var.cloudhealth_api_key
}
oauth2_login_query_value_attribute = "data.loginAPI.accessToken"
}
I added the E2E test case, please tell me if that's what you had in mind.
I'm currently writing the documentation for this, and found two separate directories with similar but not exactly the same content:
Is it needed to maintain both?
I'm currently writing the documentation for this, and found two separate directories with similar but not exactly the same content:
- https://github.com/sullivtr/terraform-provider-graphql/tree/master/docs => https://registry.terraform.io/providers/sullivtr/graphql/latest/docs
- https://github.com/sullivtr/terraform-provider-graphql/tree/master/docsite => https://sullivtr.github.io/terraform-provider-graphql
Is it needed to maintain both?
So the docs under /docs is for the terraform registry site. For that you can keep the documentation high level, really just list the available properties with descriptions. As for the stuff under /docsite, that is the more detailed documentation where I expand on why/how things work with deeper examples
I originally built the docsite before the terraform registry was a thing, so I'd like to eventually move everything onto the TF registry docs, but some of it gets in the weeds a bit and might not be appropriate for the registry level docs.
@pdecat looking good. Thanks for the PR. I’ll do one more Passover tonight and get a release out of all is good. Today is a holiday so I’ll be out most of the day with family, but I’ll do my best to get this review wrapped up asap.
Hi @sullivtr, no hurry, take all the time you need, I'm already (UTC+1 local time) on vacation for a week 🙂
@pdecat Release v2.4.0
of the provider is being published right now. It will include the OAuth2 feature you added with this pull request. Thanks again for your contribution.
Hi @sullivtr, v2.4.0 is working great, thanks for the quick review and merge!
This PR adds support for authenticating to a GraphQL API server using a login query (or mutation) with configuration at the provider level.
Example configuration with this PR:
The main benefit of configuring the login query and credentials at the provider level is avoiding storing API credentials in the terraform state.
Without this PR, it currently requires using an additional provider configuration and a query datasource to perform the authentication query to configure the main provider:
TODO:
Note: I did not explore what authentication queries/mutations other GraphQL API servers are using.