fix(eks): circular dependencies when security groups from other stack…

…s are used (aws#10339) @eladb While working on this for the `aws-eks-next` module, i've started to second guess whether we really need to force cluster replacement. This little PR fixes all the circular dependency issues as well as corrects the `cluster.connections` to include the EKS managed cluster security group. **It does not require cluster replacement**. Merging this PR will leave two quirks/limitations: - The cluster will have a redundant control plane security group, just like it has now. While redundant, it doesn't hurt, and doesn't really justify a cluster replacement. - One cluster per stack, just like we have now. I think we've already agreed that this limitation also does not justify cluster replacement, and the only reason we wanted to implement it is because we wanted to piggy back on the previous argument, which I now believe is not strong enough. WDYT? -------- This PR resolves a few problem related to circular dependencies when the cluster is configured with security groups from other stacks. ## Problem Currently, the cluster creates a dedicated security group for the `KubectlProvider` and configures the main control plane security group to allow connections from it: https://github.com/aws/aws-cdk/blob/f5c5d060ffee19ac3daa47cbf6df8d3563133433/packages/%40aws-cdk/aws-eks/lib/cluster.ts#L918-L924 This eventually creates ingress rules in the stack of control plane security group. These ingress rules refer to the `KubectlProvider` creates in the cluster stack, hence, a dependency if formed between the control plane security group stack, and the cluster stack. `Control Plane Security Group Stack => Cluster Stack` (Control plane sg needs to allow connections from kubectl sg) On the other hand, the cluster stack naturally depends on the control plane security group stack because the cluster needs the security group when its created. And so: `Cluster Stack => Control Plane Security Group Stack` (Cluster needs the security group) When these two stacks are different, a circular dependency is created. ## Solution Do not create a security group for the `KubectlProvider` at all, and simply re-use the EKS managed security group. This security group is already configured to allow connections to the control plane, so if we simply attach it to the `KubectlProvider`, everything should work. This avoids the creation of the first dependency direction. Fixes aws#9754 Fixes aws#10020 Fixes aws#9536 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
cheruvian · Sep 14, 2020 · 857acbb · 857acbb
1 parent a2174a4
commit 857acbb
Show file tree

Hide file tree

Showing 4 changed files with 1,040 additions and 590 deletions.
diff --git a/packages/@aws-cdk/aws-eks/lib/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster.ts
@@ -848,11 +848,6 @@ export class Cluster extends ClusterBase {
       description: 'EKS Control Plane Security Group',
     });
 
-    this.connections = new ec2.Connections({
-      securityGroups: [securityGroup],
-      defaultPort: ec2.Port.tcp(443), // Control Plane has an HTTPS API
-    });
-
     this.vpcSubnets = props.vpcSubnets ?? [{ subnetType: ec2.SubnetType.PUBLIC }, { subnetType: ec2.SubnetType.PRIVATE }];
 
     // Get subnetIds for all selected subnets
@@ -916,17 +911,9 @@ export class Cluster extends ClusterBase {
 
       this.kubectlPrivateSubnets = privateSubents;
 
-      this.kubectlSecurityGroup = new ec2.SecurityGroup(this, 'KubectlProviderSecurityGroup', {
-        vpc: this.vpc,
-        description: 'Comminication between KubectlProvider and EKS Control Plane',
-      });
-
-      // grant the kubectl provider access to the cluster control plane.
-      this.connections.allowFrom(this.kubectlSecurityGroup, this.connections.defaultPort!);
-
-      // the security group and vpc must exist in order to properly delete the cluster (since we run `kubectl delete`).
+      // the vpc must exist in order to properly delete the cluster (since we run `kubectl delete`).
       // this ensures that.
-      this._clusterResource.node.addDependency(this.kubectlSecurityGroup, this.vpc);
+      this._clusterResource.node.addDependency(this.vpc);
     }
 
     this.adminRole = resource.adminRole;
@@ -951,6 +938,17 @@ export class Cluster extends ClusterBase {
     this.clusterSecurityGroupId = resource.attrClusterSecurityGroupId;
     this.clusterEncryptionConfigKeyArn = resource.attrEncryptionConfigKeyArn;
 
+    const clusterSecurityGroup = ec2.SecurityGroup.fromSecurityGroupId(this, 'ClusterSecurityGroup', this.clusterSecurityGroupId);
+
+    this.connections = new ec2.Connections({
+      securityGroups: [clusterSecurityGroup, securityGroup],
+      defaultPort: ec2.Port.tcp(443), // Control Plane has an HTTPS API
+    });
+
+    // we can use the cluster security group since its already attached to the cluster
+    // and configured to allow connections from itself.
+    this.kubectlSecurityGroup = clusterSecurityGroup;
+
     // use the cluster creation role to issue kubectl commands against the cluster because when the
     // cluster is first created, that's the only role that has "system:masters" permissions
     this.kubectlRole = this.adminRole;
@@ -1595,6 +1593,10 @@ class ImportedCluster extends ClusterBase implements ICluster {
       this.connections.addSecurityGroup(ec2.SecurityGroup.fromSecurityGroupId(this, `SecurityGroup${i}`, sgid));
       i++;
     }
+
+    if (props.clusterSecurityGroupId) {
+      this.connections.addSecurityGroup(ec2.SecurityGroup.fromSecurityGroupId(this, 'ClusterSecurityGroup', props.clusterSecurityGroupId));
+    }
   }
 
   public get vpc() {

diff --git a/packages/@aws-cdk/aws-eks/test/integ.eks-cluster-private-endpoint.expected.json b/packages/@aws-cdk/aws-eks/test/integ.eks-cluster-private-endpoint.expected.json
@@ -581,27 +581,6 @@
         }
       }
     },
-    "ClusterControlPlaneSecurityGroupfromawscdkeksclusterprivateendpointtestClusterKubectlProviderSecurityGroup6A0B729C443DF3A2707": {
-      "Type": "AWS::EC2::SecurityGroupIngress",
-      "Properties": {
-        "IpProtocol": "tcp",
-        "Description": "from awscdkeksclusterprivateendpointtestClusterKubectlProviderSecurityGroup6A0B729C:443",
-        "FromPort": 443,
-        "GroupId": {
-          "Fn::GetAtt": [
-            "ClusterControlPlaneSecurityGroupD274242C",
-            "GroupId"
-          ]
-        },
-        "SourceSecurityGroupId": {
-          "Fn::GetAtt": [
-            "ClusterKubectlProviderSecurityGroup2D90691C",
-            "GroupId"
-          ]
-        },
-        "ToPort": 443
-      }
-    },
     "ClusterCreationRole360249B6": {
       "Type": "AWS::IAM::Role",
       "Properties": {
@@ -630,7 +609,6 @@
         }
       },
       "DependsOn": [
-        "ClusterKubectlProviderSecurityGroup2D90691C",
         "VpcIGWD7BA715C",
         "VpcPrivateSubnet1DefaultRouteBE02A9ED",
         "VpcPrivateSubnet1RouteTableB2C5B500",
@@ -753,7 +731,6 @@
         ]
       },
       "DependsOn": [
-        "ClusterKubectlProviderSecurityGroup2D90691C",
         "VpcIGWD7BA715C",
         "VpcPrivateSubnet1DefaultRouteBE02A9ED",
         "VpcPrivateSubnet1RouteTableB2C5B500",
@@ -844,7 +821,6 @@
         "AttributesRevision": 2
       },
       "DependsOn": [
-        "ClusterKubectlProviderSecurityGroup2D90691C",
         "ClusterCreationRoleDefaultPolicyE8BDFC7B",
         "ClusterCreationRole360249B6",
         "VpcIGWD7BA715C",
@@ -880,22 +856,6 @@
       "UpdateReplacePolicy": "Delete",
       "DeletionPolicy": "Delete"
     },
-    "ClusterKubectlProviderSecurityGroup2D90691C": {
-      "Type": "AWS::EC2::SecurityGroup",
-      "Properties": {
-        "GroupDescription": "Comminication between KubectlProvider and EKS Control Plane",
-        "SecurityGroupEgress": [
-          {
-            "CidrIp": "0.0.0.0/0",
-            "Description": "Allow all outbound traffic by default",
-            "IpProtocol": "-1"
-          }
-        ],
-        "VpcId": {
-          "Ref": "Vpc8378EB38"
-        }
-      }
-    },
     "ClusterKubectlReadyBarrier200052AF": {
       "Type": "AWS::SSM::Parameter",
       "Properties": {
@@ -1168,7 +1128,7 @@
               },
               "/",
               {
-                "Ref": "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20S3Bucket12418C8C"
+                "Ref": "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796S3Bucket39E2BF35"
               },
               "/",
               {
@@ -1178,7 +1138,7 @@
                     "Fn::Split": [
                       "||",
                       {
-                        "Ref": "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20S3VersionKey8C9B24CA"
+                        "Ref": "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796S3VersionKey0218A255"
                       }
                     ]
                   }
@@ -1191,7 +1151,7 @@
                     "Fn::Split": [
                       "||",
                       {
-                        "Ref": "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20S3VersionKey8C9B24CA"
+                        "Ref": "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796S3VersionKey0218A255"
                       }
                     ]
                   }
@@ -1228,10 +1188,10 @@
           "referencetoawscdkeksclusterprivateendpointtestVpcPrivateSubnet3SubnetC47FD39ARef": {
             "Ref": "VpcPrivateSubnet3SubnetF258B56E"
           },
-          "referencetoawscdkeksclusterprivateendpointtestClusterKubectlProviderSecurityGroup67FA4325GroupId": {
+          "referencetoawscdkeksclusterprivateendpointtestClusterF4CF4FE8ClusterSecurityGroupId": {
             "Fn::GetAtt": [
-              "ClusterKubectlProviderSecurityGroup2D90691C",
-              "GroupId"
+              "Cluster9EE0221C",
+              "ClusterSecurityGroupId"
             ]
           },
           "referencetoawscdkeksclusterprivateendpointtestAssetParameters34131c2e554ab57ad3a47fc0a13173a5c2a4b65a7582fe9622277b3d04c8e1e1S3Bucket41FE7429Ref": {
@@ -1335,17 +1295,17 @@
       "Type": "String",
       "Description": "Artifact hash for asset \"570f91ed45d0c45e8ff145969f7499419312e806c83f009b76539ce989960e51\""
     },
-    "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20S3Bucket12418C8C": {
+    "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796S3Bucket39E2BF35": {
       "Type": "String",
-      "Description": "S3 bucket for asset \"daac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20\""
+      "Description": "S3 bucket for asset \"e843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796\""
     },
-    "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20S3VersionKey8C9B24CA": {
+    "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796S3VersionKey0218A255": {
       "Type": "String",
-      "Description": "S3 key for asset version \"daac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20\""
+      "Description": "S3 key for asset version \"e843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796\""
     },
-    "AssetParametersdaac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20ArtifactHash90BA6C4A": {
+    "AssetParameterse843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796ArtifactHash0AFD7EAC": {
       "Type": "String",
-      "Description": "Artifact hash for asset \"daac37af2b50452c854a73ef7e2c57d5229667e390db39773ffb9dfb497bbd20\""
+      "Description": "Artifact hash for asset \"e843c57c7bcb07856b1680280dc9387725661764509856e890ae6e18a5e40796\""
     }
   }
 }