Skip to content

[terraform] Add random variables to terrraform resource names #12385

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

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

nandajavarma
Copy link
Contributor

Description

This is basically the same changes @adrienthebo opened here #12321
The PR was unfortunately merged to a different branch, and this is an effort to split up the PR again to ease the review since it ended up becoming really big.

The main goal of this change is to make sure re-using the same state continues to work. So for starters things like database names cannot be re-used across most cloud providers for a week after the deletion, etc. Hence, the change @adrienthebo suggested adds a random variable to the end of resource names. Moreover, if a kubernetes cluster already exists by the name, the cluster creation is skipped and re-installation of KOTS is proceeded.

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-preview

@nandajavarma nandajavarma requested a review from a team August 25, 2022 11:56
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Aug 25, 2022
@roboquat roboquat added size/XL and removed size/M labels Aug 25, 2022
@nandajavarma nandajavarma changed the base branch from nvn/fix-cron-files to nvn/integ-flag August 25, 2022 14:28
@nandajavarma nandajavarma changed the base branch from nvn/integ-flag to nvn/fix-cron-files August 25, 2022 14:29
@roboquat roboquat added size/M and removed size/XL labels Aug 25, 2022
@nandajavarma nandajavarma force-pushed the nvn/fix-tf-var-names branch 2 times, most recently from 9288929 to a13f1bb Compare August 26, 2022 12:39
@@ -102,6 +110,7 @@ module "eks" {
enable_bootstrap_user_data = true
instance_types = [var.service_machine_type]
name = "service-${var.cluster_name}"
iam_role_name = format("%s-%s", substr("${var.cluster_name}-svc-ng", 0, 58), random_string.ng_role_suffix.result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct in reading that this it to make a string a max of 62 characters long, always ending -svc-ng-<4-random-chars>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is correct. I believe that is the max char limit.

export resource=$$(echo "$$TF_VAR_TEST_ID" | sed "s/[\\W\\-]//") && \
az aks get-credentials --name gitpod-test-nor-primary-$$resource --resource-group gitpod-test-nor-$$resource --file ${KUBECONFIG} || echo "No cluster present"
export resource=$$(echo "$$TF_VAR_TEST_ID" | sed "s/[\\W\\-]//g") && \
az aks get-credentials --name test-cluster-$$resource --resource-group sh-test-$$resource --file ${KUBECONFIG} || echo "No cluster present"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the terraform output command instead? Something like...

Suggested change
az aks get-credentials --name test-cluster-$$resource --resource-group sh-test-$$resource --file ${KUBECONFIG} || echo "No cluster present"
az aks get-credentials --name $(terraform output -json <name> | jq -r '.') --resource-group $(terraform output -json <name> | jq -r '.') --file ${KUBECONFIG} || echo "No cluster present"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would involve changing the AKS terraform module output too. Do you mind if I take this up on a follow-up? I have to refactor the AKS module soon once the reference architecture is out.

Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

/hold

Looks ok. Added a couple of questions, with a "hold" to see if you want to change anything. Remove hold at your convenience

@nandajavarma nandajavarma changed the base branch from nvn/fix-cron-files to main September 1, 2022 06:17
@nandajavarma nandajavarma changed the base branch from main to nvn/fix-cron-files September 1, 2022 06:18
Base automatically changed from nvn/fix-cron-files to main September 1, 2022 07:26
@roboquat roboquat added size/XXL and removed size/M labels Sep 1, 2022
@roboquat roboquat added size/M and removed size/XXL labels Sep 1, 2022
@nandajavarma
Copy link
Contributor Author

/unhold The extraction of name from terraform output will be addresses as a part of this PR #11534

@roboquat roboquat merged commit e249f6b into main Sep 1, 2022
@roboquat roboquat deleted the nvn/fix-tf-var-names branch September 1, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/M team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants