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.

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

[exporter/datadog] add basic API key validation on startup #36510

Merged

Conversation

jackgopack4
Copy link
Contributor

@jackgopack4 jackgopack4 commented Nov 22, 2024

Description

adds basic hexadecimal character validation to Datadog API key on startup

Link to tracking issue

Fixes #36509

Testing

new "invalid API Key" test

Documentation

changelog file

@jackgopack4 jackgopack4 marked this pull request as ready for review November 22, 2024 21:24
@jackgopack4 jackgopack4 requested a review from a team as a code owner November 22, 2024 21:24
@mx-psi
Copy link
Member

mx-psi commented Nov 25, 2024

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -56,3 +56,3 @@
        	            	  API: (config.APIConfig) {
        	            	-  Key: (configopaque.String) "",
        	            	+  Key: (configopaque.String) (len=10) "[REDACTED]",
        	            	   Site: (string) (len=13) "datadoghq.com",
        	Test:       	TestUnmarshal/unmarshall_confighttp_client_configs

@jackgopack4
Copy link
Contributor Author

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -56,3 +56,3 @@
        	            	  API: (config.APIConfig) {
        	            	-  Key: (configopaque.String) "",
        	            	+  Key: (configopaque.String) (len=10) "[REDACTED]",
        	            	   Site: (string) (len=13) "datadoghq.com",
        	Test:       	TestUnmarshal/unmarshall_confighttp_client_configs

going to look into this, not clear immediately if it's just broken/outdated test or if my change broke the intended behavior

@jackgopack4 jackgopack4 marked this pull request as draft November 25, 2024 14:42
@jackgopack4 jackgopack4 marked this pull request as ready for review November 25, 2024 15:51
@jackgopack4
Copy link
Contributor Author

there may still be a discussion to be had around if we need to improve our API Key validation logic, but this keeps the behavior of v0.110.0 and prior so as to not break existing customers

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error comes from changes in the confmap and it is intentional. You can see the "0123" row in https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/env-vars.md#comparison-table-with-current-behavior that is this very case. I think we should not do this change and just think about how to improve the user experience (I left a concrete suggestion below).

pkg/datadog/config/config.go Outdated Show resolved Hide resolved
@jackgopack4 jackgopack4 changed the title fix api key validation issue when quotations are escaped inside string [exporter/datadog] add basic API key validation on startup Nov 26, 2024
@github-actions github-actions bot added the exporter/datadog Datadog components label Nov 27, 2024
@jackgopack4 jackgopack4 requested a review from mx-psi December 2, 2024 15:38
pkg/datadog/config/config.go Outdated Show resolved Hide resolved
pkg/datadog/config/config.go Outdated Show resolved Hide resolved
pkg/datadog/config/config.go Outdated Show resolved Hide resolved
pkg/datadog/config/config.go Outdated Show resolved Hide resolved
pkg/datadog/config/config.go Outdated Show resolved Hide resolved
@jackgopack4 jackgopack4 requested a review from mx-psi December 2, 2024 18:44
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can you fix the merge conflicts so I can merge?

@jackgopack4
Copy link
Contributor Author

@mx-psi fixed merged conflicts and failing tests, should be good to go

@mx-psi mx-psi merged commit 66b0c3c into open-telemetry:main Dec 4, 2024
158 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 4, 2024
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this pull request Dec 5, 2024
…metry#36510)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
adds basic hexadecimal character validation to Datadog API key on
startup

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes open-telemetry#36509

<!--Describe what testing was performed and which tests were added.-->
#### Testing
new "invalid API Key" test

<!--Describe the documentation added.-->
#### Documentation
changelog file
<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <[email protected]>
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
…metry#36510)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
adds basic hexadecimal character validation to Datadog API key on
startup

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes open-telemetry#36509

<!--Describe what testing was performed and which tests were added.-->
#### Testing
new "invalid API Key" test

<!--Describe the documentation added.-->
#### Documentation
changelog file
<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <[email protected]>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…metry#36510)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
adds basic hexadecimal character validation to Datadog API key on
startup

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes open-telemetry#36509

<!--Describe what testing was performed and which tests were added.-->
#### Testing
new "invalid API Key" test

<!--Describe the documentation added.-->
#### Documentation
changelog file
<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Pablo Baeyens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants