fix: made ECR type checker flexible to accept the server w/o https #111
Conversation
| typ, _ = awsConfig.GetEcrType("google.com") | ||
| if typ != docker.Invalid { | ||
| msg := fmt.Sprintf("ECR type should be %v", docker.Invalid) | ||
| if typ != docker.Private { |
There was a problem hiding this comment.
I know the current code behaves like this now that we forcefully introduce a schema, and I'd imagine before the current change, if one specified https://google.com this would end-up in the docker.Private state, so this is probably out-of-scope for this PR, but is there any heuristic we can use to weed out easy invalids like this one?
Naively I'd think something like checking if the host is not the the shape of \.dkr\.ecr\.[^.]+\.amazonaws\.com we know it's not a private ECR URL?
Maybe I'm overthinking this, but I'd for sure prefer we reject more explicitely when the URI's not an ECR one, AFAICT there's basically two shapes for those so we can pinpoint them more accurately imho.
There was a problem hiding this comment.
Actually, before we introduced this code, we did not have any checks. We used to rely upon the AWS SDK. My thought process is to filter out ECR Public/Private URLs if we can otherwise let the AWS SDK handle the error raising as it used to be. And one more thing is that this part of the code is executed only for the AWS, so this is a minor edge case.
There was a problem hiding this comment.
I'm in the same light that we can probably do a bit more than force the HTTPS schema. It looks to me that the check for the protocol is not necessary at all, and that we should support URLs both with and without the schema. Is my observation correct?
That said, it looks like you are using the GetEcrType to determine if an authorization token should be obtained for the Public ECR repository or for a Private repository. The difference being just the region. Is that correct?
If so I think we can probably have logic that says if login server host is public.ecr.aws then we should get a PublicAuthorization token, for everything else we should treat as private. That said it is not clear to me if a user can CNAME the public gallery URL to something custom to the user, which in that case could be an issue for the auto detection. If CNAMing is an option should there be a configuration argument public_ecr_gallery (bool) that if true would get a Public authorization token?
Lastly for the docker login command it looks like it does not matter if you pass it a login server with or without a protocol. So this should not be an issue unless I'm mistaken on the supported values.
There was a problem hiding this comment.
Maybe I'm overthinking this, but I'd for sure prefer we reject more explicitely when the URI's not an ECR one, AFAICT there's basically two shapes for those so we can pinpoint them more accurately imho.
I don't you are over thinking here. Generally speaking we should try to validate for these values when possible. I don't know if it is as simple as checking for AWS owned domain if CNAMing is possible. Which is probably why the original code relied on the API to fail. This is not me saying we shouldn't it is more like we need to do research to make sure it does not run into the same issues as checking for a schema on the URL.
- Add an optional flag to identify whether to force push image to ECR Public - Rectify logic to identify if the image should be pushed Public/Private - Simplify code by using the new flag to authenticate with AWS - Add test cases
devashish-patel
requested review from
lbajolet-hashicorp and
nywilken
and removed request for
nywilken
builder/docker/ecr_login.go
Outdated
| // ECR. If the user sets this to `true` from the config, we will forcefully | ||
| // try to push to Public ECR otherwise set this from code based on the | ||
| // given LoginServer value | ||
| PublicEcrGallery bool `mapstructure:"public_ecr_gallery" required:"false"` |
There was a problem hiding this comment.
I'd be maindful of the technical refs here since this comment is the effective user-facing documentation for the flag. I'd recommend we get more to-the-point with this, and leave out the refs to LoginServer here
There was a problem hiding this comment.
OK, now that I looked at the implementation, I think I misunderstood this flag's use. I assumed that we removed all kinds of automatic detection with the introduction of this flag, but this is not the case, this flag bypasses the check.
Maybe we should rename it to aws_force_use_public_ecr? That'd make the intent clearer I think.
If we don't go the force route, for consistency I'd recommend prefixing the option name with aws_ still, it'd make it clear this setting is AWS-specific.
| @@ -38,39 +37,30 @@ type AwsAccessConfig struct { | |||
| // communicate with AWS. Learn how to set | |||
There was a problem hiding this comment.
Out-of-scope but this caught my attention: the Learn how to set this. feels a bit brutal when we read the docs as it points to nothing, we should probably either remove that sentence, or make it point to a learning guide
| "login_server": "https://public.ecr.aws/j9y7g6y8/dev_hc_pkr_dkr_test_1" | ||
| } | ||
| ] | ||
| ] |
There was a problem hiding this comment.
The indentation looks off here, is it automatically formatted by the renderer? Otherwise I'd recommend fixing this, it makes it hard to read.
Resolves #110