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

cfg.EndpointResolverWithOptions is ignored when cfg.EndpointResolver is nil #1513

Closed
3 tasks done
tiedotguy opened this issue Dec 1, 2021 · 3 comments · Fixed by #1514
Closed
3 tasks done

cfg.EndpointResolverWithOptions is ignored when cfg.EndpointResolver is nil #1513

tiedotguy opened this issue Dec 1, 2021 · 3 comments · Fixed by #1514
Labels
bug This issue is a bug.

Comments

@tiedotguy
Copy link

tiedotguy commented Dec 1, 2021

Documentation

Describe the bug

The code generated for api_client.go, generates the following function:

func resolveAWSEndpointResolver(cfg aws.Config, o *Options) {
	if cfg.EndpointResolver == nil {
		return
	}
	o.EndpointResolver = withEndpointResolver(cfg.EndpointResolver, cfg.EndpointResolverWithOptions, NewDefaultEndpointResolver())
}

If the caller is using cfg.EndpointResolverWithOptions without cfg.EndpointResolver (which is expected, as cfg.EndpointResolver is deprecated), then cfg.EndpointResolverWithOptions isn't used.

Appears to be introduced in #1274

Expected behavior

There's 4 options that can occur:

  • Resolver absent / ResolverWithOptions absent - resolved using default handler
  • Resolver present / ResolverWithOptions absent - resolved using Resolver, fallback to default handler
  • Resolver absent / ResolverWithOptions present - resolved using ResolverWithOptions, fallback to default handler
  • Resolver present / ResolverWithOptions present - I think resolved using ResolverWithOptions, fallback to Resolver, fallback to default handler, but it's kind of a garbage in, garbage out situation.

Current behavior

  • Resolver absent / ResolverWithOptions absent - resolved using default handler 🆗
  • Resolver present / ResolverWithOptions absent - resolved using Resolver, fallback to default handler 🆗
  • Resolver absent / ResolverWithOptions present - resolved using default handler
  • Resolver present / ResolverWithOptions present - resolved using ResolverWithOptions, fallback to default handler ❔

Steps to Reproduce

Note, this uses s3, but any service will do as service/*/api_client.go is generated

package main

import (
	"context"
	"fmt"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/service/s3"
)

func main() {
	lbi := &s3.ListBucketsInput{}

	cfgBothResolvers := aws.Config{
		EndpointResolver: aws.EndpointResolverFunc(func(service, region string) (aws.Endpoint, error) {
			fmt.Printf("executing EndpointResolver\n")
			return aws.Endpoint{}, &aws.EndpointNotFoundError{}

		}),
		EndpointResolverWithOptions: aws.EndpointResolverWithOptionsFunc(func(service, region string, options ...interface{}) (aws.Endpoint, error) {
			fmt.Printf("executing EndpointResolverWithOptions\n")
			return aws.Endpoint{}, &aws.EndpointNotFoundError{}
		}),
	}

	cfgWithoutEndpointResolver := cfgBothResolvers.Copy()
	cfgWithoutEndpointResolver.EndpointResolver = nil

	cfgWithoutEndpointResolverWithOptions := cfgBothResolvers.Copy()
	cfgWithoutEndpointResolverWithOptions.EndpointResolverWithOptions = nil

	fmt.Printf("Resolving with both, expecting EndpointResolverWithOptions and EndpointResolver to be executed (maybe?)\n")
	s3.NewFromConfig(cfgBothResolvers).ListBuckets(context.Background(), lbi)
	fmt.Printf("------\n")

	fmt.Printf("Resolving with just EndpointResolver, expecting EndpointResolverWithOptions to be executed\n")
	s3.NewFromConfig(cfgWithoutEndpointResolverWithOptions).ListBuckets(context.Background(), lbi)
	fmt.Printf("------\n")

	fmt.Printf("Resolving with just EndpointResolverWithOptions, expecting EndpointResolverWithOptions to be executed\n")
	s3.NewFromConfig(cfgWithoutEndpointResolver).ListBuckets(context.Background(), lbi)
}

Possible Solution

For scenario 3, it should invoke ResolverWithOptions, for scenario 4 I am unsure.

Bug is around

private void writeAwsConfigEndpointResolver(GoWriter writer) {
writer.openBlock("func $L(cfg aws.Config, o *Options) {", "}", RESOLVE_AWS_CONFIG_ENDPOINT_RESOLVER, () -> {
writer.openBlock("if cfg.$L == nil {", "}", ENDPOINT_RESOLVER_CONFIG_NAME, () -> writer.write("return"));
writer.write("o.$L = $L(cfg.$L, cfg.$L, $L())", ENDPOINT_RESOLVER_CONFIG_NAME,
EndpointGenerator.AWS_ENDPOINT_RESOLVER_HELPER, ENDPOINT_RESOLVER_CONFIG_NAME,
AWS_ENDPOINT_RESOLVER_WITH_OPTIONS,
EndpointGenerator.RESOLVER_CONSTRUCTOR_NAME);
});
}

Semi-related, while checking the docs to see if scenario 4 is documented, I noted that they weren't fully updated (https://pkg.go.dev/github.com/aws/[email protected]/aws#EndpointResolverWithOptions)

EndpointResolverWithOptions is an endpoint resolver that can be used to provide or override an endpoint for the given service, region, and the service clients EndpointOptions. API clients will attempt to use the EndpointResolver first to resolve an endpoint if available. If the EndpointResolver returns an EndpointNotFoundError error, API clients will fallback to attempting to resolve the endpoint using its internal default endpoint resolver.

AWS Go SDK version used

v1.11.1

Compiler and Version used

1.17.1

Operating System and version

Linux, Windows

@tiedotguy tiedotguy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 1, 2021
@skmcgrail
Copy link
Member

Thank you for reporting this issue @tiedotguy and providing a detailed write-up. I agree with the assessment of root cause of this issue is that this check was missed during the update:

if cfg.EndpointResolver == nil {
    return
}

This should likely be:

if cfg.EndpointResolver == nil && cfg.EndpointResolverWithOptions == nil {
    return
}

The intention of the original short-circuit evaluation when it was written was to avoid allocation of the wrappedEndpointResolver type, as there is nothing to be wrapped, and the behavior is to just use the default EndpointResolver implementation for the service. With that said I believe updating this check will address the issue, as the call to withEndpointResolver is what handles constructing the wrapper to either use cfg.EndpointResolverWithOptions or cfg.EndpointResolver with the appropriate precedence and fallback.

@KaibaLopez KaibaLopez removed the needs-triage This issue or PR still needs to be triaged. label Dec 1, 2021
@skmcgrail
Copy link
Member

skmcgrail commented Dec 1, 2021

Using your example, I get the following output behavior now with the resolvers that we expect to be executed.

Resolving with both, expecting EndpointResolverWithOptions and EndpointResolver to be executed (maybe?)
executing EndpointResolverWithOptions
------
Resolving with just EndpointResolver, expecting EndpointResolverWithOptions to be executed
executing EndpointResolver
------
Resolving with just EndpointResolverWithOptions, expecting EndpointResolverWithOptions to be executed
executing EndpointResolverWithOptions

Regarding scenario #4, since the intention is to deprecate to the aws.EndpointResolver in favor of aws.EndpointResolverWithOptions and have users migrate usage to that type, the intent was that by explicitly opting into using aws.EndpointResolverWithOptions we will call that resolver and then just fallback to the default resolver for the service. Creating a chaining hierarchy was not an intended goal. But this is a great call out that is not clear in the documentation. We will get that updated as well.

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants