Skip to content

contrib/aws: fix sns nil pointer dereference#2025

Merged
zarirhamza merged 5 commits into
DataDog:mainfrom
satorunooshie:main
Jun 12, 2023
Merged

contrib/aws: fix sns nil pointer dereference#2025
zarirhamza merged 5 commits into
DataDog:mainfrom
satorunooshie:main

Conversation

@satorunooshie

@satorunooshie satorunooshie commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes bugs since v1.51.0

https://github.com/aws/aws-sdk-go-v2/blob/fa15a743cc4b93dfc290d2622c92b3969ca004cf/service/sns/api_op_Publish.go#L114-L133

	// The phone number to which you want to deliver an SMS message. Use E.164 format.
	// If you don't specify a value for the PhoneNumber parameter, you must specify a
	// value for the TargetArn or TopicArn parameters.
	PhoneNumber *string

	// Optional parameter to be used as the "Subject" line when the message is
	// delivered to email endpoints. This field will also be included, if present, in
	// the standard JSON messages delivered to other endpoints. Constraints: Subjects
	// must be ASCII text that begins with a letter, number, or punctuation mark; must
	// not include line breaks or control characters; and must be less than 100
	// characters long.
	Subject *string

	// If you don't specify a value for the TargetArn parameter, you must specify a
	// value for the PhoneNumber or TopicArn parameters.
	TargetArn *string

	// The topic you want to publish to. If you don't specify a value for the TopicArn
	// parameter, you must specify a value for the PhoneNumber or TargetArn parameters.
	TopicArn *string

Motivation

Fixes #2001

Describe how to test/QA your changes

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@satorunooshie
satorunooshie requested a review from a team June 8, 2023 06:33

@zARODz11z zARODz11z left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @satorunooshie, thank you for the contribution! Adding the phone number seems out of scope for fixing #2001 due to privacy concerns that we'd need to reevaluate further. Lets remove the addition of the phone number tag for now.

@satorunooshie

satorunooshie commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

@zARODz11z Hello, thanks for your review. Actually I was wondering how I should handle the phone number.
fixed at 5aa235b

@zARODz11z

Copy link
Copy Markdown
Contributor

@satorunooshie Thank you for the changes! That is what I was looking for.

@zarirhamza zarirhamza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM as well. The go tracer team should review shortly

@zarirhamza
zarirhamza merged commit b9a86a0 into DataDog:main Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] contrib/aws/aws-sdk-go-v2/aws: Setting targetArn instead of topicArn leads to nil pointer dereference

4 participants