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

Add AWS Custom Endpoint capability #70588 #72245

Merged
merged 11 commits into from Jan 29, 2019

Conversation

@ampsingram
Copy link
Contributor

ampsingram commented Dec 20, 2018

Solves "Allow to override default AWS endpoint #70588"

Add several new properties to AWS CloudConfig to support custom endpoints.
Initialize/Parse on aws.go init() method which gets called when aws is loaded.
Allows overridden endpoints per servce and region. This allows functionality on air gapped networks.
This change is benign if services are not overridden in CloudConfig

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #70588

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Adds configuration for AWS endpoint fine control:
OverrideEndpoints bool Set to true to allow custom endpoints
ServiceDelimiter string Delimiter to use to separate overridden services (multiple services) Defaults to "&"
ServicenameDelimiter string Delimiter to use to separate servicename from its configuration parameters Defaults "|"
OverrideSeparator string Delimiter to use to separate region of occurrence, url and signing region for each override Defaults to ","
ServiceOverrides string example: s3|region1, https://s3.foo.bar, some signing_region & ec2|region2, https://ec2.foo.bar, signing_region

Add AWS Custom Endpoint capability #70588
Solves "Allow to override default AWS endpoint #70588"

Add several new properties to AWS CloudConfig to support custom endpoints.
Initialize/Parse on aws.go init() method which gets called when aws is loaded.
Allows overridden endpoints per servce and region. This allows functionality on air gapped networks.
This change is benign if services are not overridden in CloudConfig
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 20, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 20, 2018

Hi @ampsingram. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Dec 20, 2018

Signed up Ampsight Inc as a corporate contributor, accepted the CLA

@ingvagabund ingvagabund self-assigned this Dec 20, 2018

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Dec 20, 2018

I signed it

@ingvagabund
Copy link
Contributor

ingvagabund left a comment

Thank you for the PR. In overall, it looks good. I am just worried about the way of representing overriden services in s3|region1, https://s3.foo.bar, sregion & s3|region2, https://s3.foo.bar, sregion form. The line can get pretty long with many endpoints. Which makes it hard to read and configure.

Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws_test.go
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
@ingvagabund

This comment has been minimized.

Copy link
Contributor

ingvagabund commented Dec 24, 2018

gcfg allows to use multi-value variables. See https://godoc.org/gopkg.in/gcfg.v1#ReadStringInto, part Example (Multivalue):

[global]
...
ServiceOverride=s3, us-east-1, https://s3.foo.bar, sregion
ServiceOverride=ec2, us-weat-1, https://s3.foo.bar, sregion
...
ServiceOverride=kms, us-east-2, https://s3.foo.bar, sregion

Reducing to only one delimiter, where the first component of each override 4-tuple needs to be service name, then region, then endpoint, then signing region. Don't think it's necessary to have name on different level than the remaining regions and endpoint. The (name, region, endpoint, sregion) tuple is easy to read as well.

ampsingram added some commits Jan 16, 2019

Improvement suggested by PR comment
Fail early makes the code more readable
Fail early, helps readability
responding to a comment in the PR
@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 17, 2019

/check-cla

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 17, 2019

I have 2 suggestions:

  1. Instead of doing a bunch of parsing of an individual line, why not take advantage of gcfg sections? You could do something like this:
package main

import (
	"fmt"

	"gopkg.in/gcfg.v1"
)

func main() {
	cfgStr := `
[Global]
vpc = vpc-abc1234567

[serviceoverride "s3"]
Region= us-west-2
URL = https://s3.foo.bar
SigningRegion = custom-signing-region

[serviceoverride "ec2"]
Region= us-west-2
URL = https://ec2.foo.bar
SigningRegion = custom-signing-region
`
        // change the CloudConfig type to have the "ServiceOverride" map
	cfg := struct {
		Global struct {
			VPC string
			// ... other fields
		}
		ServiceOverride map[string]*struct {
			Region        string
			URL           string
			SigningRegion string
		}
	}{}

	err := gcfg.ReadStringInto(&cfg, cfgStr)
	if err != nil {
		panic(fmt.Errorf("Failed to parse gcfg data: %s", err))
	}
	fmt.Printf("Global: %+v\n", cfg.Global)
	for service, override := range cfg.ServiceOverride {
		fmt.Printf("ServiceOverride '%s': %+v\n", service, override)
	}

	fmt.Printf("ServiceOverride for s3: %+v\n", cfg.ServiceOverride["s3"]) // key lookup is case sensitive
}

The output of this would look like:

$ go run main.go 
Global: {VPC:vpc-abc1234567}
ServiceOverride 's3': &{Region:us-west-2 URL:https://s3.foo.bar SigningRegion:custom-signing-region}
ServiceOverride 'ec2': &{Region:us-west-2 URL:https://ec2.foo.bar SigningRegion:custom-signing-region}
ServiceOverride for s3: &{Region:us-west-2 URL:https://s3.foo.bar SigningRegion:custom-signing-region}

If all the fields you need are not present, spit out a warning and ignore the service override.

  1. Update service the methods on the Services interface to accept a CloudConfig field. This way newAWSCloud() can pass down the CloudConfig it gets into the service constructors. (You'll have to update some tests to make this work, but it should be pretty simple.

Once you do those two things, you can do away with most of the parsing and loading code, and remove the global variables and new types.

@justinsb @mcrute @nckturner Any thoughts on modifying the CloudConfig and Services interface methods in that way?

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 17, 2019

#72245 (comment)

But the map is signaled by the combined service name and region. Your example collides on s3|us-west-2 ; It is not a horrible idea, it has great merit, but that is a weakness that must be overcome.

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 17, 2019

But when does cloud controller talk to more than one region for a given service? Can you think of a case where Kubernetes would actually need to call a service (current possibilities are just EC2, ELB, ELBV2, KMS, and ASG) in more than one region?

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 17, 2019

absolutely. S3 buckets can be in different regions. Other services, not so much, but still conceivable.

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 17, 2019

Sure, but cloudprovider does not interact with S3, and as far as I’m aware, there are no plans to do so.

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 18, 2019

in the interest of maximum flexibility, I would prefer to leave the services specified by both service name and region. I see no real downside to this and I think it provides better adherence to AWS's own rule that a service is specified by both a name and a region.

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 18, 2019

I did think of an example for multi-region callout. Kubelet calls ECR for different regions (depending on where an image is located).

Ok, instead of keying off the section title, why not do this?

package main

import (
	"fmt"

	"github.com/aws/aws-sdk-go/aws/endpoints"
	"gopkg.in/gcfg.v1"
)

type CloudConfig struct {
	Global struct {
		VPC string
		// ... other fields
	}
	ServiceOverride map[string]*struct {
		Region        string
		URL           string
		SigningRegion string
		Service       string
	}
}

func (c *CloudConfig) getResolver() endpoints.ResolverFunc {
	return func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) {
		for _, override := range c.ServiceOverride {
			if service == override.Service && region == override.Region {
                                 // TODO: iterate over optFns before returning
				return endpoints.ResolvedEndpoint{
					URL:           override.URL,
					SigningRegion: override.SigningRegion,
				}, nil
			}
		}

		return endpoints.DefaultResolver().EndpointFor(service, region, optFns...)
	}
}

func main() {
	cfgStr := `
[Global]
vpc = vpc-abc1234567

[serviceoverride "ec2_us-west-2"]
URL = https://ec2.foo.bar
SigningRegion = custom-signing-region
Region = us-west-2
Service = ec2

[serviceoverride "ecr_us-west-2"]
URL = https://ecr.foo.bar
SigningRegion = custom-signing-region
Region = us-west-2
Service = ecr

[serviceoverride "ecr_us-east-1"]
URL = https://ecr.bar.foo
SigningRegion = custom-signing-region
Region = us-east-1
Service = ecr
`
	// change the CloudConfig type to have the "ServiceOverride" map
	cfg := &CloudConfig{}
	err := gcfg.ReadStringInto(cfg, cfgStr)
	if err != nil {
		panic(fmt.Errorf("Failed to parse gcfg data: %s", err))
	}

	cases := []struct {
		service string
		region  string
	}{
		{"ec2", "us-east-1"}, // not in config above
		{"ec2", "us-west-2"},
		{"ecr", "us-west-2"},
		{"ecr", "us-west-2"},
	}

	resolver := cfg.getResolver()

	for _, c := range cases {
		ep, err := resolver.EndpointFor(c.service, c.region)
		fmt.Printf("EndpointFor(%s, %s) got %+v, %+v\n", c.service, c.region, ep, err)
	}
}

which would output:

EndpointFor(ec2, us-east-1) got {URL:https://ec2.us-east-1.amazonaws.com SigningRegion:us-east-1 SigningName:ec2 SigningNameDerived:true SigningMethod:v4}, <nil>
EndpointFor(ec2, us-west-2) got {URL:https://ec2.foo.bar SigningRegion:custom-signing-region SigningName: SigningNameDerived:false SigningMethod:}, <nil>
EndpointFor(ecr, us-west-2) got {URL:https://ecr.foo.bar SigningRegion:custom-signing-region SigningName: SigningNameDerived:false SigningMethod:}, <nil>
EndpointFor(ecr, us-west-2) got {URL:https://ecr.foo.bar SigningRegion:custom-signing-region SigningName: SigningNameDerived:false SigningMethod:}, <nil>

ampsingram added some commits Jan 23, 2019

Change to CloudConfig methods for validation and Resolver fn get
Move to a separate section for service overrides in INI, populate a struct for each override
update-bazel, update-gofmt and verify-spelling run
@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 23, 2019

/ok-to-test

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 24, 2019

/retest

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 24, 2019

/retest

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 24, 2019

/test pull-kubernetes-e2e-kops-aws

@micahhausler
Copy link
Member

micahhausler left a comment

This is looking great. Just a few changes

Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/aws/aws.go Outdated

ampsingram added some commits Jan 25, 2019

Add SigningMethod, make it optional, copy in verbatim
An empty SigningMethod will default properly.
@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 25, 2019

changes made, the one test failure appears to be resource related.

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 25, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 27, 2019

/retest

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 28, 2019

/lgtm

Add SigningName as optional parameter
Makes AWS testing simpler

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 29, 2019

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 29, 2019

/test pull-kubernetes-integration

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 29, 2019

/test pull-kubernetes-e2e-kops-aws

@micahhausler

This comment has been minimized.

Copy link
Member

micahhausler commented Jan 29, 2019

/test pull-kubernetes-e2e-kops-aws
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ampsingram, micahhausler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ampsingram

This comment has been minimized.

Copy link
Contributor Author

ampsingram commented Jan 29, 2019

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 29, 2019

@ampsingram: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 5daa004 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit 26d32a7 into kubernetes:master Jan 29, 2019

17 of 18 checks passed

pull-kubernetes-e2e-kops-aws Job failed.
Details
cla/linuxfoundation ampsingram authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment