Add script which uploads metadata to CDN servers

[raksooo]

This PR adds a script which uploads metadata to the CDN servers via the build server. It needs to be integrated in desktop/scripts/release/4-make-release and might need to upload artifacts as well.

---

This change is 

[linear]

DES-1781 Upload metadata to CDN servers

[faern]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions

---

desktop/scripts/release/upload-metadata-to-cdn line 12 at r1 (raw file):

cdn_metadata_dir="desktop/metadata"

buildserver_host="app-build-linux3"

I don't think data like this should be hardcoded into the scripts. Creates needless churn when migrating build servers and/or for admins who have named this server something different in their ssh config. I'd prefer if this was an argument to the script.

---

desktop/scripts/release/upload-metadata-to-cdn line 37 at r1 (raw file):

function remove_buildserver_tmp_dir {
  run_on_build_server rm -r $buildserver_metadata_dir 2> /dev/null || true

Do we really want || true here? Don't we want to fail if removing these files returns an error? Just so we don't continue creating new info and try to upload it to the same dir and get some mixed problems from that.

[dlon]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern and @raksooo)

---

desktop/scripts/release/upload-metadata-to-cdn line 37 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Do we really want || true here? Don't we want to fail if removing these files returns an error? Just so we don't continue creating new info and try to upload it to the same dir and get some mixed problems from that.

Could you use -f instead, in case the dir doesn't exist?

[raksooo]

I've added arguments checking and calling the script from 4-make-release.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @faern)

---

desktop/scripts/release/upload-metadata-to-cdn line 12 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I don't think data like this should be hardcoded into the scripts. Creates needless churn when migrating build servers and/or for admins who have named this server something different in their ssh config. I'd prefer if this was an argument to the script.

Done.

---

desktop/scripts/release/upload-metadata-to-cdn line 37 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could you use -f instead, in case the dir doesn't exist?

Done. I went with Davids suggestoin to use -f to not fail if the directory doesn't exist.

[raksooo]

Testing the script again along with removing the --dry-run argument still remains.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @faern)

[faern]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions

---

desktop/scripts/release/4-make-release line 17 at r2 (raw file):

    echo "        <build server SSH destination> \\"
    echo "        <CDN server SSH destination>"
    echo $'\nNote that the CDN server SSH destination is part of the rsync command executed on the build server and will be checked against the SSH config of build@$buildserver_host.'

I think we should distinguish between releases.mullvad.net, where we upload this metadata, and the third party CDN where we upload and host installer artifacts. If we just call both things "CDN" it will be messy. So I don't think we should call this thing right here the "CDN" (so this comment also applies to the naming of the other script, which has CDN in its name).

Can we be more specific and include "metadata" or "release metadata" in this name, to make it clear what said host is hosting? I suggest replacing "CDN server" with something like "metadata server". And maybe rename the other script from upload-metadata-to-cdn to publish-metadata-to-api or something? It does not push it directly to the API, but this is the closest we get. The API fetch the metadata from there, so in practice what this does is to publish the metadata to the API. This more direct name also makes it clearer that something is actually published which might keep a caller more awake, knowing it's meddling with production :)

[faern]

Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @raksooo)

---

desktop/scripts/release/4-make-release line 24 at r2 (raw file):
I was going comment about this not being consistent. But I then realized that all the scripts in this dir follow this pattern. IMO a bit unfortunate that we did not follow the standard we have had for a long time in many other scripts. with calling the variable REPO_DIR and defining it as relative to SCRIPT_DIR (and as a result removes the dependency on cd $SCRIPT_DIR being called before this assignment.

REPO_DIR="$( cd "$SCRIPT_DIR/.." && pwd )"

---

desktop/scripts/release/upload-metadata-to-cdn line 57 at r2 (raw file):

# Send the metadata on the buildserver to the cdn server
# TODO: Remove --dry-run when ready to take for a test drive
buildserver_rsync --dry-run $BUILDSERVER_METADATA_DIR "$CDN_HOST":"$(dirname "$CDN_METADATA_DIR")"

I guess the --dry-run part is what keeps the PR in draft mode? Just opening this thread here so I can approve the file but keep track of this needed change before merge.

---

desktop/scripts/release/4-make-release line 37 at r2 (raw file):

rm -rf $ARTIFACT_DIR
mkdir -p $ARTIFACT_DIR

Nit. We have precedence in doing this kind of "cleaning operation". Not a big deal, but we might want to be consistent with the existing code. Nice to follow a familiar pattern:

mullvadvpn-app/ci/linux-repository-builder/build-linux-repositories.sh

Lines 104 to 105 in cae71a3

	# Recreate the artifact dir, cleaning it
	rm -rf "$artifact_dir" && mkdir -p "$artifact_dir" || exit 1

[faern]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @raksooo)

---

desktop/scripts/release/4-make-release line 60 at r2 (raw file):

        fi
        echo ""
        echo "GOOD SIGNATURE IN $pkg_filename"

Nit. The signature is not in the package. The important part is probably more like "GOOD SIGNATURE FOR $pkg_filename"?

---

desktop/scripts/release/4-make-release line 69 at r2 (raw file):

function publish_metadata {
    local platforms
    platforms=(windows macos linux)

Both the platforms and the requirement to set VERSION_METADATA_SECRET are things a caller of the script might want to know. Would both of these things not be better declared the top of the script? Having a constant at the top with the hardcoded list of platforms. And a comment at the top stating that it must be executed with the secrets variable set.

---

desktop/scripts/release/4-make-release line 87 at r2 (raw file):

    echo ">>> Signing $PRODUCT_VERSION metadata"
    meta sign --secret "$VERSION_METADATA_SECRET" "${platforms[@]}"

I'm not extremely comfortable with passing the important private key as a command line argument. It will end up in the process list on the machine, and potentially in various logs and other places.

[faern]

Reviewable status: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @raksooo)

---

desktop/scripts/release/4-make-release line 32 at r2 (raw file):

REPO_URL="[email protected]:mullvad/mullvadvpn-app"
ARTIFACT_DIR="./artifacts"
REPO_DIR=$(mktemp -d)

Wait wait... I was confused for a long time here before realizing what's going on. We have strong precedence for calling the variable pointing to the currently checked out repository REPO_DIR in many other scripts in this repository. I really think we should avoid using this variable name for a local new temporary clone.

[raksooo]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @faern)

---

desktop/scripts/release/4-make-release line 17 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think we should distinguish between releases.mullvad.net, where we upload this metadata, and the third party CDN where we upload and host installer artifacts. If we just call both things "CDN" it will be messy. So I don't think we should call this thing right here the "CDN" (so this comment also applies to the naming of the other script, which has CDN in its name).

Can we be more specific and include "metadata" or "release metadata" in this name, to make it clear what said host is hosting? I suggest replacing "CDN server" with something like "metadata server". And maybe rename the other script from upload-metadata-to-cdn to publish-metadata-to-api or something? It does not push it directly to the API, but this is the closest we get. The API fetch the metadata from there, so in practice what this does is to publish the metadata to the API. This more direct name also makes it clearer that something is actually published which might keep a caller more awake, knowing it's meddling with production :)

Done.

---

desktop/scripts/release/4-make-release line 24 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I was going comment about this not being consistent. But I then realized that all the scripts in this dir follow this pattern. IMO a bit unfortunate that we did not follow the standard we have had for a long time in many other scripts. with calling the variable REPO_DIR and defining it as relative to SCRIPT_DIR (and as a result removes the dependency on cd $SCRIPT_DIR being called before this assignment.

REPO_DIR="$( cd "$SCRIPT_DIR/.." && pwd )"

You're right in your confusion! I at some point forgot that this shouldn't be run in the repo. I've now adjusted the script to work standalone. This line has been removed.

---

desktop/scripts/release/4-make-release line 32 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Wait wait... I was confused for a long time here before realizing what's going on. We have strong precedence for calling the variable pointing to the currently checked out repository REPO_DIR in many other scripts in this repository. I really think we should avoid using this variable name for a local new temporary clone.

This should be fixed now.

---

desktop/scripts/release/4-make-release line 60 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nit. The signature is not in the package. The important part is probably more like "GOOD SIGNATURE FOR $pkg_filename"?

Fixed.

---

desktop/scripts/release/upload-metadata-to-cdn line 57 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I guess the --dry-run part is what keeps the PR in draft mode? Just opening this thread here so I can approve the file but keep track of this needed change before merge.

Yes, that's correct 👍

[dlon]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @faern and @raksooo)

---

desktop/scripts/release/4-make-release line 87 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I'm not extremely comfortable with passing the important private key as a command line argument. It will end up in the process list on the machine, and potentially in various logs and other places.

Fixed on main.