fix(eks): cannot disable cluster logging once it has been enabled #21185

guessi · 2022-07-17T07:10:43Z

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

$ node --version
v16.16.0

$ yarn --version
1.22.19

yarn build

$ 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

$ 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

$ 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:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-07-17T07:10:50Z

guessi · 2022-07-17T09:46:12Z

Test failed, but log are unrelated? Trigger test again.

@aws-cdk/aws-appsync: /codebuild/output/src931734453/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:377
@aws-cdk/aws-appsync:             throw Error('API key expiration must be between 1 and 365 days.');
@aws-cdk/aws-appsync:             ^
@aws-cdk/aws-appsync: Error: API key expiration must be between 1 and 365 days.
@aws-cdk/aws-appsync:     at GraphqlApi.createAPIKey (/codebuild/output/src931734453/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:377:19)
@aws-cdk/aws-appsync:     at new GraphqlApi (/codebuild/output/src931734453/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/lib/graphqlapi.js:189:40)
@aws-cdk/aws-appsync:     at Object.<anonymous> (/codebuild/output/src931734453/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-appsync/test/integ.auth-apikey.js:23:13)
@aws-cdk/aws-appsync:     at Module._compile (internal/modules/cjs/loader.js:1085:14)
@aws-cdk/aws-appsync:     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
@aws-cdk/aws-appsync:     at Module.load (internal/modules/cjs/loader.js:950:32)
@aws-cdk/aws-appsync:     at Function.Module._load (internal/modules/cjs/loader.js:790:12)
@aws-cdk/aws-appsync:     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:75:12)
@aws-cdk/aws-appsync:     at internal/main/run_main_module.js:17:47
@aws-cdk/aws-appsync:   ERROR      integ.auth-apikey 0.592s
@aws-cdk/aws-appsync:       Command exited with status 1

Amazon EKS Loggings was first introduced in aws#18112 and added Fargate logging support in aws#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: aws#19898

guessi · 2022-07-24T05:55:43Z

rebased

TheRealAmazonKendra

Thanks for implementing this fix! I think this is the first PR that I have no notes for.

mergify · 2022-08-01T21:57:41Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-08-01T22:34:49Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 29b53f5
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-08-01T22:35:11Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

introduced by aws#21185 Fixes: aws#21436

…#21463) introduced by #21185 Fixes: #21436 ---- ### 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*

…bled (#21185)" This reverts commit e41b073.

…bled" (#21545) Reverts #21185 and #21463 Closes #21515 Re-opens #19898. Fix for this in progress. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

…s#21185) ### Description Amazon EKS Loggings was first introduced in aws#18112 and added Fargate logging support in aws#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: aws#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*

…aws#21463) introduced by aws#21185 Fixes: aws#21436 ---- ### 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*

…bled" (aws#21545) Reverts aws#21185 and aws#21463 Closes aws#21515 Re-opens aws#19898. Fix for this in progress. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

aws-cdk-automation requested a review from a team July 17, 2022 07:10

github-actions bot added the p2 label Jul 17, 2022

guessi force-pushed the extend-support-for-eks-cluster-loggings branch 3 times, most recently from da58aa1 to cc7509d Compare July 17, 2022 09:41

guessi force-pushed the extend-support-for-eks-cluster-loggings branch from cc7509d to 95ef236 Compare July 24, 2022 01:31

TheRealAmazonKendra changed the title ~~chore(eks): extend support for clusterLogging disable~~ fix(eks): cannot disable cluster logging once it has been enabled Aug 1, 2022

TheRealAmazonKendra approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'main' into extend-support-for-eks-cluster-loggings

29b53f5

mergify bot merged commit e41b073 into aws:main Aug 1, 2022

This was referenced Aug 3, 2022

aws-eks: Recent change breaks update of existing clusters #21436

Closed

aws-eks: Cannot update cluster endpoint access #21439

Closed

guessi deleted the extend-support-for-eks-cluster-loggings branch August 3, 2022 16:49

guessi added a commit to guessi/aws-cdk that referenced this pull request Aug 3, 2022

fix(eks): missing question marks cause update cluster setting failure

2a5f339

introduced by aws#21185 Fixes: aws#21436

guessi added a commit to guessi/aws-cdk that referenced this pull request Aug 4, 2022

fix(eks): missing question marks cause update cluster setting failure

7e1b1dd

introduced by aws#21185 Fixes: aws#21436

guessi mentioned this pull request Aug 4, 2022

fix(eks): missing question marks cause update cluster setting failure #21463

Merged

4 tasks

TCBeekley mentioned this pull request Aug 9, 2022

aws-cdk-eks: stack fails and corrupts after upgrade to v2.35 due to EKS logging config change #21515

Closed

vinayak-kukreja mentioned this pull request Aug 10, 2022

fix: revert recent EKS fixes(e41b07341 and 1000abe43) #21544

Closed

TheRealAmazonKendra added a commit that referenced this pull request Aug 10, 2022

Revert "fix(eks): cannot disable cluster logging once it has been ena…

6dbc82a

…bled (#21185)" This reverts commit e41b073.

TheRealAmazonKendra mentioned this pull request Aug 10, 2022

revert "fix(eks): cannot disable cluster logging once it has been enabled" #21545

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eks): cannot disable cluster logging once it has been enabled #21185

fix(eks): cannot disable cluster logging once it has been enabled #21185

guessi commented Jul 17, 2022

gitpod-io bot commented Jul 17, 2022

guessi commented Jul 17, 2022 •

edited

Loading

guessi commented Jul 24, 2022

TheRealAmazonKendra left a comment

mergify bot commented Aug 1, 2022

aws-cdk-automation commented Aug 1, 2022

mergify bot commented Aug 1, 2022

fix(eks): cannot disable cluster logging once it has been enabled #21185

fix(eks): cannot disable cluster logging once it has been enabled #21185

Conversation

guessi commented Jul 17, 2022

Description

Test Results

Tool versions

yarn build

yarn test

yarn integ

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 17, 2022

guessi commented Jul 17, 2022 • edited Loading

guessi commented Jul 24, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 1, 2022

aws-cdk-automation commented Aug 1, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 1, 2022

guessi commented Jul 17, 2022 •

edited

Loading