feat(nodeadm): use ecr-credential-provider for public.ecr.aws in 1.27+ #2153
Conversation
| { | ||
| "apiVersion": "{{.ConfigApiVersion}}", | ||
| "kind": "CredentialProviderConfig", | ||
| "providers": [ |
There was a problem hiding this comment.
thoughts on just using the go type for CredentialProviderConfig and serializing it to json?
| @@ -74,6 +88,8 @@ func generateImageCredentialProviderConfig(cfg *api.NodeConfig, ecrCredentialPro | |||
| } else { | |||
| templateVars.ConfigApiVersion = "kubelet.config.k8s.io/v1" | |||
| templateVars.ProviderApiVersion = "credentialprovider.kubelet.k8s.io/v1" | |||
| // ecr-credential-provider has support for public.ecr.aws in 1.27+ | |||
| templateVars.MatchImages = append(templateVars.MatchImages, "public.ecr.aws") | |||
There was a problem hiding this comment.
ignore me -- I breezed past this part.
Nick's suggestion to use a go type is probably reasonable given how clunky loops are in a go template. Looks like the versions we need are still available in the latest k/k: k8s.io/kubelet/config/v1alpha1, etc
|
Hi @cartermckinnon, I'm using the following command for the build: During the build process, I receive the following error: |
|
@suhailms18 I don't see how that's related to this PR, please open a new issue. |
mselim00
force-pushed
the
public-ecr-al2023
branch
from
c36507e to
b9ba3e7
Compare
mselim00
force-pushed
the
public-ecr-al2023
branch
from
b9ba3e7 to
f06baef
Compare
mselim00
force-pushed
the
public-ecr-al2023
branch
from
f06baef to
d14a8f9
Compare
|
Have we verified:
|
|
@tzneal yep! @mselim00 tested this in an earlier PR: #1949 (comment) |
mselim00
force-pushed
the
public-ecr-al2023
branch
from
ac449db to
aeb233c
Compare
| "*.dkr.ecr.*.c2s.ic.gov", | ||
| "*.dkr.ecr.*.sc2s.sgov.gov", | ||
| "*.dkr.ecr.*.cloud.adc-e.uk", | ||
| "*.dkr.ecr.*.csp.hci.ic.gov", | ||
| } | ||
| kubeletVersion, err := GetKubeletVersion() |
There was a problem hiding this comment.
this is now available in cfg.Status.KubeletVersion, so you should keep that param in this func
| k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| k8sruntime "k8s.io/apimachinery/pkg/runtime" | ||
| k8sjson "k8s.io/apimachinery/pkg/runtime/serializer/json" | ||
| k8sconfigv1 "k8s.io/kubelet/config/v1" | ||
| k8sconfigv1alpha1 "k8s.io/kubelet/config/v1alpha1" |
There was a problem hiding this comment.
| k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| k8sruntime "k8s.io/apimachinery/pkg/runtime" | |
| k8sjson "k8s.io/apimachinery/pkg/runtime/serializer/json" | |
| k8sconfigv1 "k8s.io/kubelet/config/v1" | |
| k8sconfigv1alpha1 "k8s.io/kubelet/config/v1alpha1" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/runtime" | |
| "k8s.io/apimachinery/pkg/runtime/serializer/json" | |
| configv1 "k8s.io/kubelet/config/v1" | |
| configv1alpha1 "k8s.io/kubelet/config/v1alpha1" |
There was a problem hiding this comment.
You can also remove the force import of embed, I think?
| func generateImageCredentialProviderConfig(ecrCredentialProviderBinPath string) ([]byte, error) { | ||
| scheme := k8sruntime.NewScheme() | ||
| providerName := filepath.Base(ecrCredentialProviderBinPath) | ||
| defaultCacheDuration := &k8smetav1.Duration{Duration: 12 * time.Hour} |
There was a problem hiding this comment.
Should be a package-level constant, I think?
| Name: providerName, | ||
| MatchImages: matchImages, | ||
| DefaultCacheDuration: defaultCacheDuration, | ||
| APIVersion: "credentialprovider.kubelet.k8s.io/v1alpha1", |
There was a problem hiding this comment.
This should be set by the scheme encoder
| Providers: []k8sconfigv1.CredentialProvider{ | ||
| { | ||
| Name: providerName, | ||
| MatchImages: append(matchImages, "public.ecr.aws"), |
There was a problem hiding this comment.
Need a comment here explaining why this is only for v1/1.27+
| gvk, err := apiutil.GVKForObject(cfg, scheme) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cfg.GetObjectKind().SetGroupVersionKind(gvk) |
There was a problem hiding this comment.
this looks like a no-op, what's it for?
| if semver.Compare(kubeletVersion, "v1.27.0") < 0 { | ||
| templateVars.ConfigApiVersion = "kubelet.config.k8s.io/v1alpha1" | ||
| templateVars.ProviderApiVersion = "credentialprovider.kubelet.k8s.io/v1alpha1" | ||
| err = k8sconfigv1alpha1.AddToScheme(scheme) |
There was a problem hiding this comment.
I think you should move the scheme setup stuff into init()
Issue #, if available:
Description of changes:
Ports 78f54f6 to nodeadm/al2023
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Testing Done
Built a 1.30 AMI, checked the config file had
public.ecr.awsin the machImages list and that running a pod using an image from public ECR (public.ecr.aws/nginx/nginx:latest) does not fail when using a role withoutecr-publicpermissions.See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.