Skip to content

Commit

Permalink
fix(eks): cannot disable cluster logging once it has been enabled (#2…
Browse files Browse the repository at this point in the history 
…1185)

### Description

Amazon EKS Loggings was first introduced in #18112 and added Fargate logging support in #20707. However, disabled loggings was not take into consideration at the very beginning of the design.

With this PR, enabled clusterLogging support for enablement/disablement. Also, added test cases for no logging configured, partial logging configured, full logging configured.

Fixes: #19898

### Test Results

#### Tool versions

```sh
$ node --version
v16.16.0
```

```sh
$ yarn --version
1.22.19
```

#### yarn build

```sh
$ yarn build
yarn run v1.22.19
$ cdk-build
...

Build times for @aws-cdk/aws-eks: Total time (59.5s) | /home/ec2-user/aws-cdk/tools/@aws-cdk/cdk-build-tools/node_modules/jsii/bin/jsii (35.0s) | cfn2ts (0.6s)
Done in 59.94s.
```

#### yarn test

```sh
$ yarn test

yarn run v1.22.19
$ cdk-test
PASS test/cluster.test.js (33.488 s)
PASS test/nodegroup.test.js (10.465 s)
PASS test/helm-chart.test.js (5.964 s)
PASS test/fargate.test.js (5.391 s)
PASS test/k8s-manifest.test.js
PASS test/service-account.test.js
PASS test/user-data.test.js
PASS test/awsauth.test.js
PASS test/alb-controller.test.js
PASS test/k8s-patch.test.js
...

=============================== Coverage summary ===============================
Statements   : 93.08% ( 888/954 )
Branches     : 89.47% ( 442/494 )
Functions    : 94.03% ( 142/151 )
Lines        : 94.83% ( 881/929 )
================================================================================

Test Suites: 13 passed, 13 total
Tests:       277 passed, 277 total
Snapshots:   0 total
Time:        77.227 s, estimated 91 s
Ran all test suites.

Verifying integration test snapshots...

  UNCHANGED  integ.eks-oidc-provider 27.294s
  UNCHANGED  integ.fargate-cluster 35.153s
  UNCHANGED  integ.eks-cluster-private-endpoint 35.223s
  UNCHANGED  integ.eks-helm-asset 35.602s
  UNCHANGED  integ.eks-bottlerocket-ng 35.981s
  UNCHANGED  integ.eks-cluster-handlers-vpc 36.266s
  UNCHANGED  integ.eks-inference 36.801s
  UNCHANGED  integ.alb-controller 37.565s
  UNCHANGED  integ.eks-cluster 38.041s

Snapshot Results:

Tests:    9 passed, 9 total
Tests successful. Total time (1m59.1s) | /home/ec2-user/aws-cdk/node_modules/jest/bin/jest.js (1m18.4s) | integ-runner (40.6s)
Done in 119.26s.
```

#### yarn integ

```sh
$ yarn integ
yarn run v1.22.19
$ integ-runner

Verifying integration test snapshots...

  UNCHANGED  integ.eks-oidc-provider 25.049s
  UNCHANGED  integ.eks-bottlerocket-ng 34.895s
  UNCHANGED  integ.eks-cluster-private-endpoint 35.012s
  UNCHANGED  integ.fargate-cluster 35.417s
  UNCHANGED  integ.eks-cluster-handlers-vpc 35.563s
  UNCHANGED  integ.eks-helm-asset 35.691s
  UNCHANGED  integ.eks-inference 36.303s
  UNCHANGED  integ.alb-controller 37.071s
  UNCHANGED  integ.eks-cluster 37.843s

Snapshot Results:

Tests:    9 passed, 9 total
Done in 40.59s.
```

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [X] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@guessi
guessi authored Aug 1, 2022
1 parent c64cccb commit e41b073
Show file tree
Hide file tree
Showing 8 changed files with 220 additions and 6 deletions.
4 changes: 4 additions & 0 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,10 @@ function parseProps(props: any): aws.EKS.CreateClusterRequest {
parsed.logging.clusterLogging[0].enabled = parsed.logging.clusterLogging[0].enabled === 'true';
}

if (typeof (parsed.logging?.clusterLogging[1].enabled) === 'string') {
parsed.logging.clusterLogging[1].enabled = parsed.logging.clusterLogging[1].enabled === 'false';
}

return parsed;

}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-eks/lib/cluster-resource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export interface ClusterResourceProps {
readonly onEventLayer?: lambda.ILayerVersion;
readonly clusterHandlerSecurityGroup?: ec2.ISecurityGroup;
readonly tags?: { [key: string]: string };
readonly logging?: { [key: string]: [ { [key: string]: any } ] };
readonly logging?: { [key: string]: [ { [key: string]: any }, { [key: string]: any } ] };
}

/**
Expand Down
18 changes: 17 additions & 1 deletion packages/@aws-cdk/aws-eks/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,7 @@ export class Cluster extends ClusterBase {

private readonly version: KubernetesVersion;

private readonly logging?: { [key: string]: [ { [key: string]: any } ] };
private readonly logging?: { [key: string]: [ { [key: string]: any }, { [key: string]: any } ] };

/**
* A dummy CloudFormation resource that is used as a wait barrier which
Expand Down Expand Up @@ -1347,12 +1347,28 @@ export class Cluster extends ClusterBase {
// Get subnetIds for all selected subnets
const subnetIds = Array.from(new Set(flatten(selectedSubnetIdsPerGroup)));

// The value of clusterLoggingTypeDisabled should be invert of props.clusterLogging.
let clusterLoggingTypeDisabled: ClusterLoggingTypes[] = [];

// Find out type(s) to disable.
Object.values(ClusterLoggingTypes).forEach(function (key) {
let clusterLoggingTypeEnabled = Object.values(props.clusterLogging ? Object.values(props.clusterLogging) : []);
if (!Object.values(clusterLoggingTypeEnabled).includes(key)) {
clusterLoggingTypeDisabled.push(key);
};
});

// Leave it untouched as undefined if (props.clusterLogging === undefined).
this.logging = props.clusterLogging ? {
clusterLogging: [
{
enabled: true,
types: Object.values(props.clusterLogging),
},
{
enabled: false,
types: Object.values(clusterLoggingTypeDisabled),
},
],
} : undefined;

Expand Down
16 changes: 16 additions & 0 deletions packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,10 @@ describe('cluster resource provider', () => {
types: ['api'],
enabled: true,
},
{
types: ['audit', 'authenticator', 'controllerManager', 'scheduler'],
enabled: false,
},
],
},
}, {
Expand All @@ -581,6 +585,10 @@ describe('cluster resource provider', () => {
types: ['api'],
enabled: true,
},
{
types: ['audit', 'authenticator', 'controllerManager', 'scheduler'],
enabled: false,
},
],
},
});
Expand Down Expand Up @@ -622,6 +630,10 @@ describe('cluster resource provider', () => {
types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'],
enabled: true,
},
{
types: [],
enabled: false,
},
],
},
resourcesVpcConfig: {
Expand All @@ -644,6 +656,10 @@ describe('cluster resource provider', () => {
types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'],
enabled: true,
},
{
types: [],
enabled: false,
},
],
},
resourcesVpcConfig: {
Expand Down
94 changes: 94 additions & 0 deletions packages/@aws-cdk/aws-eks/test/cluster.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3156,4 +3156,98 @@ describe('cluster', () => {
},
});
});

test('create a cluster without logging configure', () => {
// GIVEN
const { stack } = testFixture();

// WHEN
new eks.Cluster(stack, 'Cluster', {
version: CLUSTER_VERSION,
});

// THEN
Template.fromStack(stack).resourceCountIs('Custom::AWSCDK-EKS-Cluster::Config::logging', 0);
});

test('create a cluster with partial logging configure', () => {
// GIVEN
const { stack } = testFixture();

// WHEN
new eks.Cluster(stack, 'Cluster', {
version: CLUSTER_VERSION,
clusterLogging: [
eks.ClusterLoggingTypes.API,
eks.ClusterLoggingTypes.AUTHENTICATOR,
eks.ClusterLoggingTypes.SCHEDULER,
],
});

// THEN
Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', {
Config: {
logging: {
clusterLogging: [
{
enabled: true,
types: [
'api',
'authenticator',
'scheduler',
],
},
{
enabled: false,
types: [
'audit',
'controllerManager',
],
},
],
},
},
});
});

test('create a cluster with all logging configure', () => {
// GIVEN
const { stack } = testFixture();

// WHEN
new eks.Cluster(stack, 'Cluster', {
version: CLUSTER_VERSION,
clusterLogging: [
eks.ClusterLoggingTypes.API,
eks.ClusterLoggingTypes.AUDIT,
eks.ClusterLoggingTypes.AUTHENTICATOR,
eks.ClusterLoggingTypes.CONTROLLER_MANAGER,
eks.ClusterLoggingTypes.SCHEDULER,
],
});

// THEN
Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', {
Config: {
logging: {
clusterLogging: [
{
enabled: true,
types: [
'api',
'audit',
'authenticator',
'controllerManager',
'scheduler',
],
},
{
enabled: false,
types: [],
},
],
},
},
});
});
});
9 changes: 8 additions & 1 deletion ...s/@aws-cdk/aws-eks/test/eks-cluster.integ.snapshot/aws-cdk-eks-cluster-test.template.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,13 @@
"authenticator",
"scheduler"
]
},
{
"enabled": false,
"types": [
"audit",
"controllerManager"
]
}
]
}
Expand Down Expand Up @@ -4032,4 +4039,4 @@
"Default": "/aws/service/bottlerocket/aws-k8s-1.21/x86_64/latest/image_id"
}
}
}
}
9 changes: 8 additions & 1 deletion ...s/@aws-cdk/aws-eks/test/eks-helm-asset.integ.snapshot/aws-cdk-eks-helm-test.template.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,13 @@
"authenticator",
"scheduler"
]
},
{
"enabled": false,
"types": [
"audit",
"controllerManager"
]
}
]
}
Expand Down Expand Up @@ -1316,4 +1323,4 @@
"Description": "Artifact hash for asset \"3d78a5cdc39276c4ee8503417d4363951a0693b01cfd99ec9786feed456d012f\""
}
}
}
}
74 changes: 72 additions & 2 deletions packages/@aws-cdk/aws-eks/test/fargate.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,62 @@ describe('fargate', () => {

});

test('supports cluster logging with FargateCluster', () => {
test('supports cluster logging without FargateCluster', () => {
// GIVEN
const stack = new Stack();

// WHEN

new eks.FargateCluster(stack, 'FargateCluster', {
version: CLUSTER_VERSION,
});

//THEN
Template.fromStack(stack).resourceCountIs('Custom::AWSCDK-EKS-Cluster::Config::logging', 0);
});

test('supports cluster partial logging enabled with FargateCluster', () => {
// GIVEN
const stack = new Stack();

// WHEN

new eks.FargateCluster(stack, 'FargateCluster', {
version: CLUSTER_VERSION,
clusterLogging: [
eks.ClusterLoggingTypes.API,
eks.ClusterLoggingTypes.AUTHENTICATOR,
eks.ClusterLoggingTypes.SCHEDULER,
],
});

//THEN
Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', {
Config: {
logging: {
clusterLogging: [
{
enabled: true,
types: [
'api',
'authenticator',
'scheduler',
],
},
{
enabled: false,
types: [
'audit',
'controllerManager',
],
},
],
},
},
});
});

test('supports cluster all logging enabled with FargateCluster', () => {
// GIVEN
const stack = new Stack();

Expand All @@ -469,7 +524,9 @@ describe('fargate', () => {
version: CLUSTER_VERSION,
clusterLogging: [
eks.ClusterLoggingTypes.API,
eks.ClusterLoggingTypes.AUDIT,
eks.ClusterLoggingTypes.AUTHENTICATOR,
eks.ClusterLoggingTypes.CONTROLLER_MANAGER,
eks.ClusterLoggingTypes.SCHEDULER,
],
});
Expand All @@ -479,7 +536,20 @@ describe('fargate', () => {
Config: {
logging: {
clusterLogging: [
{ enabled: true, types: ['api', 'authenticator', 'scheduler'] },
{
enabled: true,
types: [
'api',
'audit',
'authenticator',
'controllerManager',
'scheduler',
],
},
{
enabled: false,
types: [],
},
],
},
},
Expand Down

0 comments on commit e41b073

Please sign in to comment.