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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 7 commits into from
+63 −19

Conversation

jackgopack4
Copy link
Contributor

@jackgopack4 jackgopack4 commented Nov 22, 2024 •
edited
Loading

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

mx-psi
mx-psi requested changes Nov 26, 2024
View reviewed changes
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
Comment on lines 253 to 254
// remove leading/trailing quotation marks to avoid issues with the API key
c.API.Key = configopaque.String(strings.Trim(string(c.API.Key), `"'`))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not do this. If we want to improve user experience we should validate the API format (e.g. I think a reasonable check that we can do is to check for any character that is not an ASCII alphanumeric and error out when those are present)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the approach to validate hex characters via regex

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mx-psi mx-psi mx-psi requested changes

@ankitpatel96 ankitpatel96 Awaiting requested review from ankitpatel96

@dineshg13 dineshg13 Awaiting requested review from dineshg13

@jade-guiton-dd jade-guiton-dd Awaiting requested review from jade-guiton-dd

@liustanley liustanley Awaiting requested review from liustanley

@mackjmr mackjmr Awaiting requested review from mackjmr

@songy23 songy23 Awaiting requested review from songy23 songy23 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@atoulme atoulme

@djaglowski djaglowski

Labels
pkg/datadog
Projects
None yet
Milestone
No milestone
4 participants
@jackgopack4 @mx-psi @atoulme @djaglowski