fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified #21362

watany-dev · 2022-07-28T15:39:33Z

fixed: #20779

The original issue was that in update-cluster-config, updates.updateLogging and updates.updateAccess were not separated and were state dependent.
We reviewed the logic in the conditional branch and decided to make a decision for each state.

As a point of interest for this repair, we picked out the case where updateClusterConfigRequest is updates.updateLogging && updates.updateAccess, and used unittest and We have not added integtest because it seems that the existing integtest is sufficient to verify that the existing behavior is as before.
I would be very glad to get advice on this, as I have a little trouble coming up with a perspective to check if it is necessary.

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-28T15:39:42Z

TheRealAmazonKendra

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log.

Additionally, please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)

TheRealAmazonKendra · 2022-07-28T15:44:13Z

I think we also need some integration tests for this fix.

watany-dev · 2022-07-28T16:01:47Z

Thank you. I will challenge the INTEGRATION TESTS and take this issue in a better direction.

yakireliyahu1987 · 2022-08-02T19:04:07Z

packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts

@@ -137,10 +137,17 @@ export class ClusterResourceHandler extends ResourceHandler {
    }

    if (updates.updateLogging || updates.updateAccess) {


@watany-dev an error should be thrown when updates.updateLogging and updates.updateAccess are both true, as updating both logging configuration and resources vpc configuration results in an error from the EKS API (even though it is not documented in EKS UpdateClusterConfig API)

It seems I did not know the correct specifications. I will try to re-write it again with that useful information. Thanks.

Pull request has been modified.

TheRealAmazonKendra

Overall this needs more testing. I've already mentioned integration tests, I think, but this also needs a bit more unit testing coverage for the change. If it's actually already covered but is just not showing up in this PR because you didn't make changes to it, please just link it and I'll take a look to ensure I think it's enough.

TheRealAmazonKendra · 2022-08-03T20:01:31Z

packages/@aws-cdk/aws-eks/test/cluster.test.ts

    });
+    Template.fromStack(stack);


This is no longer testing anything.

TheRealAmazonKendra · 2022-08-03T20:10:42Z

packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts

@@ -136,23 +136,33 @@ export class ClusterResourceHandler extends ResourceHandler {
      return this.updateClusterVersion(this.newProps.version);
    }

-    if (updates.updateLogging || updates.updateAccess) {
+    //error should be thrown when updates.updateLogging and updates.updateAccess are both true


Suggested change

//error should be thrown when updates.updateLogging and updates.updateAccess are both true

// Error should be thrown when updates.updateLogging and updates.updateAccess are both true

TheRealAmazonKendra · 2022-08-03T20:15:24Z

packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts

-    if (updates.updateLogging || updates.updateAccess) {
+    //error should be thrown when updates.updateLogging and updates.updateAccess are both true
+    if (updates.updateLogging && updates.updateAccess) {
+      throw new Error('Cannot update cluster updateLogging and updateAccess are both true');


Suggested change

throw new Error('Cannot update cluster updateLogging and updateAccess are both true');

throw new Error('updateLogging and updateAccess cannot both be set to true');

TheRealAmazonKendra · 2022-08-03T20:22:23Z

packages/@aws-cdk/aws-eks/test/cluster.test.ts

  });

+


Suggested change

TheRealAmazonKendra

Actually, I've just done a deeper dive into the code and I don't think this is the right path forward. I think that we shouldn't throw an error if both are true and, instead, do one update and then the other in succession.

watany-dev · 2022-08-04T03:58:52Z

Thanks for your comment. I will re-implement this policy.

if both are true and, instead, do one update and then the other in succession.

I am embarrassed to ask this because it is an elementary question.
I am having trouble getting .updateClusterConfig to return as a single value when I run it twice and return it as a single return, as in the sample below.
When I try to merge updateResponse1 and updateResponse2 in the example below, the updateLogging result is overwritten by the updateAccess result. (i.e., Logging is empty).

    if (updates.updateLogging) {
      const config: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
        logging: this.newProps.logging,
      };
      if (!updates.updateAccess) {
        const updateResponse1: aws.EKS.UpdateClusterConfigResponse = await this.eks.updateClusterConfig(config);
        return { EksUpdateId: updateResponse1.update?.id };
      }

      const config2: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
      };
      config2.resourcesVpcConfig = {
        endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess,
        endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess,
        publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs,
      };

      const updateResponse1 = await this.eks.updateClusterConfig(config);
      const updateResponse2 = await this.eks.updateClusterConfig(config2);
      const updateResponse = { ...updateResponse1, ...updateResponse2 };
      // some deep copy?
      return { EksUpdateId: (updateResponse.update?.id) };
    }

yakireliyahu1987 · 2022-08-04T10:20:33Z

Thanks for your comment. I will re-implement this policy.

if both are true and, instead, do one update and then the other in succession.

I am embarrassed to ask this because it is an elementary question. I am having trouble getting .updateClusterConfig to return as a single value when I run it twice and return it as a single return, as in the sample below. When I try to merge updateResponse1 and updateResponse2 in the example below, the updateLogging result is overwritten by the updateAccess result. (i.e., Logging is empty).

    if (updates.updateLogging) {
      const config: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
        logging: this.newProps.logging,
      };
      if (!updates.updateAccess) {
        const updateResponse1: aws.EKS.UpdateClusterConfigResponse = await this.eks.updateClusterConfig(config);
        return { EksUpdateId: updateResponse1.update?.id };
      }

      const config2: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
      };
      config2.resourcesVpcConfig = {
        endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess,
        endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess,
        publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs,
      };

      const updateResponse1 = await this.eks.updateClusterConfig(config);
      const updateResponse2 = await this.eks.updateClusterConfig(config2);
      const updateResponse = { ...updateResponse1, ...updateResponse2 };
      // some deep copy?
      return { EksUpdateId: (updateResponse.update?.id) };
    }

@watany-dev I'm not sure the suggested logic will work (even before merging the responses, which has its own issues), as when you update the cluster configuration in the first command, it transitions to an "Updating" state, and will reject any further requests until that operation is done, and the cluster is back to an "Active" state. You should have some waiting logic that ensures the cluster has applied the first update before continuing to the next operation.

TheRealAmazonKendra · 2022-08-04T19:24:17Z

Thanks for your comment. I will re-implement this policy.

if both are true and, instead, do one update and then the other in succession.

I am embarrassed to ask this because it is an elementary question. I am having trouble getting .updateClusterConfig to return as a single value when I run it twice and return it as a single return, as in the sample below. When I try to merge updateResponse1 and updateResponse2 in the example below, the updateLogging result is overwritten by the updateAccess result. (i.e., Logging is empty).
    if (updates.updateLogging) {
      const config: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
        logging: this.newProps.logging,
      };
      if (!updates.updateAccess) {
        const updateResponse1: aws.EKS.UpdateClusterConfigResponse = await this.eks.updateClusterConfig(config);
        return { EksUpdateId: updateResponse1.update?.id };
      }

      const config2: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
      };
      config2.resourcesVpcConfig = {
        endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess,
        endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess,
        publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs,
      };

      const updateResponse1 = await this.eks.updateClusterConfig(config);
      const updateResponse2 = await this.eks.updateClusterConfig(config2);
      const updateResponse = { ...updateResponse1, ...updateResponse2 };
      // some deep copy?
      return { EksUpdateId: (updateResponse.update?.id) };
    }
@watany-dev I'm not sure the suggested logic will work (even before merging the responses, which has its own issues), as when you update the cluster configuration in the first command, it transitions to an "Updating" state, and will reject any further requests until that operation is done, and the cluster is back to an "Active" state. You should have some waiting logic that ensures the cluster has applied the first update before continuing to the next operation.

@yakireliyahu1987 Thanks for adding your input on this. I would think that adding in retry logic would do the trick here.

TheRealAmazonKendra · 2022-08-04T19:28:50Z

Thanks for your comment. I will re-implement this policy.

if both are true and, instead, do one update and then the other in succession.

I am embarrassed to ask this because it is an elementary question. I am having trouble getting .updateClusterConfig to return as a single value when I run it twice and return it as a single return, as in the sample below. When I try to merge updateResponse1 and updateResponse2 in the example below, the updateLogging result is overwritten by the updateAccess result. (i.e., Logging is empty).

    if (updates.updateLogging) {
      const config: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
        logging: this.newProps.logging,
      };
      if (!updates.updateAccess) {
        const updateResponse1: aws.EKS.UpdateClusterConfigResponse = await this.eks.updateClusterConfig(config);
        return { EksUpdateId: updateResponse1.update?.id };
      }

      const config2: aws.EKS.UpdateClusterConfigRequest = {
        name: this.clusterName,
      };
      config2.resourcesVpcConfig = {
        endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess,
        endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess,
        publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs,
      };

      const updateResponse1 = await this.eks.updateClusterConfig(config);
      const updateResponse2 = await this.eks.updateClusterConfig(config2);
      const updateResponse = { ...updateResponse1, ...updateResponse2 };
      // some deep copy?
      return { EksUpdateId: (updateResponse.update?.id) };
    }

Absolutely no need to be embarrassed by any questions! I'm happy to deep dive with you on this. Investing in our contributor community both helps everyone learn and helps us get better code.

Besides my generic answer of adding a retry, I'll provide some more thoughts on approaches shortly. I need to look a little closer at the codebase here. It's not one of my areas of expertise.

TheRealAmazonKendra · 2022-08-06T07:28:43Z

So sorry that I didn't get back to this. I'll provide some thoughts for real on Monday.

TheRealAmazonKendra · 2022-08-08T22:55:12Z

So, basically, what you want to do here is probably add a while loop around the first update being performed that waits for isUpdateComplete() to return a terminal response before starting the next update. If the first fails, you can utilize the failure handling in isActive()and let that throw an error, but if it succeeds, then performupdates.updateAccess`. Does this make sense?

aws-cdk-automation · 2022-08-11T09:25:36Z

AWS CodeBuild CI Report

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

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

watany-dev · 2022-08-11T12:35:25Z

I apologize for closing with an operational error.
I am in the process of writing a unit test of the proposed fix. I will not finish the response. I will issue another pullrequest in some form.

aws-cdk-automation requested a review from a team July 28, 2022 15:39

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

TheRealAmazonKendra previously requested changes Jul 28, 2022

View reviewed changes

watany-dev changed the title ~~fix: EKS cluster_logging property makes cluster unmanageable~~ fix(eks): EKS cluster_logging property makes cluster unmanageable Jul 28, 2022

TheRealAmazonKendra changed the title ~~fix(eks): EKS cluster_logging property makes cluster unmanageable~~ fix(eks): cluster_logging property makes cluster unmanageable Jul 28, 2022

yakireliyahu1987 reviewed Aug 2, 2022

View reviewed changes

watany-dev force-pushed the fix-20779 branch from 70dfdba to 20a2eb1 Compare August 3, 2022 10:49

watany-dev force-pushed the fix-20779 branch from 20a2eb1 to 1ca8a2d Compare August 3, 2022 11:06

TheRealAmazonKendra changed the title ~~fix(eks): cluster_logging property makes cluster unmanageable~~ fix(eks): cluster built with cluster_logging property set prevents the cluster from being modified Aug 3, 2022

TheRealAmazonKendra requested changes Aug 3, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~fix(eks): cluster built with cluster_logging property set prevents the cluster from being modified~~ fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified Aug 3, 2022

watany-dev closed this Aug 11, 2022

watany-dev force-pushed the fix-20779 branch from 0996582 to de69290 Compare August 11, 2022 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified #21362

fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified #21362

watany-dev commented Jul 28, 2022 •

edited

gitpod-io bot commented Jul 28, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra commented Jul 28, 2022

watany-dev commented Jul 28, 2022

yakireliyahu1987 Aug 2, 2022

watany-dev Aug 2, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 3, 2022

TheRealAmazonKendra Aug 3, 2022

TheRealAmazonKendra Aug 3, 2022

TheRealAmazonKendra Aug 3, 2022

TheRealAmazonKendra left a comment

watany-dev commented Aug 4, 2022 •

edited

yakireliyahu1987 commented Aug 4, 2022

TheRealAmazonKendra commented Aug 4, 2022

TheRealAmazonKendra commented Aug 4, 2022

TheRealAmazonKendra commented Aug 6, 2022

TheRealAmazonKendra commented Aug 8, 2022

aws-cdk-automation commented Aug 11, 2022

watany-dev commented Aug 11, 2022

		@@ -137,10 +137,17 @@ export class ClusterResourceHandler extends ResourceHandler {
		}

		if (updates.updateLogging \|\| updates.updateAccess) {

	//error should be thrown when updates.updateLogging and updates.updateAccess are both true
	// Error should be thrown when updates.updateLogging and updates.updateAccess are both true

	throw new Error('Cannot update cluster updateLogging and updateAccess are both true');
	throw new Error('updateLogging and updateAccess cannot both be set to true');

fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified #21362

fix(eks): cluster built with clusterLogging property set prevents the cluster from being modified #21362

Conversation

watany-dev commented Jul 28, 2022 • edited

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 28, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jul 28, 2022

watany-dev commented Jul 28, 2022

yakireliyahu1987 Aug 2, 2022

Choose a reason for hiding this comment

watany-dev Aug 2, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 3, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 3, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 3, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 3, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

watany-dev commented Aug 4, 2022 • edited

yakireliyahu1987 commented Aug 4, 2022

TheRealAmazonKendra commented Aug 4, 2022

TheRealAmazonKendra commented Aug 4, 2022

TheRealAmazonKendra commented Aug 6, 2022

TheRealAmazonKendra commented Aug 8, 2022

aws-cdk-automation commented Aug 11, 2022

AWS CodeBuild CI Report

watany-dev commented Aug 11, 2022

watany-dev commented Jul 28, 2022 •

edited

watany-dev commented Aug 4, 2022 •

edited