httpclient: Introduce composable UserAgent() #22272

radeksimko · 2019-07-31T16:09:53Z

Context

Providers need to know version of Terraform that is calling them, so they can report this via User-Agent header to upstream APIs. Currently providers do this basically by leveraging Terraform's version package as Terraform provides the SDK for interacting with core and is therefore imported to providers.

Such version is inaccurate and bb9ae50 already introduced a way for providers to source real version of Terraform (the binary that is talking to provider's gRPC server).

Proposal

This new interface reflects recent changes to the protocol in 0.12 and proposes standard User-Agent format for providers to use. It will also allow providers to avoid depending on the version package which will not be available in the extracted SDK v1.

It is meant to replace the following ones (in use by some providers):

httpclient.UserAgentString() (e.g. AzureRM, Google)
terraform.UserAgentString() (e.g. OpenStack, ProfitBricks)
terraform.VersionString() (e.g. AWS, AzureStack, DigitalOcean, Kubernetes)

This also proposes the initial UA string to be set to

HashiCorp Terraform/X.Y.Z (+https://www.terraform.io)

and some providers already defined this convention for encoding provider version

terraform-provider-name/X.Y.Z

Compatibility

Since the TerraformVersion field was introduced in the gRPC protocol of 0.12, it is worth noting that 0.10 and 0.11 do not send any version and it is up to the provider to decide what to do in such case. If this is not "special-case'd", UA will be constructed with empty version string, i.e.

HashiCorp Terraform/ (+https://www.terraform.io)

httpclient/useragent.go

appilon

LGTM

kmoe · 2019-08-01T16:29:30Z

httpclient/useragent.go

+		b.WriteString(fmt.Sprintf("/%s", uap.Version))
+	}
+	if uap.Comment != "" {
+		if uap.Name != "" {


This does not appear to be RFC7231 compliant.

From section 5.5.3:

User-Agent = product *( RWS ( product / comment ) )

The User-Agent field-value consists of one or more product
identifiers, each followed by zero or more comments

@paultyng suggested it is?

I have to admit I'm struggling to interpret the RFC, whilst also correlating all this with reality.

The RFC mentions the following ABNF patterns:

User-Agent = product *( RWS ( product / comment ) ) product = token ["/" product-version] product-version = token

(where / means OR)

You are right that it does mention one or more product identifiers.

I'm inclined to apply the pragmatic approach here, as long as it doesn't hurt RFC compliant input (i.e. as long as valid input is still parsed correctly), which I don't think it does?

I agree that the RFC is not entirely clear: there seems to be an inconsistency between the grammar and the text describing it.

In the ABNF patterns you reproduced, the User-Agent rule contains the rule name product only once, with no variable repetition. Assuming that the rule name product corresponds with "product identifier" in the text, the grammar does not seem to allow for more than one product identifier, making the text "one or more product identifiers" misleading.

However, the example given later in the same section of the RFC seems to show the intent more clearly:

User-Agent: CERN-LineMode/2.15 libwww/2.17b3

Based on this, I believe the grammar should have been:

User-Agent = 1*( product *( RWS ( product / comment ) ) ) product = token ["/" product-version] product-version = token

or possibly even:

User-Agent = 1*product *( RWS ( product / comment ) ) product = token ["/" product-version] product-version = token

Note that neither option for resolving the ambiguity permits product to be absent entirely from the User-Agent field.

I think the three examples on user-agents.org of the form ( token ) are not spec compliant.

I agree with your interpretation, but also I don't mind the pragmatism over strict RFC compliance.

I'm happy to discuss the regexp in the other function, but technically this condition just prevents leading space in (sic non-compliant) string?

I don't have a strong opinion either way. 🤷‍♂️

apparentlymart

This feels like a lot of new API surface for something that was previously very simple, but without seeing what the callers of this looks like I have to assume that complexity is warranted.

If this complexity is necessary, maybe it's worth later on factoring it out into a separate library altogether so that it can be shared (rather than duplicated) between Terraform Core and the providers, along with anything else that might need it.

httpclient/useragent.go

radeksimko · 2019-08-02T11:18:14Z

I understand you concerns about complexity, but I just took the lowest common denominator, e.g. AWS

Also with this new interface hides most of the complexity anyway. e.g. if you wanted to keep building strings in providers, you can:

ua := httpclient.TerraformUserAgent(c.terraformVersion)
fmt.Sprintf("%s ...", ua)

The main motivation here really is to allow providers to not depend on version package whilst having a reasonable answer to those who ask "What do I use instead of httpclient.UserAgentString() or terraform.UserAgentString()" whilst also gently nudging providers to try to align around the same/similar format.

We can have a wider debate about this as part of SDK v2, if we feel it's important enough compared to other things.

Also we discussed the significance of 1.0 in HashiCorp/1.0 via Slack and given that version-less string is RFC-compliant, I removed it.

radeksimko · 2019-08-02T14:46:06Z

Given the feedback I have received recently I will let this sit here for a few days and have a think about the solution from a higher perspective.

Perhaps all we need is this

func TerraformUserAgent(version string) string {
  return fmt.Sprintf("HashiCorp Terraform/%s", version)
}

This interface is meant to replace the following ones (in use by some providers): - httpclient.UserAgentString() (e.g. AzureRM, Google) - terraform.UserAgentString (e.g. OpenStack, ProfitBricks) - terraform.VersionString (e.g. AWS, AzureStack, DigitalOcean, Kubernetes) This also proposes the initial UA string to be set to HashiCorp Terraform/X.Y.Z (+https://www.terraform.io)

radeksimko · 2019-08-05T12:36:41Z

Apologies everyone for the back-and-forth on this. After discussing this with more people I decided to reduce the implementation to bare minimum and leave the necessary complexity at the implementation side.

The complex case requiring more than just string is basically just AWS anyway and we can deal with it there.

appilon

I'm happy with the new approach as well

ghost · 2019-09-12T01:55:53Z

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.

radeksimko added enhancement provider-sdk labels Jul 31, 2019

radeksimko requested review from a team July 31, 2019 16:09

This was referenced Jul 31, 2019

provider: Use real Terraform version in UA header hashicorp/terraform-provider-aws#9570

Merged

provider: Use real Terraform version in UA header hashicorp/terraform-provider-google#4140

Closed

paultyng reviewed Jul 31, 2019

View reviewed changes

httpclient/useragent.go Outdated Show resolved Hide resolved

appilon approved these changes Jul 31, 2019

View reviewed changes

radeksimko mentioned this pull request Aug 1, 2019

provider: Use real Terraform version in UA header hashicorp/terraform-provider-google-beta#994

Closed

radeksimko requested a review from paultyng August 1, 2019 13:57

This was referenced Aug 1, 2019

provider: Use real Terraform version in UA header cloudflare/terraform-provider-cloudflare#437

Merged

provider: Use real Terraform version in UA header fastly/terraform-provider-fastly#182

Merged

kmoe reviewed Aug 1, 2019

View reviewed changes

apparentlymart approved these changes Aug 1, 2019

View reviewed changes

httpclient/useragent.go Outdated Show resolved Hide resolved

radeksimko force-pushed the f-httpclient-ua branch from 8ba87d9 to 34d90d4 Compare August 5, 2019 10:08

radeksimko requested review from kmoe, appilon, apparentlymart and a team and removed request for apparentlymart, paultyng, kmoe and appilon August 5, 2019 12:37

appilon approved these changes Aug 8, 2019

View reviewed changes

kmoe approved these changes Aug 12, 2019

View reviewed changes

radeksimko merged commit 98a796c into master Aug 12, 2019

radeksimko deleted the f-httpclient-ua branch August 12, 2019 19:20

radeksimko mentioned this pull request Aug 22, 2019

terraform: Use real version in UA header GoogleCloudPlatform/magic-modules#2229

Merged

ghost locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

httpclient: Introduce composable UserAgent() #22272

httpclient: Introduce composable UserAgent() #22272

radeksimko commented Jul 31, 2019 •

edited

Loading

appilon left a comment

kmoe Aug 1, 2019

radeksimko Aug 1, 2019 •

edited

Loading

radeksimko Aug 1, 2019

kmoe Aug 1, 2019 •

edited

Loading

radeksimko Aug 1, 2019

apparentlymart left a comment

radeksimko commented Aug 2, 2019

radeksimko commented Aug 2, 2019

radeksimko commented Aug 5, 2019

appilon left a comment

ghost commented Sep 12, 2019

httpclient: Introduce composable UserAgent() #22272

httpclient: Introduce composable UserAgent() #22272

Conversation

radeksimko commented Jul 31, 2019 • edited Loading

Context

Proposal

Compatibility

appilon left a comment

Choose a reason for hiding this comment

kmoe Aug 1, 2019

Choose a reason for hiding this comment

radeksimko Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

radeksimko Aug 1, 2019

Choose a reason for hiding this comment

kmoe Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

radeksimko Aug 1, 2019

Choose a reason for hiding this comment

apparentlymart left a comment

Choose a reason for hiding this comment

radeksimko commented Aug 2, 2019

radeksimko commented Aug 2, 2019

radeksimko commented Aug 5, 2019

appilon left a comment

Choose a reason for hiding this comment

ghost commented Sep 12, 2019

radeksimko commented Jul 31, 2019 •

edited

Loading

radeksimko Aug 1, 2019 •

edited

Loading

kmoe Aug 1, 2019 •

edited

Loading