Skip to content

Commit

Permalink
fix: only setup LB discovery tags for in-use subnets (#147)
Browse files Browse the repository at this point in the history
<!--
  ~ Copyright 2023 StreamNative, Inc.
  ~
  ~ Licensed under the Apache License, Version 2.0 (the "License");
  ~ you may not use this file except in compliance with the License.
  ~ You may obtain a copy of the License at
  ~
  ~     http://www.apache.org/licenses/LICENSE-2.0
  ~
  ~ Unless required by applicable law or agreed to in writing, software
  ~ distributed under the License is distributed on an "AS IS" BASIS,
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
  ~ See the License for the specific language governing permissions and
  ~ limitations under the License.
-->

<!--
### Contribution Checklist

- Fill out the template below to describe the changes contributed by the
pull request. That will give reviewers the context they need to do the
review.
  
- Each pull request should address only one issue, not mix up code from
multiple issues.
  
  - Each commit in the pull request has a meaningful commit message

- Once all items of the checklist are addressed, remove the above text
and this checklist, leaving only the filled out template below.

**(The sections below can be removed for hotfixes of typos)**
-->

*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*

Fixes #<xyz>

*(or if this PR is one task of a github issue, please add `Master Issue:
#<xyz>` to link to the master issue.)*

Master Issue: #<xyz>

### Motivation
- Avoid unnecessary inter-zone traffic from LB
- Allow disabling cross-zone load balancing when specifying zones

### Modifications
- Remove duplicated tag setup in VPC module
- Only setup LB discovery tags for subnet will be used by nodepool

### Verifying this change

- [ ] Make sure that the change passes the CI checks.

*(Please pick either of the following options)*

This change is a trivial rework / code cleanup without any test
coverage.

*(or)*

This change is already covered by existing tests, such as *(please
describe tests)*.

*(or)*

This change added tests and can be verified as follows:

*(example:)*
- *Added integration tests for end-to-end deployment with large payloads
(10MB)*
  - *Extended integration test for recovery after broker failure*

### Documentation

Check the box below.

Need to update docs? 

- [ ] `doc-required` 
  
  (If you need help on updating docs, create a doc issue)
  
- [x] `no-need-doc` 
  
  (Please explain why)
  
- [ ] `doc` 
  
  (If this PR contains doc changes)
  • Loading branch information
ciiiii authored Nov 25, 2024
1 parent c7dbedf commit 7bbcbae
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 6 deletions.
1 change: 1 addition & 0 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ module "vpc_tags" {
vpc_id = var.vpc_id
public_subnet_ids = var.public_subnet_ids
private_subnet_ids = var.private_subnet_ids
node_pool_azs = var.node_pool_azs
}

resource "aws_ec2_tag" "cluster_security_group" {
Expand Down
24 changes: 20 additions & 4 deletions modules/eks-vpc-tags/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,24 @@
# See the License for the specific language governing permissions and
# limitations under the License.

data "aws_subnet" "private_subnets" {
count = length(var.private_subnet_ids)
id = var.private_subnet_ids[count.index]
}

data "aws_subnet" "public_subnets" {
count = length(var.public_subnet_ids)
id = var.public_subnet_ids[count.index]
}

locals {
cluster_subnet_ids = concat(var.private_subnet_ids, var.public_subnet_ids)
node_pool_private_subnets = [
for subnet in data.aws_subnet.private_subnets : subnet.id if(length(var.node_pool_azs) == 0 || contains(var.node_pool_azs, subnet.availability_zone))
]
node_pool_public_subnets = [
for subnet in data.aws_subnet.public_subnets : subnet.id if(length(var.node_pool_azs) == 0 || contains(var.node_pool_azs, subnet.availability_zone))
]
}

resource "aws_ec2_tag" "vpc_tag" {
Expand All @@ -30,15 +46,15 @@ resource "aws_ec2_tag" "cluster_subnet_tag" {
}

resource "aws_ec2_tag" "private_subnet_tag" {
count = length(var.private_subnet_ids)
resource_id = var.private_subnet_ids[count.index]
count = length(local.node_pool_private_subnets)
resource_id = local.node_pool_private_subnets[count.index]
key = "kubernetes.io/role/internal-elb"
value = "1"
}

resource "aws_ec2_tag" "public_subnet_tag" {
count = length(var.public_subnet_ids)
resource_id = var.public_subnet_ids[count.index]
count = length(local.node_pool_public_subnets)
resource_id = local.node_pool_public_subnets[count.index]
key = "kubernetes.io/role/elb"
value = "1"
}
6 changes: 6 additions & 0 deletions modules/eks-vpc-tags/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,10 @@ variable "vpc_id" {
condition = length(var.vpc_id) > 4 && substr(var.vpc_id, 0, 4) == "vpc-"
error_message = "The value for variable \"vpc_id\" must be a valid VPC id, starting with \"vpc-\"."
}
}

variable "node_pool_azs" {
type = list(string)
description = "A list of availability zones to use for the EKS node group. If not set, the module will use the same availability zones with the cluster."
default = []
}
4 changes: 2 additions & 2 deletions modules/vpc/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ resource "aws_subnet" "public" {
cidr_block = cidrsubnet(var.vpc_cidr, var.public_subnet_newbits, var.public_subnet_start + count.index)
availability_zone = local.azs[count.index]
map_public_ip_on_launch = var.disable_nat_gateway ? true : var.public_subnet_auto_ip
tags = merge({ "Vendor" = "StreamNative", "Type" = "public", "kubernetes.io/role/elb" = "1", Name = format("%s-public-sbn-%s", var.vpc_name, count.index) }, var.tags)
tags = merge({ "Vendor" = "StreamNative", "Type" = "public", Name = format("%s-public-sbn-%s", var.vpc_name, count.index) }, var.tags)

lifecycle {
ignore_changes = [tags]
Expand All @@ -52,7 +52,7 @@ resource "aws_subnet" "private" {
vpc_id = aws_vpc.vpc.id
cidr_block = cidrsubnet(var.vpc_cidr, var.private_subnet_newbits, var.private_subnet_start + count.index)
availability_zone = local.azs[count.index]
tags = merge({ "Vendor" = "StreamNative", "Type" = "private", "kubernetes.io/role/internal-elb" = "1", Name = format("%s-private-sbn-%s", var.vpc_name, count.index) }, var.tags)
tags = merge({ "Vendor" = "StreamNative", "Type" = "private", Name = format("%s-private-sbn-%s", var.vpc_name, count.index) }, var.tags)

lifecycle {
ignore_changes = [tags]
Expand Down

0 comments on commit 7bbcbae

Please sign in to comment.