Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support Fargate in private+public EKS environments #1117

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

zimbatm
Copy link
Contributor

@zimbatm zimbatm commented Nov 23, 2020

PR o'clock

Description

The EKS cluster can be provisioned with both private and public subnets.
But Fargate only accepts private ones.

This new variable allows to override the subnets to explicitly pass the
private subnets to Fargate and work around that issue.

Checklist

@zimbatm zimbatm changed the title add fargate_subnets variable feat: support Fargate in private+public EKS environments Nov 23, 2020
@aittam
Copy link

aittam commented Nov 26, 2020

Shouldn't be better to pass the fargate subnets directly to the fargate_profiles instead of creating a new variable?

something like this:

    fargate_profiles = {
      example = {
        namespace = "default"
        subnets = ["subnet-0ca3e3d1234a56c78"]
      }
    }

The patch for the module would be similar to yours:
subnets = each.value.subnets != null ? each.value.subnets : var.subnets

@zimbatm
Copy link
Contributor Author

zimbatm commented Nov 26, 2020

@aittam like that?

@aittam
Copy link

aittam commented Nov 26, 2020

@aittam like that?

nope, I just sent you a PR on your forked repo on how I intended it. I have tested it and it works. Look at the examples/fargate/main.tf file on how to use it.

@zimbatm
Copy link
Contributor Author

zimbatm commented Nov 27, 2020

merged

@zimbatm
Copy link
Contributor Author

zimbatm commented Nov 28, 2020

fixed the formatting issue

@zimbatm
Copy link
Contributor Author

zimbatm commented Dec 7, 2020

rebased and re-formatted with Terraform 0.14

@zimbatm
Copy link
Contributor Author

zimbatm commented Dec 7, 2020

Terraform 0.14 has been released and the formatter is not stable. So I pinned the version to 0.12.29 instead.

@RenGeng
Copy link

RenGeng commented Jan 11, 2021

Any update on this PR ?

@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 11, 2021

rebased again. That PR was ready but rotted a bit.

The EKS cluster can be provisioned with both private and public subnets.
But Fargate only accepts private ones.

This new variable allows to override the subnets to explicitly pass the
private subnets to Fargate and work around that issue.
Copy link

@TBeijen TBeijen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not a repo owner). Looks concise and complete to me.

@TBeijen
Copy link

TBeijen commented Jan 22, 2021

Just tested this, specifying only the private subnets of the cluster in the fargate_profile. Works as intended.

Looking at the lookup part I'm pretty confident the default behaviour is properly preserved as well.

Edit: Also tested not specifying subnets in profile. The it correctly takes subnets var from parent module (as was always the case).

Copy link

@aittam aittam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as intended, I am using it in a real word project.

Copy link
Member

@barryib barryib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zimbatm

@barryib barryib merged commit 576aa29 into terraform-aws-modules:master Jan 28, 2021
ArchiFleKs pushed a commit to ArchiFleKs/terraform-aws-eks that referenced this pull request Apr 16, 2021
…aws-modules#1117)

NOTES: The EKS cluster can be provisioned with both private and public subnets. But Fargate only accepts private ones. This new variable allows to override the subnets to explicitly pass the private subnets to Fargate and work around that issue.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants