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.

Sign up for GitHub

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

Jump to bottom

use sts regional endpoint for aws session #283

Closed
wants to merge 2 commits into from
Closed

use sts regional endpoint for aws session #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fc72be
use sts regional endpoint for aws session
jyotimahapatra Nov 4, 2021
eae393f
use sts regional endpoint for aws session
jyotimahapatra Nov 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion pkg/providers/v1/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"io"
"net"
"net/http"
"os"
"path"
"regexp"
"sort"
Expand Down Expand Up @@ -287,6 +288,10 @@ const (

// privateDNSNamePrefix is the prefix added to ENI Private DNS Name.
privateDNSNamePrefix = "ip-"

// Environment Variable to create sts session with regional endpoint
// If unset or false, uses global sts endpoint
useRegionalSts = "USE_REGIONAL_STS"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the switch should be the opposite because 1. we want everyone to use regional STS endpoints and 2. we are still publishing alpha containers, and we can document in the upgrade notes that if you have STS regional endpoints disabled in your AWS account, you need to change this setting. Also, I think it should be a flag instead of an environment variable (and/or a cloud config option). We can do something more complicated, like a feature gate of sorts that initially is opt-in but when it moves to beta becomes opt-out (I.e. the feature gate is called "UseRegionalSTS" and is false in alpha, but true in beta, and eventually is removed and cannot be opted out of). Whatever we end up with though, we should have users opt-out of regional endpoints, rather than opt-in to them, because STS wants to move traffic off the global endpoint.

Copy link
Author

@jyotimahapatra jyotimahapatra Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i generally agree, but a few things i wanted to discuss about how we make the changes that could affect users.
Since sts has an ability to disable regional endpoints, shifting the default to regional could be a breaking change. I propose that we keep the original behavior intact and deprecate over a release. In the next release of ccm, we could shift the default to regional and put a release note(included in PR description)

From my investigation so far, using a flag is difficult because of the how the cloud provider is instatiated. The kube controller passes no configurations while initializing the cloud provider. Looking at the signature cloudprovider.RegisterCloudProvider(ProviderName, func(config io.Reader) (cloudprovider.Interface, error) { there is no other config apart from CloudConfig file reader. It would take a refactoring in k/k to pass additional flagset. Do you find this explanation correct? If so we'll have to use an env var until flagset can be passed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flag can be added to https://github.com/kubernetes/cloud-provider-aws/blame/master/cmd/aws-cloud-controller-manager/main.go no problem

)

const (
Expand Down Expand Up @@ -1208,8 +1213,12 @@ func init() {
return nil, fmt.Errorf("unable to validate custom endpoint overrides: %v", err)
}

awsSessionConfig, err := getSessionConfig(&awsSDKProvider{cfg: cfg})
if err != nil {
return nil, fmt.Errorf("unable to create aws session: %v", err)
}
sess, err := session.NewSessionWithOptions(session.Options{
Config: aws.Config{},
Config: *awsSessionConfig,
SharedConfigState: session.SharedConfigEnable,
})
if err != nil {
Expand Down Expand Up @@ -5319,3 +5328,26 @@ func (c *Cloud) describeNetworkInterfaces(nodeName string) (*ec2.NetworkInterfac
}
return eni.NetworkInterfaces[0], nil
}

func getSessionConfig(awsServices Services) (*aws.Config, error) {
metadata, err := awsServices.Metadata()
if err != nil {
return nil, fmt.Errorf("error creating AWS metadata client: %q", err)
}
az, err := getAvailabilityZone(metadata)
if err != nil {
return nil, fmt.Errorf("error creating retrieving AZ from metadata: %q", err)
}
region, err := azToRegion(az)
if err != nil {
return nil, fmt.Errorf("error getting region from AZ: %q", err)
}

useRegionalSts, _ := strconv.ParseBool(os.Getenv(useRegionalSts))
awsSessionConfig := &aws.Config{}
if useRegionalSts {
klog.Infof("Using Regional STS")
awsSessionConfig = aws.NewConfig().WithRegion(region).WithSTSRegionalEndpoint(endpoints.RegionalSTSEndpoint)
}
return awsSessionConfig, nil
}
38 changes: 38 additions & 0 deletions pkg/providers/v1/aws_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ import (
"fmt"
"io"
"math/rand"
"os"
"reflect"
"sort"
"strings"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
"github.com/aws/aws-sdk-go/service/elb"
Expand Down Expand Up @@ -3844,3 +3846,39 @@ func TestDescribeInstances(t *testing.T) {
})
}
}

func TestRegionalSession(t *testing.T) {
tests := []struct {
name string
useRegionalSts string
expectRegion string
expectEndpoint endpoints.STSRegionalEndpoint
}{
{
name: "sts regional endpoint is set",
useRegionalSts: "true",
expectRegion: "us-east-1",
expectEndpoint: endpoints.RegionalSTSEndpoint,
},
{
name: "sts regional endpoint is unset",
useRegionalSts: "",
expectRegion: "",
expectEndpoint: endpoints.UnsetSTSEndpoint,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
os.Setenv(useRegionalSts, test.useRegionalSts)
defer os.Setenv(useRegionalSts, "")
session, err := getSessionConfig(newMockedFakeAWSServices(TestClusterID))
assert.NoError(t, err)
gotRegion := ""
if session.Region != nil {
gotRegion = *session.Region
}
assert.Equal(t, test.expectRegion, gotRegion)
assert.Equal(t, test.expectEndpoint, session.STSRegionalEndpoint)
})
}
}