-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Restrict component identifiers character set #9208
Labels
Milestone
Comments
mx-psi
added a commit
that referenced
this issue
Feb 2, 2024
**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
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 6, 2024
**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
mx-psi
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 8, 2024
…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
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 8, 2024
**Description:** More changes in tests for open-telemetry/opentelemetry-collector/pull/9472 **Link to tracking Issue:** open-telemetry/opentelemetry-collector/pull/9208
mx-psi
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 8, 2024
**Description**: Some more changes in tests for open-telemetry/opentelemetry-collector/pull/9472 **Link to tracking Issue**: open-telemetry/opentelemetry-collector/issues/9208
anthoai97
pushed a commit
to anthoai97/opentelemetry-collector-contrib
that referenced
this issue
Feb 12, 2024
…1038) **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
anthoai97
pushed a commit
to anthoai97/opentelemetry-collector-contrib
that referenced
this issue
Feb 12, 2024
…ame` when passing literal strings (open-telemetry#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
anthoai97
pushed a commit
to anthoai97/opentelemetry-collector-contrib
that referenced
this issue
Feb 12, 2024
**Description:** More changes in tests for open-telemetry/opentelemetry-collector/pull/9472 **Link to tracking Issue:** open-telemetry/opentelemetry-collector/pull/9208
anthoai97
pushed a commit
to anthoai97/opentelemetry-collector-contrib
that referenced
this issue
Feb 12, 2024
…metry#31135) **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
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 14, 2024
) **Description:** Passes a `component.Type` to `NewDefaultScraperControllerSettings` in the `hostmetrics` receiver tests. **Link to tracking Issue:** open-telemetry/opentelemetry-collector/issues/9208
mx-psi
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Feb 14, 2024
**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
**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
…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
**Description:** Fixes configschema uses of `component.Type` **Link to tracking Issue:** open-telemetry/opentelemetry-collector/issues/9208
nslaughter
pushed a commit
to nslaughter/opentelemetry-collector-contrib
that referenced
this issue
May 23, 2024
…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
nslaughter
pushed a commit
to nslaughter/opentelemetry-collector-contrib
that referenced
this issue
May 23, 2024
**Description:** Fixes configschema uses of `component.Type` **Link to tracking Issue:** open-telemetry/opentelemetry-collector/issues/9208
This was referenced Jul 19, 2024
mx-psi
pushed a commit
that referenced
this issue
Jul 25, 2024
#### 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.-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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 acomponent.ID
.The text was updated successfully, but these errors were encountered: