-
Notifications
You must be signed in to change notification settings - Fork 42
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
[Create Environment] Adding ec module #1742
Conversation
This pull request does not have a backport label. Could you fix it @gurevichdmitry? 🙏
|
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.
Nice!
# Check if the version contains a commit hash, remove it | ||
if [[ $stack_version =~ -[a-f0-9]+ ]]; then | ||
cleaned_version=$(echo $stack_version | awk -F"-" '{print $1}') | ||
echo "STACK_VERSION=$cleaned_version" >> $GITHUB_ENV |
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.
Remove it's declaration on line 115
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.
Updates the declaration to support the working of the entire flow retroactively. The version with the hash is only required for Terraform to pin the version of the stack to be deployed.
es_docker_image = lookup(var.docker_image, "elasticsearch", "") | ||
es_docker_image_tag_override = lookup(var.docker_image_tag_override, "elasticsearch", "") | ||
kibana_docker_image = lookup(var.docker_image, "kibana", "") | ||
kibana_docker_image_tag_override = lookup(var.docker_image_tag_override, "kibana", "") | ||
apm_docker_image = lookup(var.docker_image, "apm", "") | ||
apm_docker_image_tag_override = lookup(var.docker_image_tag_override, "apm", "") |
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.
Do we really have a usecase of overriding these?
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.
The main use case is to support pinned BC versions. As described in this task, if we provide just the version (e.g., 8.12.0) and a new BC candidate is released, the stack will be updated with the newest version underneath. To prevent that, we should provide the exact version to be deployed. Unfortunately, the ec provider
does not support the direct deployment of versions with a hash. To work around this, we need to override the docker_image for all stack components (ElasticSearch, Kibana, and Integrations Server). In the test environments' main.tf,
we provide docker_image_tag_override
, which is then passed to the ec module
and used to deploy the custom image.
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.
why do we even need the apm component?
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.
The default manual
deployment on the cloud includes the Integrations server
component.
We are installing the Integrations server
component, which was introduced in version 8.x and consists of APM and Fleet Server. Fleet is necessary to centrally manage all agents, integrations, and APM is part of Integrations server component.
description = "The Elasticsearch password" | ||
} | ||
|
||
output "stack_version" { |
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.
Nice, we should start using it instead of the "requested" version.
"elasticsearch" = "", | ||
"kibana" = "", | ||
"apm" = "", |
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.
Do we need those?
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.
Yes, each stack component may use a different image tag
deploy/test-environments/main.tf
Outdated
region = var.ess_region | ||
stack_version = var.stack_version | ||
stack_version = local.cleaned_version |
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.
So we don't even deploy the requested version? Where do we use the commit sha?
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.
In the case of a released or snapshot version, stack_version
will not be modified and remains the same. However, in the case of a BC version, we need to remove the hash from stack_version
. This is essential because the ec
provider might fail with an unknown version if the hash is included. Additionally, we need to override docker_image_tag
. Instead of creating complex logic to handle different scenarios, we always provide docker_image_override
in all cases to ensure that our deployment deploys the pinned version.
stack_version="${{ inputs.elk-stack-version }}" | ||
|
||
# Check if the version contains a commit hash, remove it | ||
if [[ $stack_version =~ -[a-f0-9]+ ]]; then |
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 that we also clean that on the terraform? Where do we use the full version?
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.
For Terraform, we are utilizing the TF_VAR_stack_version: ${{ inputs.elk-stack-version }}
environment variable, which is passed as is. Full version is used to deploy pinned BC version.
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.
A few more comments to your consideration
@@ -152,6 +152,18 @@ jobs: | |||
echo "::add-mask::$ec_api_key" | |||
echo "TF_VAR_ec_api_key=$ec_api_key" >> $GITHUB_ENV | |||
|
|||
- name: Process BC version |
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 consider getting the hash on a different optional parameter, PIN_VERSION
for example.
This way we will avoid the cleaning code implemented both in bash and terraform, and the flow of using the pinned version will be more straight forward PIN_VERSION != ""
.
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.
@amirbenun I have refactored the code to support a separate pin_version
Terraform variable. This eliminates the need for cleanup version in Terraform code. I've updated the step action responsible for initializing and updating TF_vars
and provided comments for clarity. Additionally, I have improved the documentation to make it more clear.
|
||
# Docker image overrides | ||
|
||
variable "docker_image_tag_override" { |
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.
Please add comments on why those are needed especially when deploying BC versions.
-var="project=${{ github.actor }}" \ | ||
-var="owner=${{ github.actor }}" |
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.
💯
- name: Process BC version | ||
id: clean-version | ||
run: | | ||
# Extract the stack version | ||
stack_version="${{ inputs.target-elk-stack-version }}" | ||
|
||
# Check if the version contains a commit hash, remove it | ||
if [[ $stack_version =~ -[a-f0-9]+ ]]; then | ||
cleaned_version=$(echo $stack_version | awk -F"-" '{print $1}') | ||
echo "AGENT_VERSION=$cleaned_version" >> $GITHUB_OUTPUT | ||
else | ||
echo "AGENT_VERSION=$stack_version" >> $GITHUB_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.
if we reuse this part maybe we can export it to a script or to an action
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.
The common part is just to retrieve cleaned_version
. Afterwards, the handling is different. In this action, we are adding AGENT_VERSION
to GITHUB_OUTPUT
(different jobs), and in the test-environments
step, we are updating env vars of the same job.
Summary of your changes
This PR introduces a new internal EC module that deploys the basic ELK stack. Additionally, the ability to override the image has been implemented as part of this change.
Screenshot/Data
Related Issues
Checklist
Introducing a new rule?