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.

Sign up for GitHub

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

Jump to bottom

Restrict component identifiers character set #9208

Closed
mx-psi opened this issue Jan 3, 2024 · 0 comments · Fixed by #9472
Closed

Restrict component identifiers character set #9208

mx-psi opened this issue Jan 3, 2024 · 0 comments · Fixed by #9472
Labels
area:component priority:p1 High release:required-for-ga Must be resolved before GA release
Milestone
go.opentelemetry....

Comments

@mx-psi
Copy link
Member

mx-psi commented Jan 3, 2024

Similar to what we did for feature gates on #8766, we should have a clear idea on what characters we want to allow on component names and verify it.

At a minimum, we want to disallow / characters in the component name and have the component name not be empty. My proposal would be to further restrict this for now to an ASCII alphanumeric + _ identifier, starting with an ASCII alphabetic character.

This would be a breaking change, so it is needed before GA of the component package. We should do it as part of building a component.Type or a component.ID.
@mx-psi mx-psi added release:required-for-ga Must be resolved before GA release priority:p1 High area:component labels Jan 3, 2024
@mx-psi mx-psi mentioned this issue Jan 29, 2024
Merged
mx-psi added a commit that referenced this issue Feb 2, 2024
@mx-psi
[component] Add MustNewType constructor for component.Type (#9414)
26c157e 
**Description:** 

- Adds `component.MustNewType` to create a type. This function panics if
the type has invalid characters. Add similar functions
`component.MustNewID` and `component.MustNewIDWithName`.
- Adds `component.Type.String` to recover the string
- Use `component.MustNewType`, `component.MustNewID`,
`component.MustNewIDWithName` and `component.Type.String` everywhere in
this codebase. To do this I changed `component.Type` into an opaque
struct and checked for compile-time errors.

Some notes:

1. All components currently on core and contrib follow this rule. This
is still breaking for other components.
2. A future PR will change this into a struct, to actually validate this
(right now you can just do `component.Type("anything")` to bypass
validation). I want to do this in two steps to avoid breaking contrib
tests: we first introduce this function, and after that we change into a
struct.

**Link to tracking Issue:** Updates #9208
@mx-psi mx-psi mentioned this issue Feb 5, 2024
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 6, 2024
@mx-psi
[chore][mdatagen] Use MustNewType for metadata.Type (#31038)
8ae1539 
**Description:** 

Follow up to open-telemetry/opentelemetry-collector/pull/9414, adds the
same changes in `mdatagen` regarding validation of `component.Type`s.

On this PR the validation logic and templating is a bit more complex
because of subcomponents. I will add these changes back on core once
this PR is merged.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
This was referenced Feb 6, 2024
Merged
Closed
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 8, 2024
@mx-psi
[chore] Switch to component.MustNewID and `component.MustNewIDWithN…
66a5fa5 
…ame` when passing literal strings (#31125)

**Description:** 

Runs the following commands:

- `rg "component.NewID\(\".*?\"\)" -l | xargs sd
'component.NewID\((".*?")\)' 'component.MustNewID($1)'`
- `rg "component.NewIDWithName\(\".*?\"" -l | xargs sd
'component.NewIDWithName\((".*?")' 'component. MustNewIDWithName($1'`

Additionally, makes some manual fixes

Needed for open-telemetry/opentelemetry-collector/pull/9472

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
@mx-psi mx-psi mentioned this issue Feb 8, 2024
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 8, 2024
@mx-psi
[chore] Migrate even more component.Type tests (#31131)
07e5786 
**Description:** More changes in tests for
open-telemetry/opentelemetry-collector/pull/9472

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/pull/9208
@mx-psi mx-psi mentioned this issue Feb 8, 2024
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 8, 2024
@mx-psi
[chore] Even more one-off fixes for component.Type changes (#31135)
cd0db68 
**Description**: Some more changes in tests for
open-telemetry/opentelemetry-collector/pull/9472

**Link to tracking Issue**:
open-telemetry/opentelemetry-collector/issues/9208
@mx-psi mx-psi mentioned this issue Feb 14, 2024
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 14, 2024
@mx-psi
[chore][receiver/hostmetrics] Use metadata.Type in scraper tests (#31257
1be86a6 
)

**Description:** Passes a `component.Type` to
`NewDefaultScraperControllerSettings` in the `hostmetrics` receiver
tests.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
@mx-psi mx-psi mentioned this issue Feb 14, 2024
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 14, 2024
@mx-psi
[chore] Fix configschema uses of component.Type (#31263)
b7aea84 
**Description:** Fixes configschema uses of `component.Type`

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
mx-psi added a commit that referenced this issue Mar 6, 2024
@mx-psi
[component] Change component.Type underlying type to a struct (#9472)
b269362 
**Description:** 

Follow up to #9414 and
open-telemetry/opentelemetry-collector-contrib/pull/31038.

**Link to tracking Issue:** Fixes #9208.
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
@mx-psi @XinRanZhAWS
[chore][receiver/hostmetrics] Use metadata.Type in scraper tests (ope…
07b8c17 
…n-telemetry#31257)

**Description:** Passes a `component.Type` to
`NewDefaultScraperControllerSettings` in the `hostmetrics` receiver
tests.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
@mx-psi @XinRanZhAWS
[chore] Fix configschema uses of component.Type (open-telemetry#31263)
b822b02 
**Description:** Fixes configschema uses of `component.Type`

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
@FISHMANPET FISHMANPET mentioned this issue Mar 26, 2024
Closed
@mx-psi mx-psi mentioned this issue Apr 1, 2024
Closed
This was referenced Jul 19, 2024
Closed
Merged
mx-psi pushed a commit that referenced this issue Jul 25, 2024
@mwear
[component] Restrict character set for component.ID name (#10674)
95902c1 
#### Description
While working on
#10495 we
discovered that there are not any restrictions on the `name` field of a
`component.ID`. There are restrictions on the `type` field introduced in
#9208. This PR adds similar restrictions to `name`.

A type must
- have at least one character,
- start with an ASCII alphabetic character and
- can only contain ASCII alphanumeric characters and '_'.

I found that we need a slightly different set of rules for name as some
tests use a digit and others use a uuid as a name. A name is still
optional, but if it's provided it must:
- have at least one character,
- start with an ASCII alphanumeric character and
- can only contain ASCII alphanumeric characters, '_', and '-'.

I'd be willing to adjust these restrictions if anyone has any opinions
on what should or should not be allowed.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #10673

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Describe the documentation added.-->
#### Documentation
Code comments

<!--Please delete paragraphs that you did not use before submitting.-->
lapis-lark pushed a commit to lapis-lark/otel-cassandra-exporter-rework that referenced this issue Nov 4, 2024
@mx-psi
[chore][mdatagen] Use MustNewType for metadata.Type (#31038)
0fb0ce0 
**Description:** 

Follow up to open-telemetry/opentelemetry-collector/pull/9414, adds the
same changes in `mdatagen` regarding validation of `component.Type`s.

On this PR the validation logic and templating is a bit more complex
because of subcomponents. I will add these changes back on core once
this PR is merged.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector/issues/9208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:component priority:p1 High release:required-for-ga Must be resolved before GA release
Projects
Collector: v1
Archived in project
Milestone
go.opentelemetry.io/collector/compone...
Development

Successfully merging a pull request may close this issue.

[component] Change component.Type underlying type to a struct mx-psi/opentelemetry-collector
1 participant
@mx-psi