[awsv2] Panic when no parent segment is present

[adamantike]

A panic error is raised when trying to retrieve a secret value, without having a parent segment, even with AWS_XRAY_CONTEXT_MISSING set to "LOG_ERROR" or "IGNORE_ERROR".

This is a common use case, when having a Lambda function and needing to retrieve a secret value just once during its init() block. During Initialization, there's no parent segment, so setting AWS_XRAY_CONTEXT_MISSING=LOG_ERROR or AWS_XRAY_CONTEXT_MISSING=IGNORE_ERROR is needed to avoid the Lambda function from failing indefinitely.

Reproducible code example

go.mod

module example.com/test-aws-sdk-xray

go 1.16

require (
	github.com/aws/aws-sdk-go-v2 v1.7.0
	github.com/aws/aws-sdk-go-v2/config v1.4.1
	github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.4.0
	github.com/aws/aws-xray-sdk-go v1.5.1-0.20210701195726-f36e5263a798
)

main.go

package main

import (
	"context"
	"log"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
	"github.com/aws/aws-xray-sdk-go/instrumentation/awsv2"
)

func main() {
	ctx := context.TODO()

	cfg, err := config.LoadDefaultConfig(ctx)
	if err != nil {
		log.Fatal(err)
	}

	// Instrument AWS SDK v2.
	awsv2.AWSV2Instrumentor(&cfg.APIOptions)

	client := secretsmanager.NewFromConfig(cfg)

	resp, err := client.GetSecretValue(
		ctx,
		&secretsmanager.GetSecretValueInput{
			SecretId: aws.String("<SECRET_ARN>"),
		},
	)
	if err != nil {
		log.Fatal(err)
	}

	log.Println("Secret content:", *resp.SecretString)
}

Error message

Specifying AWS_XRAY_CONTEXT_MISSING as LOG_ERROR (unexpected panic)

$ AWS_XRAY_CONTEXT_MISSING=LOG_ERROR go run ./main.go
2021-07-06T18:09:18-03:00 [ERROR] Suppressing AWS X-Ray context missing panic: failed to begin subsegment named 'Secrets Manager': segment cannot be found.
panic: interface conversion: interface {} is nil, not *secretsmanager.GetSecretValueOutput

goroutine 1 [running]:
github.com/aws/aws-sdk-go-v2/service/secretsmanager.(*Client).GetSecretValue(0xc0003d8210, 0xd76dd0, 0xc00019c008, 0xc0001a89d8, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001d5ac0)
        /home/mike/.go/pkg/mod/github.com/aws/aws-sdk-go-v2/service/secretsmanager@v1.4.0/api_op_GetSecretValue.go:43 +0x1f5
main.main()
        /home/mike/dev/test-aws-sdk-xray/main.go:26 +0x29b
exit status 2

Specifying AWS_XRAY_CONTEXT_MISSING as IGNORE_ERROR (unexpected panic)

$ AWS_XRAY_CONTEXT_MISSING=IGNORE_ERROR go run ./main.go
panic: interface conversion: interface {} is nil, not *secretsmanager.GetSecretValueOutput

goroutine 1 [running]:
github.com/aws/aws-sdk-go-v2/service/secretsmanager.(*Client).GetSecretValue(0xc000356210, 0xd76dd0, 0xc00003e118, 0xc00000e9f0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc00009bb00)
        /home/mike/.go/pkg/mod/github.com/aws/aws-sdk-go-v2/service/secretsmanager@v1.4.0/api_op_GetSecretValue.go:43 +0x1f5
main.main()
        /home/mike/dev/test-aws-sdk-xray/main.go:26 +0x29b
exit status 2

Additional context

• Running the code with AWS_XRAY_SDK_DISABLED=TRUE avoids the panic and returns correctly.

• Not specifying AWS_XRAY_CONTEXT_MISSING (or setting it to "RUNTIME_ERROR"), generates an expected panic.

• The following code with the AWS SDK v1, also works as expected, without any issues:

go.mod

module example.com/test-aws-sdk-xray-v1

go 1.16

require (
	github.com/aws/aws-sdk-go v1.17.12
	github.com/aws/aws-xray-sdk-go v1.5.1-0.20210701195726-f36e5263a798
)

main.go

package main

import (
	"context"
	"log"

	"github.com/aws/aws-sdk-go/aws"
	"github.com/aws/aws-sdk-go/aws/session"
	"github.com/aws/aws-sdk-go/service/secretsmanager"
	"github.com/aws/aws-xray-sdk-go/xray"
)

func main() {
	ctx := context.TODO()

	sess := session.Must(session.NewSession())
	secrets := secretsmanager.New(sess)

	// Instrument AWS SDK v1.
	xray.AWS(secrets.Client)

	resp, err := secrets.GetSecretValueWithContext(
		ctx,
		&secretsmanager.GetSecretValueInput{
			SecretId: aws.String("<SECRET_ARN>"),
		},
	)
	if err != nil {
		log.Fatal(err)
	}

	log.Println("Secret content:", *resp.SecretString)
}

[adamantike]

Not familiar enough with this project, but it seems this is fixed after applying the following changes:

diff --git a/instrumentation/awsv2/awsv2.go b/instrumentation/awsv2/awsv2.go
index ffe73e3..9584361 100644
--- a/instrumentation/awsv2/awsv2.go
+++ b/instrumentation/awsv2/awsv2.go
@@ -28,7 +28,7 @@ func initializeMiddlewareAfter(stack *middleware.Stack) error {
 		// Start the subsegment
 		ctx, subseg := xray.BeginSubsegment(ctx, serviceName)
 		if subseg == nil {
-			return
+			return next.HandleInitialize(ctx, in)
 		}
 		subseg.Namespace = "aws"
 		subseg.GetAWS()["region"] = v2Middleware.GetRegion(ctx)
@@ -52,7 +52,11 @@ func deserializeMiddleware(stack *middleware.Stack) error {
 		ctx context.Context, in middleware.DeserializeInput, next middleware.DeserializeHandler) (
 		out middleware.DeserializeOutput, metadata middleware.Metadata, err error) {
 
-		subseg := ctx.Value(awsV2SubsegmentKey{}).(*xray.Segment)
+		subseg, ok := ctx.Value(awsV2SubsegmentKey{}).(*xray.Segment)
+		if !ok {
+			return next.HandleDeserialize(ctx, in)
+		}
+
 		in.Request.(*smithyhttp.Request).Header.Set(xray.TraceIDHeaderKey, subseg.DownstreamHeader().String())
 
 		out, metadata, err = next.HandleDeserialize(ctx, in)

cc @bhautikpip, that built this instrumentation with the AWS SDK v2.

[bhautikpip]

Thanks for this suggestion. I also agree that this would fix the panic issue when parent segment is not being present. As per the current code behavior this code would lead to nil err and result. Hence this line would throw panic. Basically we need to call middlewares even in this case to complete aws sdk v2 request. Would you be able to submit a PR with the above mentioned changes since you proposed these changes? I can definitely submit a fix for this issue if you're not available though :)

[adamantike]

I have filed #316, after finding a way to add some unit tests for completeness. Thank you for the quick feedback!