Review buildkite scripts cleanup process #1487

mrodm · 2023-10-04T11:06:53Z

No description provided.

This reverts commit f25127f.

mrodm · 2023-10-05T10:53:45Z

.buildkite/scripts/integration_tests.sh

@@ -87,12 +95,11 @@ upload_safe_logs() {
        return
    fi

-    local gsUtilLocation=$(google_cloud_auth_safe_logs)
+    google_cloud_auth_safe_logs


Calling this function as $(google_cloud_auth_safe_logs) make that the variable that was exported in that function (GOOGLE_APPLICATION_CREDENTIALS) was not available for the rest of the function. It seems that it creates a subshell and that variable does not exist outside of it.

mrodm · 2023-10-05T10:54:13Z

.buildkite/scripts/integration_tests.sh

@@ -67,14 +77,12 @@ if [[ "${TARGET}" == "" ]]; then
 fi

 google_cloud_auth_safe_logs() {
-    local gsUtilLocation=$(mktemp -d -p . -t ${TMP_FOLDER_TEMPLATE})
+    local gsUtilLocation=$(mktemp -d -p ${WORKSPACE} -t ${TMP_FOLDER_TEMPLATE})


Ensure that the temporal folder is created under WORKSPACE

mrodm · 2023-10-05T10:55:00Z

.buildkite/scripts/integration_tests.sh

 cleanup() {
+    local error_code=$?
+
+    if [ $error_code != 0 ] ; then
+        if [ -f ${GOOGLE_APPLICATION_CREDENTIALS+x} ]; then
+             google_cloud_logout_active_account
+        fi
+    fi
+
    echo "Deleting temporal files..."
    cd ${WORKSPACE}
    rm -rf "${TMP_FOLDER_TEMPLATE_BASE}.*"
    echo "Done."
+
+    exit $error_code
 }
 trap cleanup EXIT


I didn't want to move this function to tooling.sh to avoid more refactors in this PR.

Each pipeline (script) has its own cleanup and the one from release pipeline is different.

The command trap cleanup EXIT will run every time before exit.
But the variable GOOGLE_APPLICATION_CREDENTIALS can be not defined yet, for example, when the script finishes with an error before google_cloud_auth* func. Perhaps, in this case, we have to look for the google-cloud-credentials.json file in the ${WORKSPACE}/* folder and subfolders instead of using the variable.
@mrodm WDYT?

I tried to take into account this by using this parameter expansion ${var+x}

if [ -n "${GOOGLE_APPLICATION_CREDENTIALS+x}" ]; then

In that case, if the variable is not defined the logout is not executed.

I think that it could be a good approach, since that variable is set by the google_cloud_auth just after the login command

elastic-package/.buildkite/scripts/tooling.sh

Lines 23 to 25 in f1b90c7

gcloud auth activate-service-account --key-file ${keyFile} 2> /dev/null

export GOOGLE_APPLICATION_CREDENTIALS=${secretFileLocation}

In this commit I added two conditions that the variable is defined and the file pointing that variable exists. Maybe it can be just leave that the variable is defined. If the variable is defined, run the logout function

@sharbuz WDYT ?

mrodm · 2023-10-05T10:55:22Z

.buildkite/scripts/integration_tests.sh

+    if [ $error_code != 0 ] ; then
+        if [ -f ${GOOGLE_APPLICATION_CREDENTIALS+x} ]; then
+             google_cloud_logout_active_account
+        fi
+    fi


Ensure that in case of failure, the active account is logged out.

perfect! we check the variable and if it exists we check the file!

sharbuz · 2023-10-11T05:22:55Z

.buildkite/hooks/pre-exit

+    unset ELASTIC_PACKAGE_AWS_ACCESS_KEY
+    unset ELASTIC_PACKAGE_AWS_SECRET_KEY
+    unset AWS_ACCESS_KEY_ID
+    unset AWS_SECRET_ACCESS_KEY


Can we update the unset_secrets func to unset _KEY and _KEY_ID secrets to exclude whole this condition in the pre-exit hook?

Also, if we already have a great cleanup func that checks the current GCP connection I suppose we can add running of this func in the pre-exit hook right after unset_secrets to avoid plenty of conditions with gcp_logout* calls.
WDYT?

look at the example: https://github.com/elastic/package-storage-infra/pull/570
refactored cleanup and unset_secrets funcs in the common.sh and pre-exit hook.

Can we update the unset_secrets func to unset _KEY and _KEY_ID secrets to exclude whole this condition in the pre-exit hook?

It's not a good idea :) I've checked it. in this case, we will unset all BUILDKITE_*_KEY and similar variables needed for the pipeline.

This unset_secrets function is the same as in other repositories, and I didn't want to deviate much from the other usages in case it could be moved to a shared location.

sharbuz · 2023-10-11T05:27:13Z

.buildkite/scripts/signAndPublishPackage.sh

+    local error_code=$?
+
+    if [ $error_code != 0 ] ; then
+        # if variable is defined, run the logout
+        if [ -n "${GOOGLE_APPLICATION_CREDENTIALS+x}" ]; then
+             google_cloud_logout_active_account
+        fi
+    fi
+


Perhaps it's better to move the cleanup func into tooling.sh script to avoid duplicating the code in the signAndPublishPackage.sh and integration_tests.sh. @mrodm WDYT?

I wanted to avoid that in this PR at least, I added a comment about this in this other thread:
#1487 (comment)

sharbuz · 2023-10-11T05:41:17Z

.buildkite/hooks/pre-exit

+    unset ELASTIC_PACKAGE_AWS_ACCESS_KEY
+    unset ELASTIC_PACKAGE_AWS_SECRET_KEY
+    unset AWS_ACCESS_KEY_ID
+    unset AWS_SECRET_ACCESS_KEY


Also, if we already have a great cleanup func that checks the current GCP connection I suppose we can add running of this func in the pre-exit hook right after unset_secrets to avoid plenty of conditions with gcp_logout* calls.
WDYT?

sharbuz · 2023-10-11T05:52:57Z

.buildkite/hooks/pre-exit

+    unset ELASTIC_PACKAGE_AWS_ACCESS_KEY
+    unset ELASTIC_PACKAGE_AWS_SECRET_KEY
+    unset AWS_ACCESS_KEY_ID
+    unset AWS_SECRET_ACCESS_KEY


look at the example: https://github.com/elastic/package-storage-infra/pull/570
refactored cleanup and unset_secrets funcs in the common.sh and pre-exit hook.

sharbuz · 2023-10-11T06:03:54Z

.buildkite/hooks/pre-exit

+    unset ELASTIC_PACKAGE_AWS_ACCESS_KEY
+    unset ELASTIC_PACKAGE_AWS_SECRET_KEY
+    unset AWS_ACCESS_KEY_ID
+    unset AWS_SECRET_ACCESS_KEY


Can we update the unset_secrets func to unset _KEY and _KEY_ID secrets to exclude whole this condition in the pre-exit hook?

It's not a good idea :) I've checked it. in this case, we will unset all BUILDKITE_*_KEY and similar variables needed for the pipeline.

.buildkite/scripts/tooling.sh

Co-authored-by: sharbuz <87968844+sharbuz@users.noreply.github.com>

mrodm · 2023-10-11T13:49:46Z

/test

mrodm · 2023-10-11T15:29:44Z

/test

mrodm · 2023-10-11T16:02:52Z

/test

mrodm · 2023-10-11T18:11:17Z

/test

mrodm · 2023-10-16T16:43:31Z

/test

…ripts

jsoriano · 2023-10-19T08:23:58Z

.buildkite/hooks/pre-exit

+unset_secrets
+
+# unset the environment variables not managged by "unset_secrets"
+if [[ "$BUILDKITE_PIPELINE_SLUG" == "elastic-package" && "$BUILDKITE_STEP_KEY" == "integration-parallel-gcp" ]]; then


There would be any problem with unsetting all these variables always?

Suggested change

if [[ "$BUILDKITE_PIPELINE_SLUG" == "elastic-package" && "$BUILDKITE_STEP_KEY" == "integration-parallel-gcp" ]]; then

mmm yes , and it probably it would be safer.
Executing those unset commands even if the variable does not exist should not cause any issue.
I'll change it.

elasticmachine · 2023-10-19T09:44:47Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 0e07078

Failed CI Steps

:go: Running integration test: test-stack-command-8x

History

💚 Build #1746 succeeded 26018f2
💚 Build #1736 succeeded 3f7207e
💔 Build #1720 failed 3f7207e
💔 Build #1718 failed 3f7207e
💔 Build #1715 failed 3f7207e

cc @mrodm

Update buildkite scripts

f25127f

mrodm self-assigned this Oct 4, 2023

mrodm added 9 commits October 4, 2023 16:16

Revert "Update buildkite scripts"

546e3ea

This reverts commit f25127f.

Use retry from tooling.sh

3b35cbf

Add pre-exit hook

c41d68c

Add logout as a trap function

75e3de3

Add missing source script

bb92179

Ensure variable exists

cc2dd51

test

e3329be

Remove call to unexisting function

8ea365e

Remove debug messages

265ce2a

mrodm commented Oct 5, 2023

View reviewed changes

mrodm marked this pull request as ready for review October 5, 2023 10:55

mrodm requested review from sharbuz and a team October 5, 2023 10:55

mrodm added 2 commits October 10, 2023 17:15

Ensure that variable is defined

34c0375

Just check if variable is defined

006c194

sharbuz reviewed Oct 11, 2023

View reviewed changes

mrodm requested a review from sharbuz October 11, 2023 10:03

sharbuz reviewed Oct 11, 2023

View reviewed changes

mrodm and others added 2 commits October 11, 2023 14:54

Apply suggestions from code review

d6f3908

Co-authored-by: sharbuz <87968844+sharbuz@users.noreply.github.com>

Add double quotes

3f7207e

sharbuz approved these changes Oct 11, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into update_buildkite_sc…

26018f2

…ripts

mrodm changed the title ~~Update buildkite scripts~~ Review buildkite scripts cleanup process Oct 19, 2023

jsoriano approved these changes Oct 19, 2023

View reviewed changes

Execute unset without ifs

0e07078

mrodm merged commit 950abd3 into elastic:main Oct 19, 2023

mrodm deleted the update_buildkite_scripts branch October 19, 2023 10:18

	gcloud auth activate-service-account --key-file ${keyFile} 2> /dev/null

	export GOOGLE_APPLICATION_CREDENTIALS=${secretFileLocation}

Review buildkite scripts cleanup process #1487

Review buildkite scripts cleanup process #1487

Uh oh!

Conversation

mrodm commented Oct 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrodm commented Oct 11, 2023

Uh oh!

mrodm commented Oct 11, 2023

Uh oh!

mrodm commented Oct 11, 2023

Uh oh!

mrodm commented Oct 11, 2023

Uh oh!

mrodm commented Oct 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💛 Build succeeded, but was flaky

Failed CI Steps

History

Uh oh!

Uh oh!

elasticmachine commented Oct 19, 2023 •

edited

Loading