nabsul / k8s-ecr-login-renew

Renews Docker login credentials for an AWS ECR container registry.
MIT License
205 stars 49 forks source link

Lack of configurable accountId parameter in helm values #65

Closed rayrapetyan closed 3 months ago

rayrapetyan commented 4 months ago

A common scenario for utilizing this tool involves refreshing AWS ECR credentials within a Kubernetes deployment outside of AWS. Presently, the deployment assumes the presence of the ACCOUNT_ID environment variable, as if it were operating within AWS, and relies on its proper configuration. However, this assumption doesn't hold for deployments on other cloud platforms. It would greatly enhance flexibility to allow configuring this variable through Helm values during installation. Of course, "registries" can be used as a workaround.

nabsul commented 4 months ago

Hi Robert, thanks for the note and suggestion. However, unless I'm misunderstanding something: The tool does not assume an AWS K8s (EKS) environment. It was designed specifically to allow non-AWS K8s to integrate with AWS's registry.

I used AWS's naming convention for the environment variables, and I think this is consistent and easy to understand. You do have to provide the AWS_* environment variables to the tool, but I don't think there is any other way to grant the tool access to AWS. You either create a secret yourself with these values, or you can let the Helm chart create them for you. That's already supported.

nabsul commented 4 months ago

If I'm misunderstanding something, please let me know. I'm happy to continue the conversation.

rayrapetyan commented 4 months ago

Thanks Nabeel, I might be missing something, but here is how my resource in defined currently:

resource "helm_release" "k8s_ecr_login_renew" {
  repository       = "https://nabsul.github.io/helm"
  chart            = "k8s-ecr-login-renew"
  name             = "k8s-ecr-login-renew"
  namespace        = "default"
  version          = "v1.0.2"
  cleanup_on_fail  = true
  force_update     = false
  set {
    name  = "awsRegion"
    value = var.aws_ecr_region
  }
  set {
    name  = "awsAccessKeyId"
    value = var.aws_ecr_access_key_id
  }
  set {
    name  = "awsSecretAccessKey"
    value = var.aws_ecr_secret_access_key
  }
  set {
    name = "dockerSecretName"
    value = var.aws_ecr_docker_secret_name
  }
  set {
    name = "registries"
    value = var.aws_ecr_registries
  }
}

As you can see I'm able to pass all values (region, keys etc) on chart install as chart values, except the account id. How to pass account id in the resource defined above?

rayrapetyan commented 4 months ago

@nabsul Another useful feature would be running this job once right after creation. I have to do this manually (and I'm sure everyone has to do this while running cluster init scripts). Let me know if you want a separate issue to be created for this. Thanks.

nabsul commented 4 months ago

As you can see I'm able to pass all values (region, keys etc) on chart install as chart values, except the account id. How to pass account id in the resource defined above?

Ah, I see. Is account id necessary? I have never needed it, and I have used digital ocean k8s cluster with no problem. If it's needed for certain types of auth, then I agree, this is something that can and should be added to the tool.

nabsul commented 4 months ago

@nabsul Another useful feature would be running this job once right after creation. I have to do this manually (and I'm sure everyone has to do this while running cluster init scripts). Let me know if you want a separate issue to be created for this. Thanks.

Let's start a separate thread for this. There's a way to do this with kubectl after the initial installation, and there are instructions for it. If we can add it to helm, I think that would be nice.

rayrapetyan commented 4 months ago

Sure! For AWS ECR, account id and region are two necessary pieces:

{account_id}.dkr.ecr.{region}.amazonaws.com

The only workaround is to specify "registries" explicitly (as I did above).

Will create a separate issue for initial installation.

nabsul commented 4 months ago

Hmmmmm... isn't it simpler to give the list of registries instead of individual accounts and regions? That way you only have to provide one environment variable instead of two. And also, what do you do if you want to authenticate multiple registries (like if you have registries in different regions)?

nabsul commented 4 months ago

Basically what I'm saying is: It doesn't seem to me like AWS_ACCOUNT is needed. For authenticating with the AWS credentials, you only need the key id, secret, and region. After that what really matters is the registry URL, which you can either get the default one, or you can customize it with DOCKER_REGISTRIES.

rayrapetyan commented 4 months ago

I agree that using registries is more flexible and covers all possible scenarios (with a single and multiple registries). But in most majority of cases, only one ECR repo is being used per project. That's why popular implementations, like minikube (https://minikube.sigs.k8s.io/docs/tutorials/configuring_creds_for_aws_ecr/) or aws-ecr-credential (https://artifacthub.io/packages/helm/architectminds/aws-ecr-credential) are using concept of a single ECR repo and generating registry implicitly based on account id and region.

Your explicit registries implementation is totally fine, but I'm slightly confused with this:

AWS_REGION (required): The AWS region where your ECR instance is created.

In case user specifies multiple registries in different regions, how this parameter is applied?

nabsul commented 4 months ago

It's been a while since I did all this, but I believe:

AWS region is the region that the tool will connect to in order to get the docker credentials. By default that will be the registry name it generates for.

So you only need to define the registries parameter if you have multiple registries or if it's in a different region.