-
Notifications
You must be signed in to change notification settings - Fork 3
ROSA automation #49
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
base: main
Are you sure you want to change the base?
ROSA automation #49
Conversation
|
Please remove the two pipeline runs in the .tekton folder. |
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
|
Thanks for your progress on this. After reviewing, I think we need to pivot our approach on the IaC tool. Given our broader strategy involving a hybrid cloud, Terraform is much better suited for our needs than CloudFormation. Could you please switch the implementation from CloudFormation to Terraform? Once the foundation is in Terraform, we can proceed with adding the following services:
Happy to sync up to discuss the implementation. Let me know your thoughts. |
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
@mjiao I've added Terraform as the IaC tool, would appreciate your review. |
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
| @@ -0,0 +1,572 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EFS can also be managed via terraform https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/efs_file_system
You might need to add further configuration to make it usable in the openshift.
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
|
@mjiao please review the latest changes, which have end-to-end Terraform integration. FYI, the EFS has still not been integrated completely. I'm in the process of testing that. |
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
|
as we now have terraform code I suggest to run terraform linter in CI/CD, here is what tflint output at the moment tflint
5 issue(s) found:
Warning: [Fixable] Comparing a collection with an empty list is invalid. To detect an empty collection, check its length. (terraform_empty_list_equali
ty)
on rosa-hcp.tf line 24:
24: aws_availability_zones = var.availability_zones != [] ? var.availability_zones : []
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.12.0/docs/rules/terraform_empty_list_equality.md
Warning: [Fixable] variable "channel_group" is declared but not used (terraform_unused_declarations)
on variables.tf line 102:
102: variable "channel_group" {
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.12.0/docs/rules/terraform_unused_declarations.md
Warning: [Fixable] variable "enable_autoscaling" is declared but not used (terraform_unused_declarations)
on variables.tf line 200:
200: variable "enable_autoscaling" {
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.12.0/docs/rules/terraform_unused_declarations.md
Warning: [Fixable] variable "min_replicas" is declared but not used (terraform_unused_declarations)
on variables.tf line 206:
206: variable "min_replicas" {
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.12.0/docs/rules/terraform_unused_declarations.md
Warning: [Fixable] variable "max_replicas" is declared but not used (terraform_unused_declarations)
on variables.tf line 212:
212: variable "max_replicas" {
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.12.0/docs/rules/terraform_unused_declarations.md
|
rosa.makefile
Outdated
| rosa-login: ## Login using ROSA token | ||
| $(call required-environment-variables,ROSA_TOKEN) | ||
| @rosa login --token="${ROSA_TOKEN}" | ||
| @rosa create account-roles --mode auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe separate makefile target?
rosa.makefile
Outdated
| .PHONY: rosa-domain-zone-exists | ||
| rosa-domain-zone-exists: ## Fail if Route53 hosted zone does not exist | ||
| $(call required-environment-variables,ROSA_DOMAIN) | ||
| ROSA_DOMAIN=${ROSA_DOMAIN} rosa/rosa-domain-zone-exists.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROSA_DOMAIN is already environment variable, no need to reassign it.
rosa.makefile
Outdated
| ROSA_DOMAIN=${ROSA_DOMAIN} rosa/rosa-domain-zone-exists.sh | ||
|
|
||
| .PHONY: rosa-deploy | ||
| rosa-deploy: rosa-domain-zone-exists rosa-network-deploy rosa-account-roles rosa-cluster rosa-operator-roles rosa-oidc-provider ## Deploy ROSA cluster with all dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rosa account roles will not be created here, because rosa-login is not in dependencies list, one more reason to have it as separate makefile target
rosa/rosa-domain-records.sh
Outdated
| # deploys the appropriate CloudFormation stack (A-records or CNAMEs) under your domain. | ||
|
|
||
| # shellcheck disable=SC2059 | ||
| set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need for if statements below to check status if 'euo' is set. Script will fail as soon as any command will fail.
rosa/rosa-domain-records.sh
Outdated
| # shellcheck disable=SC2059 | ||
| set -euo pipefail | ||
|
|
||
| DOMAIN="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see domain is required below, no need to set it to empty string here, I think
| @echo "Destroying ROSA HCP cluster using Terraform..." | ||
| @cd rosa/terraform && \ | ||
| terraform destroy \ | ||
| -var="rosa_token=${ROSA_TOKEN}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose to set secret parameters via env variables
export TF_VAR_rosa_token="$ROSA_TOKEN"This guarantees that we will not forget some @ at the begining of the command and expose secret in the log
rosa.makefile
Outdated
| echo 'aws_region = "${ROSA_REGION}"' > network.tfvars && \ | ||
| echo 'cluster_name = "${CLUSTER_NAME}"' >> network.tfvars && \ | ||
| echo 'vpc_name = "rosa-${CLUSTER_NAME}-vpc"' >> network.tfvars && \ | ||
| echo 'environment_tag = "rosa"' >> network.tfvars && \ | ||
| echo 'vpc_cidr = "10.0.0.0/16"' >> network.tfvars && \ | ||
| echo 'public_subnet_1_cidr = "10.0.0.0/24"' >> network.tfvars && \ | ||
| echo 'public_subnet_2_cidr = "10.0.1.0/24"' >> network.tfvars && \ | ||
| echo 'public_subnet_3_cidr = "10.0.4.0/24"' >> network.tfvars && \ | ||
| echo 'private_subnet_1_cidr = "10.0.2.0/24"' >> network.tfvars && \ | ||
| echo 'private_subnet_2_cidr = "10.0.3.0/24"' >> network.tfvars && \ | ||
| echo 'private_subnet_3_cidr = "10.0.5.0/24"' >> network.tfvars && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you are just creating a file. Makefile targets are perfect for this. So we should have makefile target rosa/terraform/network.tfvars
Second point - there is command envsubst that uses template file and substites environment variables with their values to create new file. For instance:
envsubst < rosa/terraform/network.tfvars.envsubst > rosa/terraform/network.tfvars
| echo 'private_subnet_1_cidr = "10.0.2.0/24"' >> network.tfvars && \ | ||
| echo 'private_subnet_2_cidr = "10.0.3.0/24"' >> network.tfvars && \ | ||
| echo 'private_subnet_3_cidr = "10.0.5.0/24"' >> network.tfvars && \ | ||
| terraform plan -var-file=network.tfvars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a separate target, its prerequsite is that network.tfvars file is present.
| echo 'private_subnet_3_cidr = "10.0.5.0/24"' >> network.tfvars && \ | ||
| terraform plan -var-file=network.tfvars | ||
|
|
||
| .PHONY: rosa-terraform-output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a way to run any command with make targets. I will show you example separately, so you can adopt it to this use case.
Signed-off-by: RishabhKodes <rishabhbhandari6@gmail.com>
| # availability_zones = ["eu-central-1a", "eu-central-1b", "eu-central-1c"] | ||
|
|
||
| # VPC Configuration (when create_vpc = true) | ||
| vpc_name = "rosa-sapeic-vpc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls make sure vpc name is derived from cluster_name, e.g. vpc_name = "rosa-${var.cluster_name}-vpc"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjiao the vpc_name is already being set as rosa-${var.cluster_name}-vpc here https://github.com/redhat-sap/sap-edge/blob/rosa/rosa.makefile#L113, I also checked in the AWS console it is being set correctly.
Nevertheless, I have updated the code in terraform.tfvars.example as well, ensuring that it does not create any confusion.
| @@ -0,0 +1,244 @@ | |||
| # SPDX-FileCopyrightText: 2024 SAP edge team | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running terraform apply multiple times on this ROSA HCP deployment, Terraform detects
"drift" because ROSA/OpenShift automatically adds tags to subnets (like
kubernetes.io/cluster/, api.openshift.com/, etc.) that aren't managed in the Terraform
configuration.
Could you pls fix this?
Added the scripts for creating and managing ROSA clusters.