Fix ECR Client Migration to V2 #4550
Conversation
TheanLim
changed the title
TheanLim
changed the title
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
8cf4edb to
d672034
Compare
| ) | ||
|
|
||
| const httpsPrefix = "https://" |
There was a problem hiding this comment.
Q: Do we also need to consider http as well?
Looks like in AWS SDK Go V1, it takes that into consideration when appending the prefixes -> https://github.com/aws/aws-sdk-go/blob/8d203ccff393340d080be0417d091cc60354449b/aws/endpoints/endpoints.go#L324-L339
There was a problem hiding this comment.
No, we don't need to consider http.
We currently (in v1) do not instantiate a new client with Config.DisableSSL set to true. This flag is passed to AddScheme to determine whether the HTTP or HTTPS schemes should be added.
In short, we only prepend https prefix, if it's needed.
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
d672034 to
8f62fed
Compare
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
48399a7 to
5fd8b89
Compare
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
5fd8b89 to
621514e
Compare
| if !strings.HasPrefix(endpoint, httpsPrefix) { | ||
| endpoint = httpsPrefix + endpoint | ||
| } | ||
| opts = append(opts, awsconfig.WithBaseEndpoint(endpoint)) |
There was a problem hiding this comment.
BaseEndpoint override in config has a higher precedence than environment variable and shared config file per https://docs.aws.amazon.com/sdkref/latest/guide/settings-reference.html. Verified this by manual testings too.
This means that customer host configurations should not affect endpoint override, which is the v1 behaviour
TheanLim
changed the title
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
621514e to
8c77c09
Compare
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
8c77c09 to
a0dd1e8
Compare
TheanLim
force-pushed
the
fix-ecr-migration
branch
from
a0dd1e8 to
ede603a
Compare
Summary
A fix on PR #4512 by adding HTTPS prefix.
Implementation details
HTTPS/HTTP prefix is automatically added to
endpointin sdk v1 but not in v2.Adding HTTPS prefix to endpoint override if it's not already so.
Testing
New tests cover the changes: no, but adapting the existing test.
Description for the changelog
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
Does this PR include the addition of new environment variables in the README?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.