chore: track terraform module source type in telemetry #15590
Conversation
| ModuleSourceTypeUnknown ModuleSourceType = "unknown" | ||
| ) | ||
|
|
||
| func GetModuleSourceType(source string) ModuleSourceType { |
There was a problem hiding this comment.
It might be good for future readers to add a doc comment to this function along with a link to https://developer.hashicorp.com/terraform/language/modules/sources, as they may have some similar questions to me before reading:
- Wait, these aren't just URLS?! Nope.
- There's difference between how relative and absolute local paths are handled? yup
- Wait, you can have "localterraform.com" as a generic "local" hostname? yup
- Wait, you can just use an un-prefixed GitHub registry as a module source? yup
- etc...
coderd/telemetry/telemetry.go
Outdated
| if !strings.Contains(source, "://") && !strings.Contains(source, ".") && strings.Contains(source, "/") { | ||
| return ModuleSourceTypePublicRegistry | ||
| } |
There was a problem hiding this comment.
Should we tighten this condition up a bit and assert that it follows the form namespace/name/provider? And maybe move it up slightly?
|
@johnstcn I made changes according to your feedback. Thanks! |
| if strings.Contains(source, "registry.terraform.io") { | ||
| return ModuleSourceTypePublicRegistry | ||
| } | ||
| if strings.Contains(source, "app.terraform.io") || strings.Contains(source, "localterraform.com") { | ||
| return ModuleSourceTypePrivateRegistry | ||
| } | ||
| if strings.Contains(source, "registry.coder.com") { | ||
| return ModuleSourceTypeCoderRegistry | ||
| } |
There was a problem hiding this comment.
What about registry.coder.com.mydomain.foo.bar? Should we instead use strings.HasPrefix here too?
There was a problem hiding this comment.
@johnstcn I wasn't aware that people could access the coder registry that way. How does this work? Can people self-host our registry?
There was a problem hiding this comment.
As I write this, I realise that prefix-checking won't even help here unless we attempt to parse the module as an URL and extract only the hostname.
In any case, I don't see any reason that someone sufficiently motiviated couldn't do this if they controlled the domain mydomain.foo.bar and used something like outsiders/citizen to self-host a registry.
It's more of a contrived hypothetical though and I'm not going to block on this!
There was a problem hiding this comment.
I think it's acceptable to misclassify a small percentage of modules. With telemetry, we’re more focused on aggregates, and cases like this are unlikely to affect the overall picture.
| require.Equal(t, modules[1].Source, "registry.coder.com/terraform/aws") | ||
| require.Equal(t, modules[1].Version, "1.0.0") | ||
| require.Equal(t, modules[1].SourceType, telemetry.ModuleSourceTypeCoderRegistry) | ||
| }) | ||
| t.Run("ModuleSourceType", func(t *testing.T) { |
Addresses https://github.com/coder/nexus/issues/141