Abstract region discovery #757
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
approved
micahhausler
force-pushed
the
region-discovery
branch
2 times, most recently
from
45fbf23
to
c73ba48
Compare
k8s-ci-robot
added
the
needs-rebase
With the deprecation of AWS SDK Go, and the removal of the endpoints package, we need a new way to discover valid regions. This change preserves the previous behavior, while adding a new method that calls accounts:ListRegions, or looking up endpoints from a file.
micahhausler
force-pushed
the
region-discovery
branch
from
371eddf
to
4caf6ad
Compare
micahhausler
force-pushed
the
region-discovery
branch
from
4caf6ad
to
b2bba49
Compare
|
@micahhausler: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
k8s-ci-robot
removed
the
needs-rebase
|
|
||
| awscfg, err := aws_config.LoadDefaultConfig(context.TODO()) | ||
| if err != nil { | ||
| logrus.WithError(err).Fatal("unable to create AWS config") |
There was a problem hiding this comment.
nit: should we return an error instead of calling Fatal? Seems like getConfig is expected to propagate errors
| for { | ||
| select { | ||
| case <-stopCh: | ||
| logrus.Info("shut down mapper before return from Run") | ||
| close(c.internalHandler.backendMapper.mapperStopCh) | ||
| close(s.internalHandler.backendMapper.mapperStopCh) | ||
| c.endpointVerifier.Stop() |
There was a problem hiding this comment.
nit: should this be
| c.endpointVerifier.Stop() | |
| s.endpointVerifier.Stop() |
?
| // API makes the server invoke the AWS API call `account:ListRegions` to find a list of valid regions. | ||
| // | ||
| // File has the server read a file containing a JSON list of strings with valid STS endpoints. | ||
| EndpointValidationMode string |
There was a problem hiding this comment.
nit: Would it be worthwhile to define this using an enum/const generator, instead of string?
eg:
type ValidationMode int64
const (
Legacy ValidationMode = iota
API
File
)
It would save us from requiring defensive checks in the switch/case statements, but I also understand if the overhead of dealing with enums in Go isn't worth the hassle.
| return parsedURL.Hostname(), nil | ||
| } | ||
|
|
||
| func stsHostsForPartition(partitionID, region string) map[string]bool { |
There was a problem hiding this comment.
Nice job refactoring this legacy logic into regions.sdkV1Discoverer 💯
| { | ||
| name: "InvalidJSON", | ||
| fs: func() fs.FS { | ||
| tmpfs := afero.NewMemMapFs() |
There was a problem hiding this comment.
I noticed afero in the last PR too - this looks pretty useful!
| var tlds []string | ||
| serviceNames := []string{"sts", "sts-fips"} | ||
| switch d.partition { | ||
| case "aws": |
There was a problem hiding this comment.
Similar to my enums question above: is it worthwhile to use enums here?
What this PR does / why we need it:
With the deprecation of AWS SDK Go v1, and the removal of the endpoints package, we need a new way to discover valid regions. This change preserves the previous behavior, while adding a new method that calls accounts:ListRegions, or looking up endpoints from a file.
See #736 (comment)
Which issue(s) this PR fixes
Further progress toward #736