Fix: don't overwrite .env file (#2261)

* Fix: don't overwrite .env file Do not overwrite the .env file in case ctl commands are executed. This is specially important if extra environment variables are passed in the .env file Signed-off-by: Raphael Silva <[email protected]> * Add tests for control script Signed-off-by: Raphael Silva <[email protected]> * Fix typo in github workflow Signed-off-by: Raphael Silva <[email protected]> * Fix shell linting errors Signed-off-by: Raphael Silva <[email protected]> * Update .github/workflows/PR-build.yml * Make debian aligned with RPM Signed-off-by: Raphael Silva <[email protected]> * properly install collector deb Signed-off-by: Raphael Silva <[email protected]> * Use dpkg to install collector Signed-off-by: Raphael Silva <[email protected]> * Properly use cache Signed-off-by: Raphael Silva <[email protected]> * Move control script tests to the CI Signed-off-by: Raphael Silva <[email protected]> * Add the pr-build tests back Signed-off-by: Raphael Silva <[email protected]> --------- Signed-off-by: Raphael Silva <[email protected]> Co-authored-by: bryan-aguilar <[email protected]>
aws-observability · Aug 11, 2023 · 3d239c5 · 3d239c5
1 parent 4da7dca
commit 3d239c5
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 7 deletions.
diff --git a/.github/scripts/test-collector-ctl.sh b/.github/scripts/test-collector-ctl.sh
@@ -0,0 +1,49 @@
+#!/bin/bash -x
+
+set -e
+
+# This script is supposed to run during the CI to validate that the control
+# script is working as expected
+# Important: this script requires root permission.
+
+COLLECTOR_DEB=$1
+COLLECTOR_CONFIG=$2
+
+ADOT_CTL=/opt/aws/aws-otel-collector/bin/aws-otel-collector-ctl
+ENV_FILE="/opt/aws/aws-otel-collector/etc/.env"
+CONFIG_FILE="/opt/aws/aws-otel-collector/etc/config.yaml"
+
+setup() {
+    dpkg -i "$COLLECTOR_DEB"
+}
+
+test_collector_ctl_does_not_overwrite_env() {
+    $ADOT_CTL -a start
+    if [ ! -e $CONFIG_FILE ]; then
+        echo "Config file does not exist"
+        exit 1
+    fi
+
+    echo "EXTRA_ENV=\"some value\"" | tee -a $ENV_FILE > /dev/null
+
+    # We don't need the collector to succeed, just that the control script is execised
+    $ADOT_CTL -a start -c "http://example.com"
+
+    if ! grep -q EXTRA_ENV "$ENV_FILE"; then
+        echo "Env file was overwritten"
+        exit 1
+    fi
+
+    $ADOT_CTL -a start -c "$COLLECTOR_CONFIG"
+    if ! grep -q EXTRA_ENV "$ENV_FILE"; then
+        ehco "Env file was overwritten"
+        exit 1
+    fi
+    echo "${FUNCNAME[0]} ... OK"
+}
+
+
+setup
+
+## Tests
+test_collector_ctl_does_not_overwrite_env
diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml
@@ -379,6 +379,27 @@ jobs:
         run: |
           gpg --fingerprint --with-colons [email protected] grep "^fpr" | sed -n 's/^fpr:::::::::\([[:alnum:]]\+\):/\1/p' | xargs gpg --batch --yes --delete-secret-keys
           gpg --list-secret-keys
+
+  test-control-stript:
+    runs-on: ubuntu-latest
+    needs: [packaging-deb]
+    steps:
+      - name: Checkout
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        uses: actions/checkout@v3
+
+      - name: Restore cached Debs
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        uses: actions/cache@v3
+        with:
+          key: "cached_debs_${{ github.run_id }}"
+          path: ${{ env.PACKAGING_ROOT }}
+
+      - name: Execute tests
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        run: |
+          sudo ./.github/scripts/test-collector-ctl.sh $PACKAGING_ROOT/debian/amd64/aws-otel-collector.deb ./config.yaml
+
   packaging-ssm:
     runs-on: ubuntu-latest
     needs: [packaging-rpm, packaging-deb, packaging-msi]

diff --git a/.github/workflows/PR-build.yml b/.github/workflows/PR-build.yml
@@ -247,12 +247,40 @@ jobs:
           key: "cached_binaries_${{ github.run_id }}"
           path: build
 
+      - name: Cache built debian
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        uses: actions/cache@v3
+        with:
+          key: "cached_deb_${{ github.run_id }}"
+          path: build/packages/debian
+
       - name: Build Debs
         if: ${{ needs.changes.outputs.changed == 'true' }}
         run: |
           ARCH=amd64 DEST=build/packages/debian/amd64 tools/packaging/debian/create_deb.sh
           ARCH=arm64 DEST=build/packages/debian/arm64 tools/packaging/debian/create_deb.sh
 
+  test-control-stript:
+    runs-on: ubuntu-latest
+    needs: [changes, packaging-deb]
+    steps:
+      - name: Checkout
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        uses: actions/checkout@v3
+
+      - name: Restore cached deb
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        uses: actions/cache@v3
+        with:
+          key: "cached_deb_${{ github.run_id }}"
+          path: build/packages/debian
+
+      - name: Execute tests
+        if: ${{ needs.changes.outputs.changed == 'true' }}
+        run: |
+          file ./build/packages/debian/amd64/aws-otel-collector.deb
+          sudo ./.github/scripts/test-collector-ctl.sh ./build/packages/debian/amd64/aws-otel-collector.deb ./config.yaml
+
   get-test-cases:
     runs-on: ubuntu-latest
     outputs:

diff --git a/tools/ctl/linux/aws-otel-collector-ctl.sh b/tools/ctl/linux/aws-otel-collector-ctl.sh
@@ -53,7 +53,7 @@ UsageString="
 aoc_config_remote_uri() {
     config="${1:-}"
 
-    echo "config=\"--config ${config}\"" > $ENV_FILE
+    sed -i 's#^config=.*$#config="--config '"${config}"'"#' $ENV_FILE
 }
 
 
@@ -63,12 +63,18 @@ aoc_config_local_uri() {
     # Strip the file scheme in case it is present
     config="${config#file:}"
 
-    echo "config=\"--config /opt/aws/aws-otel-collector/etc/config.yaml\"" > $ENV_FILE
+    sed -i 's#^config=.*$#config="--config /opt/aws/aws-otel-collector/etc/config.yaml"#' $ENV_FILE
 
     if [ -n "$config" ] && [ -f "$config" ]; then
         cp "$config" $CONFDIR/config.yaml
+    else
+        echo "File $config does not exist"
+        exit 1
     fi
+}
 
+# Used in case the collector starts for the first time without a configuration parameter
+aoc_ensure_default_config() {
     if [ ! -f $CONFDIR/config.yaml ]; then
         cp $DFT_CONFDIR/.config.yaml $CONFDIR/config.yaml
     fi
@@ -87,10 +93,15 @@ is_remote_uri() {
 aoc_start() {
     config="${1:-}"
 
-    if is_remote_uri "$config"; then
-        aoc_config_remote_uri "$config"
+    # The previous configuration should be used if no configuration parameter is passed
+    if [ -z "$config" ]; then
+        aoc_ensure_default_config
     else
-        aoc_config_local_uri "$config"
+        if is_remote_uri "$config"; then
+            aoc_config_remote_uri "$config"
+        else
+            aoc_config_local_uri "$config"
+        fi
     fi
 
     if [ "${SYSTEMD}" = 'true' ]; then

diff --git a/tools/packaging/debian/create_deb.sh b/tools/packaging/debian/create_deb.sh
@@ -46,15 +46,15 @@ cp LICENSE "${AOC_ROOT}/opt/aws/aws-otel-collector/"
 cp VERSION "${AOC_ROOT}/opt/aws/aws-otel-collector/bin/"
 cp "build/linux/${ARCH}/aoc" "${AOC_ROOT}/opt/aws/aws-otel-collector/bin/aws-otel-collector"
 cp tools/ctl/linux/aws-otel-collector-ctl.sh "${AOC_ROOT}/opt/aws/aws-otel-collector/bin/aws-otel-collector-ctl"
-cp config.yaml "${AOC_ROOT}/opt/aws/aws-otel-collector/etc"
+# default configuration
+cp config.yaml "${AOC_ROOT}/opt/aws/aws-otel-collector/var/.config.yaml"
 cp .env "${AOC_ROOT}/opt/aws/aws-otel-collector/etc"
 cp extracfg.txt "${AOC_ROOT}/opt/aws/aws-otel-collector/etc"
 cp tools/packaging/linux/aws-otel-collector.service "${AOC_ROOT}/etc/systemd/system/"
 cp tools/packaging/linux/aws-otel-collector.conf "${AOC_ROOT}/etc/init/"
 
 chmod ug+rx "${AOC_ROOT}/opt/aws/aws-otel-collector/bin/aws-otel-collector"
 chmod ug+rx "${AOC_ROOT}/opt/aws/aws-otel-collector/bin/aws-otel-collector-ctl"
-chmod ug+rx "${AOC_ROOT}/opt/aws/aws-otel-collector/etc/config.yaml"
 chmod ug+rx "${AOC_ROOT}/opt/aws/aws-otel-collector/etc/.env"
 chmod ug+rx "${AOC_ROOT}/opt/aws/aws-otel-collector/etc/extracfg.txt"