service/ecr: waiter comparator type mismatch from ImageScanCompleteWaiter

[saj]

Confirm by changing [ ] to [x] below to ensure that it's a bug:

• I've gone though the API reference
• I've checked AWS Forums and StackOverflow for answers
• I've searched for previous similar issues and didn't find any solution

Describe the bug

ecr.ImageScanCompleteWaiter appears to be unusable; it fails with the following error:

waiter comparator expected string value got types.ScanStatus

I believe this is a bug because the value under comparison will never be of string type.

Version of AWS SDK for Go?

1.0.0

Version of Go (go version)?

go version go1.15.6 darwin/amd64

To Reproduce (observed behavior)

It is difficult to share a complete working example; here is the general gist of what I am trying to do:

import (
	"context"

	"github.com/aws/aws-sdk-go-v2/service/ecr"
	ecrtypes "github.com/aws/aws-sdk-go-v2/service/ecr/types"
)

c := ecr.NewFromConfig(...)
p := ecr.DescribeImageScanFindingsInput{
	RepositoryName: aws.String("some-repo-name"),
	ImageId:        &ecrtypes.ImageIdentifier{ImageTag: aws.String("some-unique-image-tag")},
}
w := ecr.NewImageScanCompleteWaiter(c)
if err := w.Wait(context.TODO(), &p, 10*time.Minute); err != nil {
	// handle error
}

Expected behavior

The Wait() method should eventually return no error.

Additional context

Separately, it seems that in order to use the provided paginator and waiter for this type, it is necessary to make the same request over the network twice: once in order to unblock the waiter, then again in order to begin iterating through the paginated responses. Is this a known limitation, or have I missed something?

[skotambkar]

Thanks for reporting the bug. We will investigate the issue and create a fix.

Could you also clarify what your use case is when using waiter along with paginator? If you have an example of how you would be doing waiters with paginators currently, that would make it easier for us to understand.

[saj]

Sure. We provide our staff with a small command-line utility that can be used to check newly built container images for known vulnerabilities in packaged software. This tool works by uploading the image to AWS ECR and blocking for scan results. The results are printed when they eventually become available. (We have one repository set aside for this purpose; the repository is configured with ScanOnPush and an aggressive lifecycle policy that will remove images after about one day. The copy on ECR is effectively ephemeral.) We were already using AWS ECR for long-term production image storage (and vulnerability reporting), so this little command-line utility felt like a natural extension.

After uploading an image, it can take a moment (maybe a minute or two, though I have not measured) before the scan results become available from ECR. We block here. Previously, this block was implemented using our own interruptible sleep-and-retry implementation. Upon seeing the GA release of aws-sdk-go-v2, I decided to port this trivial program over and familiarise myself with your new API. This was where I attempted to make use of the ecr.ImageScanCompleteWaiter.

Once we know the scan is complete, we then begin iterating through the scan results. Depending on the nature of the image, there may be many findings in the scan -- too many to fit within a single page of results. Usually, we are interested in the most severe findings (say, those of FindingSeverityHigh or more). IIRC, the DescribeImageScanFindings action is not documented to return findings in any specific order, so we must be careful to iterate through all pages and filter/sort locally to be assured that the most severe findings are surfaced to the user. This was where I had intended to make use of the DescribeImageScanFindingsPaginator.

Previously, on v1, our sleep-and-retry implementation handled both waits and page iteration. The response we received from the DescribeImageScanFindings action would first be tested for status == COMPLETE; if that were true, would we then reuse that response to kick off the page walk. There was no need for a redundant request. (This approach is still viable on v2, of course, but I was hoping to make use of the provided waiter/paginator now that they exist.)

---

As a workaround, I have written a simple pass-through retry widget that implements the DescribeImageScanFindingsAPIClient interface:

import (
	"context"
	"errors"
	"fmt"
	"time"

	"github.com/aws/aws-sdk-go-v2/service/ecr"
	ecrtypes "github.com/aws/aws-sdk-go-v2/service/ecr/types"
	smithywaiter "github.com/aws/smithy-go/waiter"
)

const (
	waitMinDelay    = 5 * time.Second
	waitMaxDelay    = 2 * time.Minute
	waitMaxDuration = 10 * time.Minute
)

type imageScanWaiter struct {
	Client *ecr.Client

	terminated bool
}

var _ ecr.DescribeImageScanFindingsAPIClient = (*imageScanWaiter)(nil)

func (w *imageScanWaiter) DescribeImageScanFindings(ctx context.Context, input *ecr.DescribeImageScanFindingsInput, optFns ...func(*ecr.Options)) (*ecr.DescribeImageScanFindingsOutput, error) {
	var (
		out           *ecr.DescribeImageScanFindingsOutput
		describeError error
	)

	rem := waitMaxDuration

	for i := int64(1); ; i++ {
		started := time.Now()

		out, describeError = w.Client.DescribeImageScanFindings(ctx, input, optFns...)
		if w.terminated || describeError != nil {
			break
		}

		if out.ImageScanStatus == nil {
			return nil, errors.New("findings unavailable: did not receive ImageScanStatus")
		}
		if w.isTerminalState(out.ImageScanStatus.Status) {
			w.terminated = true
			break
		}

		rem -= time.Since(started)
		if rem < waitMinDelay || rem < 0 {
			return nil, errors.New("findings unavailable: exceeded max wait time")
		}
		d, err := smithywaiter.ComputeDelay(i, waitMinDelay, waitMaxDelay, rem)
		if err != nil {
			return nil, fmt.Errorf("findings unavailable: failed to compute wait delay: %w", err)
		}
		rem -= d
		if err := sleep(ctx, d); err != nil {
			return nil, fmt.Errorf("findings unavailable: wait interrupted: %w", err)
		}
	}
	return out, describeError
}

func (w *imageScanWaiter) isTerminalState(s ecrtypes.ScanStatus) bool {
	if s == ecrtypes.ScanStatusInProgress {
		return false
	}
	return true
}

func sleep(ctx context.Context, d time.Duration) error {
	if d == 0 {
		return nil
	}
	t := time.NewTimer(d)
	defer t.Stop()
	select {
	case <-ctx.Done():
		return ctx.Err()
	case <-t.C:
	}
	return nil
}

This widget can then be passed to the library paginator:

p := ecr.NewDescribeImageScanFindingsPaginator(&imageScanWaiter{Client: ...}, &ecr.DescribeImageScanFindingsInput{...})
for p.HasMorePages() {
	res, err := p.NextPage(ctx)
	// ...
}

---

Does that help?

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.