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

[terraform] Make AWS certificate manager optional #12253

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

nandajavarma
Copy link
Contributor

@nandajavarma nandajavarma commented Aug 22, 2022

Description

In the EWS terraform module we are by default creating AWS cert manager entries, which are not essentially needed since by default our reference architecture suggests usage of cert-manager. Hence this PR makes the creation of AWS cert manager optional using the flag user_aws_cert_manager.

As of the terraform module corresponding the reference architecture, in a future PR, I will provide an option to use aws cert manager if the user would rather use that instead of cert-manager in which case, in the output, we will provide config patch necessary to post process the service annotation to use ACM.

Related Issue(s)

Fixes #

How to test

Following the README for the terraform reference architecture, you will now see that the unnecessary ACM creation does not happen anymore.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@nandajavarma nandajavarma changed the title [terraform] remove AWS certificate manager for now [terraform] Make AWS certificate manager optional Aug 22, 2022
@nandajavarma nandajavarma marked this pull request as ready for review August 22, 2022 10:14
@nandajavarma nandajavarma requested a review from a team August 22, 2022 10:14
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Aug 22, 2022
@mrsimonemms
Copy link
Contributor

LGTM

/hold in case you want to change to my suggestion


It might be worth creating a locals.tf file in modules/eks to avoid having to write count = var.domain_name == "" || var.use_aws_cert_manager == false ? 0 : 1 and count = var.domain_name == "" ? 0 : 1 so many times.

It could look something like this:

locals {
  cert_manager_enabled : local.domain_name_enabled && var.use_aws_cert_manager == true
  cert_manager_count : local.cert_manager_enabled ? 1 : 0
  domain_name_enabled : var.domain_name != ""
  domain_name_count : local.domain_name_enabled ? 1 : 0
}

Then you could change all the count references to:

resource "aws_iam_access_key" "edns" {
  count = local.domain_name_count
}

resource "aws_acm_certificate" "gitpod" {
  count = local.cert_manager_count
}

NB I wrote this entirely in this comment box so the syntax is probably buggy/wrong, but it should be enough for you to get started. We can always chat if you want more help with this

@nandajavarma
Copy link
Contributor Author

Great tip, Simon! I have updated the dns.tf files with the local vars you suggested. There are other files with similar conditions, I will replace them in another PR with local vars. Merging this after the test here passes https://werft.gitpod-dev.com/job/gitpod-custom-nvn-remove-aws-acm.0

@nandajavarma
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 091c8d2 into main Aug 23, 2022
@roboquat roboquat deleted the nvn/remove-aws-acm branch August 23, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/L team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants