-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(eks): don't create extra podIdentity addons #33891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
8029f4c
to
44d88cf
Compare
Signed-off-by: Clay Rosenthal <[email protected]>
44d88cf
to
53b8549
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33891 +/- ##
=======================================
Coverage 82.38% 82.38%
=======================================
Files 120 120
Lines 6955 6955
Branches 1173 1173
=======================================
Hits 5730 5730
Misses 1120 1120
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please address the comments otherwise the changes look good at this point.
(child.node.tryFindChild('Resource') as CfnAddon)?.addonName === 'eks-pod-identity-agent' && | ||
(child.node.tryFindChild('Resource') as CfnAddon)?.clusterName === this.clusterName, | ||
) as Addon[]; | ||
if (existing_addons.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While its against recommendation, there can be multiple pod identity agents running for a given eks cluster for usecases such as testing/migration or multi-tenant clusters. Do we want to throw the error here explicitly or just choose the first one in the list like below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can have multiple pod identity agents running in a cluster. That's part of why I made this logic. If you have multiple it throws an error about already having that addon. See the issue (#32580) this fixes. If you can have multiple, I'd love to see some code to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe as an addon you can only have one instance of it, as the error from issue #32580 states: Error message: "eks-pod-identity-agent already exists in stack"
However I do see other installation methods as explained in the pod identity README, so it may be possible to still install multiple outside of the scope of addons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah to my knowledge, Addons are just preconfigured helm charts, so I could see how you could install multiple instance of the pod identity chart. But that doesn't cause an instant error like multiple Addons does. I'm trying to solve that validation error. If the deployment error from multiple misconfigured pod identity charts occurs, I don't think we could check and prevent that.
child instanceof CfnAddon && child.addonName === 'eks-pod-identity-agent' && | ||
child.clusterName === this.clusterName, | ||
) as CfnAddon[]; | ||
if (existing_cfn_addons.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
A minor point of clarification to point out about the problem. You hit an error about already exist if your try to use both options at the same time. To avoid/workaround the error about duplicate/already exists, I've learned to let the EKS Service Account generate it(Opt2), and left the EKS addon (Opt1) commented out. That workaround comes with a downside that I can't capture the addon version to be installed as part of IaC, as specifying the version of the addon (and extra config/config overrides) is only available in Opt1. |
Yeah @neoakris this PR is trying to fix that so you can use the Addon and Service Accounts. But yeah just giving in and only using service accounts work. Part of the problem is that behaviour isn't well documented anywhere. |
Yeah I understand what this PR is trying to fix. |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
Clarification Request: can you provide working CDK code that uses two Addons or CfnAddons of pod identity agent? I have not been able to bring up a stack like that (part of the reason for this PR) |
@clayrosenthal yeah I have some flexibility atm work wise and I should be able to supply a short self contained example & post it here by the end of the week. |
This might be useful in testing any changes.
My folder was named cdk32580 and the templating generated #!/usr/bin/env node
import * as cdk from 'aws-cdk-lib';
import * as eks from 'aws-cdk-lib/aws-eks';
import * as iam from 'aws-cdk-lib/aws-iam';
import { KubectlV32Layer } from '@aws-cdk/lambda-layer-kubectl-v32'; //npm install @aws-cdk/lambda-layer-kubectl-v32
const app = new cdk.App();
const stack_config: cdk.StackProps = {
env: {
account: process.env.CDK_DEFAULT_ACCOUNT!, //<-- process.env pulls value from CLI env,
region: process.env.CDK_DEFAULT_REGION! //<-- ! after var, tells TS it won't be null.
}
}
const stack: cdk.Stack = new cdk.Stack(app, 'example', stack_config);
//UX convienence similar to EKS Blueprints
const assumableEKSAdminAccessRole = new iam.Role(stack, 'assumableEKSAdminAccessRole', {
assumedBy: new iam.AccountRootPrincipal(), //<-- root as is root of the account,
// so assumable by any principle/identity in the account.
});
const cluster = new eks.Cluster(stack, 'eks', {
clusterName: 'example',
version: eks.KubernetesVersion.V1_32,
kubectlLayer: new KubectlV32Layer(stack, 'kubectl'),
authenticationMode: eks.AuthenticationMode.API_AND_CONFIG_MAP,
mastersRole: assumableEKSAdminAccessRole, //<-- adds aws eks update-kubeconfig output
});
const ALBC_kube_SA = new eks.ServiceAccount(stack, 'aws-load-balancer-controller_kube-sa', {
cluster: cluster,
name: 'aws-load-balancer-controller',
namespace: 'kube-system',
identityType: eks.IdentityType.POD_IDENTITY, //depends on eks-pod-identity-agent addon
//Note: It's not documented, but this generates 4 things:
//1. A kube SA in the namespace of the cluster
//2. An IAM role paired to the Kube SA
//3. An EKS Pod Identity Association
//4. The eks-pod-identity-agent addon (dependency)
});
const app2_kube_SA = new eks.ServiceAccount(stack, 'app2_kube-sa', {
cluster: cluster,
name: 'app2',
namespace: 'default',
identityType: eks.IdentityType.POD_IDENTITY,
//^-- Just adding a 2nd to show that this has limited logic to detect if addon already exists
// If you have at least 1 of these typescript objects it triggers deploy of eks-pod-identity-agent
// but if you have more than 1 it's smart enough to not fail from trying to deploy it twice.
});
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// NOTICE: Don't add eks-pod-identity-agent
// It's purposefully left out to work around CDK bug https://github.com/aws/aws-cdk/issues/32580
// The cdk bug relates to an error where there's a complaint about it already being present, if 2 things try to install it.
// The AWS Load Balancer Controller installation logic, will trigger the installation of eks-pod-identity-agent addon.
// https://github.com/aws/aws-cdk/pull/33891
// ^-- A WIP fix exists
// Note the IaC will deploy default (which isn't latest)
// but if you manually update in GUI it'll stay updated
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/*To see official names of all eks add-ons:
aws eks describe-addon-versions \
--kubernetes-version=1.31 \
--query 'sort_by(addons &owner)[].{owner: owner, addonName: addonName}' \
--output table
*/
// v-- UNCOMMENT THIS to experience the bug (or test any WIP changes)
// const eks_pod_identity_agent_addon = new eks.CfnAddon(stack, 'eks-pod-identity-agent', {
// clusterName: cluster.clusterName,
// addonName: 'eks-pod-identity-agent',
// addonVersion: 'v1.3.5-eksbuild.2', //v--query for latest
// // aws eks describe-addon-versions --kubernetes-version=1.32 --addon-name=eks-pod-identity-agent --query='addons[].addonVersions[].addonVersion' | jq '.[0]'
// resolveConflicts: 'OVERWRITE',
// configurationValues: `{}`,
// }); |
Issue # (if applicable)
Closes #32580.
Reason for this change
Allows for creating the
eksPodIdentityAgent
outside of just referencing the property.Description of changes
I changed the logic in the get function of
eksPodIdentityAgent
to look for existing addons and use that, rather than just create one every time.Describe any new or updated permissions being added
None
Description of how you validated changes
Added many unit tests for the ways to interact with this:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license