aws-samples / cdk-eks-karpenter

CDK construct for installing and configuring Karpenter on EKS clusters
Apache License 2.0
34 stars 14 forks source link

Add prop for setting service account name #136

Closed plumdog closed 8 months ago

plumdog commented 9 months ago

This is unset so defaults to the id of the resource, per https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_eks.ServiceAccount.html#name

This can get very long (I'm using CDK pipelines) and Kubernetes caps the length of the label at 63 characters.

Should add an option to allow me to set this.

I'm investigating whether I can handle this with CDK escape hatches.

plumdog commented 9 months ago

My disgraceful but effective workaround:

const orig = myCluster.addServiceAccount.bind(myCluster);
const patched = (id: string, props: eks.ServiceAccountProps) => {
    const patchedProps = id === 'karpenter' ? { ...props, name: 'karpenter' } : props;
    return orig(id, patchedProps);
};
// @ts-ignore
myCluster.addServiceAccount = patched;

This allows me to make https://github.com/aws-samples/cdk-eks-karpenter/blob/c4179c0274cb2f76a51486438a2d667b88861437/src/index.ts#L112-L114 do what I want, which is actually:

this.serviceAccount = this.cluster.addServiceAccount('karpenter', {
    namespace: this.namespace,
    name: 'karpenter',
});
andskli commented 8 months ago

@plumdog thanks for the report! Would it make sense to just default the serivceAccountName to karpenter and provide an option to the construct to specify your own name? In hindsight I don't really see the benefits of using the CDK generated name and can't really see how changing this behavior could break existing deployments either; the name of the serviceAccount would change but so would the reference to it in the Helm chart values.

plumdog commented 8 months ago

@andskli Yes, I think just defaulting to karpenter would be perfectly sensible.

And I agree it makes sense to treat that as an implementation detail, rather than a breaking change.