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

[installer] Add EKS installer test #10709

Merged
merged 3 commits into from
Jul 1, 2022
Merged

[installer] Add EKS installer test #10709

merged 3 commits into from
Jul 1, 2022

Conversation

nandajavarma
Copy link
Contributor

@nandajavarma nandajavarma commented Jun 16, 2022

Description

This PR adds the EKS terraform module for testing purposes to the repo. These are the most important updates:

  • The EKS terraform module added to infra directory - the security groups are at the moment very permissive. This can be fixed as a part of the epic to make these tf module production ready if need be.
  • changes to installer-test.ts, Makefile and main.tf to accommodate new EKS test configuration
  • eks-installer-test.yml to trigger the cronjob for this configuration
  • standardizing the external-dns and issuer modules to be use used for both azure and aws. This involves:
    • changing the external_dns_settings output from aks module to support dynamic setting in external-dns tf module.
    • renaming module external-dns to cloud-dns-external-dns(since this is only for cloud-dns - this will be cleaned up more in a future PR), renaming azure-external-dns to external-dns since this will eventually be standard across all providers
  • Added extra KOTS configs to support aws dependencies (storage, registry, db)
  • Updating the output.tf in the test directory to optionally get aws or azure outputs (this will be expanding to GCP) in a later PR

Related Issue(s)

Fixes #

How to test

To test this, get authentication to core-dev cluster on GCP. Then run:

werft run github -f -s .werft/installer-tests.ts -j .werft/eks-installer-tests.yaml -a debug=true

Release Notes

NONE

Documentation

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-nvn-infra-eks.15 because the annotations in the pull request description changed
(with .werft/ from main)

@nandajavarma nandajavarma force-pushed the nvn/infra-eks branch 2 times, most recently from b9828ea to 518ee22 Compare June 17, 2022 07:10
@adrienthebo adrienthebo added the team: delivery Issue belongs to the self-hosted team label Jun 20, 2022
@nandajavarma nandajavarma force-pushed the nvn/infra-aks branch 7 times, most recently from 10a5b0c to 8006bc7 Compare June 22, 2022 09:01
@roboquat roboquat added size/XXL and removed size/XL labels Jun 22, 2022
@nandajavarma nandajavarma changed the base branch from nvn/infra-aks to nvn/infra-aks-deps June 22, 2022 17:52
@nandajavarma nandajavarma force-pushed the nvn/infra-eks branch 2 times, most recently from 6a096d5 to 84e477a Compare June 23, 2022 09:47
@nandajavarma nandajavarma force-pushed the nvn/infra-eks branch 4 times, most recently from ca8985f to 34dd1c4 Compare June 29, 2022 15:39
@nandajavarma nandajavarma force-pushed the nvn/infra-eks branch 2 times, most recently from dfe6d7f to 9833a95 Compare June 30, 2022 11:11
Base automatically changed from nvn/infra-aks-deps to main June 30, 2022 12:16
@adrienthebo
Copy link
Contributor

This PR is pending approval, I've added the hold tag so that feedback can be addressed if desired. @nandajavarma you're good to merge this at your discretion!

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Well, blast; I had a long, throughful approval message and GitHub ate it. Here's the short version!

This is really great work, and I see the effort that you put in! This'll be really helpful for improving our developer velocity and solidifying the self hosted releases.

In short, we can ship this as is. I see some risks, outlined below, but I'm approving this and will leave any changes up to your discretion, @nandajavarma.


The following items can be optionally extracted to separate issues. Since I'm asking for the work I can pick up the issue creation and contribute fixes; let me know if you'd like support here.

Blockers

None!

Risks/recommendations

  • Hardcoded database passwords: this is the kind of thing that could bite us down the road. We can hopefully drop in a terraform random_password resource to solve this. Would prefer if we do that before merging, can address that afterwards.
  • VPC ingress/egress rules: our customers are going to have more restrictive VPC rules; we'll need to prove out what's actually needed for everything to work.

Suggestions

  • Factor our terraform apply -target: we're leaning on this feature pretty heavily and it's meant to be a niche behavior. We're likely to stretch this a little too far in the foreseeable future. We should definitely handle this one later but let's keep an eye on removing the need for targeted applies.

Small fixups

I'm still a walking spell checker 😅 Any fixups for spelling or small details are opportunistic fixes and don't impact correctness or behavior. Adjust as you see fit but don't sweat them!


Again, thanks for all of the hard work!

.werft/eks-installer-tests.yaml Outdated Show resolved Hide resolved
install/infra/terraform/eks/database.tf Outdated Show resolved Hide resolved
.werft/eks-installer-tests.yaml Outdated Show resolved Hide resolved
install/infra/terraform/eks/database.tf Show resolved Hide resolved
install/infra/terraform/eks/kubernetes.tf Show resolved Hide resolved
install/infra/terraform/eks/output.tf Outdated Show resolved Hide resolved
install/infra/terraform/eks/variables.tf Outdated Show resolved Hide resolved
install/tests/output.tf Show resolved Hide resolved
install/tests/Makefile Show resolved Hide resolved
install/infra/terraform/eks/dns.tf Outdated Show resolved Hide resolved
@nandajavarma
Copy link
Contributor Author

Updates made:

  • Removed unnecessary secret mount on eks-installer-tests.yaml
  • Fixed a couple of typos
  • Using random password for database
  • set var-region insterad of hardcoded region in output
  • increased ttl for dns zone
  • dropped harcoded default cluster_name for EKS

@nandajavarma
Copy link
Contributor Author

/unhold

Letting this one in since all the review suggestions are acknowledged.

@roboquat roboquat merged commit 1690eee into main Jul 1, 2022
@roboquat roboquat deleted the nvn/infra-eks branch July 1, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/XXL team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants