Move NewSession to manager.go to allow endpoint-url to use EndpointResolver

[ryansteakley]

Is your feature request related to a problem?

• Current implementation of endpointUrl configures the awscfg before the call to session.NewSession().
• If you are using oidc, internally NewSession will call stsAssumeRoleWithWebIdentity or if you are using cross account the call to stsAssumeRole will also occur.
• The issue lies in the fact that if one is using a custom endpoint for example to the sagemaker service, when stsAssumeRole is called it will throw a 404 as we can no longer reach that service.
• This occurs because the awscfg.Endpoint has been set to the sagemaker endpoint so now we only allow traffic to sagemaker services. As a a result of this the current implementation of endpointUrl does not work when the user is using either crossAccount or OIDC roles.

Describe the solution you'd like

• To use the endpointResolver function as provided by the aws go sdk. Here we can specify when the service == sagemaker.EndpointsID to use our custom sagemaker endpoint.

• As a result we are only using this endpoint for sagemaker api calls and we still have access to the stsAssumeRole calls.

• To allow for a better user experience instead of having them provide the service-id.EndpointsID we would move this to each resources Manager.go file to create the session and populate this check with the respective service-id and its respective EndpointsID.

• To make this change the aws_resource_manager.go struct for an AWSResourceManager needs to accept endpointUrl as well as the gvk and the service controllers VersionInfo so that we can make the injectUserAgent call.

• Then in the manager.go we will create the session and utilize the EndpointResolver func so that EndpointUrl can work when using OIDC and cross account.

• If the community thinks this is a good solution, I will implement it and get it out for code review

Describe alternatives you've considered
A description of any alternative solutions or features you've considered.

• Have tested all three of these alternatives and they work to resolve this issue however none of them seem to be the best way to resolve things.

1. Make user manually find the endpointsID and pass it in as another variable to the config..... Trying to avoid this with the solution I described above

2. Call session.NewSession() again after the initial call and grabbing the credentials from the returned session object and then calling session.NewSession() with those credentials so we don't have to call stsAssumeRole.

3. Set awscfg.Endpoint at the end of newSession instead of at the beginning and Return awscfg and pass it to the manager.go and call newResourceManager with the awscfg as an additional parameter which will now include the awscfg.Endpoint and will now use this endpoint when creating the service client.

[RedbackThomson]

I don't believe we need to move all of this logic into the manager, I think most of it can remain as common runtime code. We are already passing a number of configuration items in to newSession from the manager object. I was going to suggest we simply pass it the service ID and re-use that variable, but I found that endpoint IDs don't always match service IDs.

I just did a little test, and I think it would be totally feasible to pass the endpoint service ID as a parameter in the NewSession method, then optionally apply the endpoint resolver if the URL was specified. See my scratch code:

func (c *serviceController) NewSession(
	region ackv1alpha1.AWSRegion,
	endpointURL *string,
	endpointServiceID *string,
	assumeRoleARN ackv1alpha1.AWSResourceName,
	groupVersionKind schema.GroupVersionKind,
) (*session.Session, error) {
	awsCfg := aws.Config{
		Region:              aws.String(string(region)),
		STSRegionalEndpoint: endpoints.RegionalSTSEndpoint,
	}

	if endpointURL != nil {
		endpointServiceResolver := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) {
			if service == *endpointServiceID {
				return endpoints.ResolvedEndpoint{
					URL: *endpointURL,
					SigningRegion: region,
				}, nil
			}
			return endpoints.DefaultResolver().EndpointFor(service, region, optFns...)
		}
		awsCfg.EndpointResolver = endpoints.ResolverFunc(endpointServiceResolver)
	}

	sess, err := session.NewSession(&awsCfg)

Then when we call NewSession as part of the reconciler, we could fetch this value from a common service directory:

	acctID := r.getOwnerAccountID(desired)
	region := r.getRegion(desired)
	roleARN := r.getRoleARN(acctID)
	endpointURL := r.getEndpointURL(desired)
	endpointServiceID := r.getEndpointServiceID()
	gvk := desired.RuntimeObject().GetObjectKind().GroupVersionKind()
	sess, err := r.sc.NewSession(region, &endpointURL, &endpointServiceID, roleARN, gvk)

We don't really have anywhere to store service-common properties. We only really only have per-resource properties. But I think there is value in placing them in a common path in the service pkg directory. How these service IDs are stored there? Probably changes to code-generator to load them from the generator.yaml, for instance.

[jaypipes]

Just pointing out that I suggested using ResolverFunc back in the original PR that added endpoint support.

[RedbackThomson]

Just pointing out that I suggested using ResolverFunc back in the original PR that added endpoint support.

As was the right call, because I believe the implementation that's been in the runtime since then has actually never worked as expected.