Skip to content
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

fix(eks): fargateCluster compatibility with AuthenticationMode.API #31267

Merged
merged 16 commits into from
Sep 11, 2024
Original file line number Diff line number Diff line change
@@ -1,30 +1,53 @@
/// !cdk-integ pragma:disable-update-workflow
import { App, Stack } from 'aws-cdk-lib';
import * as integ from '@aws-cdk/integ-tests-alpha';
import { App, Stack, StackProps } from 'aws-cdk-lib';
// import * as integ from '@aws-cdk/integ-tests-alpha';
import { getClusterVersionConfig } from './integ-tests-kubernetes-version';
import * as eks from 'aws-cdk-lib/aws-eks';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import { EC2_RESTRICT_DEFAULT_SECURITY_GROUP } from 'aws-cdk-lib/cx-api';

interface EksFargateClusterStackProps extends StackProps {
authMode?: eks.AuthenticationMode;
vpc?: ec2.IVpc;
}
class EksFargateClusterStack extends Stack {

constructor(scope: App, id: string) {
super(scope, id);
public readonly vpc: ec2.IVpc;
constructor(scope: App, id: string, props?: EksFargateClusterStackProps) {
super(scope, id, props);

this.node.setContext(EC2_RESTRICT_DEFAULT_SECURITY_GROUP, false);
this.vpc = props?.vpc ?? this.createDummyVpc();
new eks.FargateCluster(this, 'FargateCluster', {
...getClusterVersionConfig(this),
prune: false,
authenticationMode: props?.authMode,
vpc: this.vpc,
});
}
private createDummyVpc(): ec2.IVpc {
return new ec2.Vpc(this, 'DummyVpc', { maxAzs: 2, natGateways: 1, restrictDefaultSecurityGroup: false });
}
}

const app = new App();

const stack = new EksFargateClusterStack(app, 'aws-cdk-eks-fargate-cluster-test');
new integ.IntegTest(app, 'aws-cdk-eks-fargate-cluster', {
testCases: [stack],
// Test includes assets that are updated weekly. If not disabled, the upgrade PR will fail.
diffAssets: false,
// create fargate cluster with undefined auth mode
const stack1 = new EksFargateClusterStack(app, 'aws-cdk-eks-fargate-cluster-test', {
authMode: eks.AuthenticationMode.API,
});

app.synth();
Array.isArray(stack1);

// // create the 2nd fargate cluster in the same vpc, but with api auth mode
// const stack2 = new EksFargateClusterStack(app, 'aws-cdk-eks-fargate-cluster-test2', {
// authMode: eks.AuthenticationMode.API,
// vpc: stack1.vpc,
// });

// new integ.IntegTest(app, 'aws-cdk-eks-fargate-cluster', {
// testCases: [stack1, stack2],
// // Test includes assets that are updated weekly. If not disabled, the upgrade PR will fail.
// diffAssets: false,
// });

// app.synth();
25 changes: 14 additions & 11 deletions packages/aws-cdk-lib/aws-eks/lib/fargate-profile.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Construct } from 'constructs';
import { Cluster } from './cluster';
import { Cluster, AuthenticationMode } from './cluster';
import { FARGATE_PROFILE_RESOURCE_TYPE } from './cluster-resource-handler/consts';
import { ClusterResourceProvider } from './cluster-resource-provider';
import * as ec2 from '../../aws-ec2';
Expand Down Expand Up @@ -201,15 +201,18 @@ export class FargateProfile extends Construct implements ITaggable {
resource.node.addDependency(previousProfile);
}

// map the fargate pod execution role to the relevant groups in rbac
// see https://github.com/aws/aws-cdk/issues/7981
props.cluster.awsAuth.addRoleMapping(this.podExecutionRole, {
username: 'system:node:{{SessionName}}',
groups: [
'system:bootstrappers',
'system:nodes',
'system:node-proxier',
],
});
const supportConfigMap = props.cluster.authenticationMode !== AuthenticationMode.API ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concluding support for something by exclusion works in a closed world, but I'm not sure the world is actually closed. If a 4th authentication method is added in the future, will this condition have to be updated? Isn't is safer to inclusively check for one of the CONFIG_MAP options?

if (supportConfigMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me like something that was unconditional is now conditional, and PR body doesn't tell me anything about what the problem was or why this is the fix.

That's leaving me to wonder: what are the backwards compatibility implications of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Just fixed.

// map the fargate pod execution role to the relevant groups in rbac
// see https://github.com/aws/aws-cdk/issues/7981
props.cluster.awsAuth.addRoleMapping(this.podExecutionRole, {
username: 'system:node:{{SessionName}}',
groups: [
'system:bootstrappers',
'system:nodes',
'system:node-proxier',
],
});
}
}
}
Loading