Skip to content

Allow for branch name lengths of 45 #8692

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

Merged
merged 2 commits into from
Mar 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as Tracing from '../observability/tracing';
import { SpanStatusCode } from '@opentelemetry/api';
import { wipePreviewEnvironmentAndNamespace, helmInstallName, listAllPreviewNamespaces } from '../util/kubectl';
import { exec } from '../util/shell';
import { previewNameFromBranchName } from '../util/preview';

// Will be set once tracing has been initialized
let werft: Werft
Expand All @@ -12,12 +13,14 @@ Tracing.initialize()
werft = new Werft("delete-preview-environment-cron")
})
.then(() => deletePreviewEnvironments())
.then(() => werft.endAllSpans())
.catch((err) => {
werft.rootSpan.setStatus({
code: SpanStatusCode.ERROR,
message: err
})
})
.finally(() => {
werft.phase("Flushing telemetry", "Flushing telemetry before stopping job")
Comment on lines -15 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure, this isn't related to the proposed change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was a tiny improvement we should make it the other jobs as well ☺️ Right now the log lines about flushing traces etc. are show up in whatever the phase was the last one in the job. With this change we introduce a new phase for the flushing so the logs become easier to read :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it was moved to "finally" as we want to flush in both the error and success cases :)

werft.endAllSpans()
})

Expand All @@ -35,25 +38,28 @@ async function deletePreviewEnvironments() {

werft.phase("Fetching branches");
const branches = getAllBranches();
const expectedPreviewEnvironmentNamespaces = new Set(branches.map(branch => parseBranch(branch)));
// During the transition from the old preview names to the new ones we have to check for the existence of both the old or new
// preview name patterns before it is safe to delete a namespace.
const expectedPreviewEnvironmentNamespaces = new Set(branches.flatMap(branch => [parseBranch(branch), expectedNamespaceFromBranch(branch)]));
Comment on lines +41 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice safety check! That didn't cross my mind when I tried to implement this

werft.done("Fetching branches");

werft.phase("Fetching previews");
let previews
let previews: string[]
try {
previews = listAllPreviewNamespaces({});
previews.forEach(previewNs => werft.log("Fetching previews", previewNs))
} catch (err) {
werft.fail("Fetching previews", err)
}
werft.done("Fetching previews");


werft.phase("Mapping previews => branches")
const previewsToDelete = previews.filter(ns => !expectedPreviewEnvironmentNamespaces.has(ns))

werft.phase("deleting previews")
try {
previewsToDelete.forEach(preview => wipePreviewEnvironmentAndNamespace(helmInstallName, preview, { slice: `Deleting preview ${preview}` }));
const previewsToDelete = previews.filter(ns => !expectedPreviewEnvironmentNamespaces.has(ns))
// Trigger namespace deletion in parallel
const promises = previewsToDelete.map(preview => wipePreviewEnvironmentAndNamespace(helmInstallName, preview, { slice: `Deleting preview ${preview}` }));
// But wait for all of them to finish before (or one of them to fail) before we continue
await Promise.all(promises)
} catch (err) {
werft.fail("deleting previews", err)
}
Expand All @@ -64,9 +70,14 @@ function getAllBranches(): string[] {
return exec(`git branch -r | grep -v '\\->' | sed "s,\\x1B\\[[0-9;]*[a-zA-Z],,g" | while read remote; do echo "\${remote#origin/}"; done`).stdout.trim().split('\n');
}

function expectedNamespaceFromBranch(branch: string): string {
const previewName = previewNameFromBranchName(branch)
return `staging-${previewName}`
}

function parseBranch(branch: string): string {
const prefix = 'staging-';
const parsedBranch = branch.normalize().split("/").join("-");

return prefix + parsedBranch;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ Tracing.initialize()
async function deletePreviewEnvironment() {
werft.phase("preparing deletion")
const version = parseVersion();

// TODO: This won't work, we need to compute the namespace using the function in
// .werft/util/preview.ts. As this job isn't executed yet I'm leaving it broken for
// now until we can test what information Werft makes available to the jobs that are
// triggered by branch deletions.
const namespace = `staging-${version.split(".")[0]}`
werft.log("preparing deletion", `Proceeding to delete the ${namespace} namespace`)
werft.done("preparing deletion")
Expand All @@ -47,4 +52,4 @@ export function parseVersion() {
version = version.substr(PREFIX_TO_STRIP.length);
}
return version
}
}
8 changes: 4 additions & 4 deletions .werft/jobs/build/job-config.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { config } from "process";
import { exec } from "../../util/shell";
import { Werft } from "../../util/werft";
import { previewNameFromBranchName } from "../../util/preview";

export interface JobConfig {
analytics: string
Expand Down Expand Up @@ -95,10 +95,10 @@ export function jobConfig(werft: Werft, context: any): JobConfig {
}
const withVM = ("with-vm" in buildConfig || repository.branch.includes("with-vm")) && !mainBuild;

const previewDestName = version.split(".")[0];
const previewEnvironmentNamespace = withVM ? `default` : `staging-${previewDestName}`;
const previewName = previewNameFromBranchName(repository.branch)
const previewEnvironmentNamespace = withVM ? `default` : `staging-${previewName}`;
const previewEnvironment = {
destname: previewDestName,
destname: previewName,
namespace: previewEnvironmentNamespace
}

Expand Down
10 changes: 7 additions & 3 deletions .werft/jobs/build/validate-changes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@ export async function validateChanges(werft: Werft, config: JobConfig) {
werft.done('validate-changes');
}

// Branch names cannot be longer than 20 characters.
// We plan to remove this limitation once we move to previews on Harvester VMs.
// Branch names cannot be longer than 45 characters.
//
// The branch name is used as part of the Werft job name. The job name is used for the name of the pod
// and k8s has a limit of 63 characters. We use 13 characters for the "gitpod-build-" prefix and 5
// more for the ".<BUILD NUMBER>" ending. That leaves us 45 characters for the branch name.
// See Werft source https://github.com/csweichel/werft/blob/057cfae0fd7bb1a7b05f89d1b162348378d74e71/pkg/werft/service.go#L376
async function branchNameCheck(werft: Werft, config: JobConfig) {
if (!config.noPreview) {
const maxBranchNameLength = 20;
const maxBranchNameLength = 45;
werft.log("check-branchname", `Checking if branch name is shorter than ${maxBranchNameLength} characters.`)

if (config.previewEnvironment.destname.length > maxBranchNameLength) {
Expand Down
32 changes: 32 additions & 0 deletions .werft/util/preview.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { createHash } from "crypto";

/**
* Based on the current branch name this will compute the name of the associated
* preview environment.
*
* NOTE: This needs to produce the same result as the function in dev/preview/util/preview-name-from-branch.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid drift, could we just call scripts/branch-namespace.sh wherever this function is called instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to for the following reasons:

  1. Currently the function doens't have any side effects. That's generally nice for utility functions as it means you can usually read the "main" script which uses the utility functions to understand what changes are made to your environment when you invoke it.
  2. The branch-namespace.sh function is core-dev specific and this utility function is used both for VMs and core-dev preview environments

*/
export function previewNameFromBranchName(branchName: string): string {
// Due to various limitations we have to ensure that we only use 20 characters
// for the preview environment name.
//
// If the branch name is 20 characters or less we just use it.
//
// Otherwise:
//
// We use the first 10 chars of the sanitized branch name
// and then the 10 first chars of the hash of the sanitized branch name
//
// That means collisions can happen. If they do, two jobs would try to deploy to the same
// environment.
//
// see https://github.com/gitpod-io/ops/issues/1252 for details.
const sanitizedBranchName = branchName.replace(/^refs\/heads\//, "").toLocaleLowerCase().replace(/[^-a-z0-9]/g, "-")

if (sanitizedBranchName.length <= 20) {
return sanitizedBranchName
}

const hashed = createHash('sha256').update(sanitizedBranchName).digest('hex')
return `${sanitizedBranchName.substring(0, 10)}${hashed.substring(0,10)}`
}
4 changes: 3 additions & 1 deletion dev/preview/install-k3s-kubeconfig.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

set -euo pipefail

VM_NAME="$(git symbolic-ref HEAD 2>&1 | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')"
source ./dev/preview/util/preview-name-from-branch.sh

VM_NAME="$(preview-name-from-branch)"

PRIVATE_KEY=$HOME/.ssh/vm_id_rsa
PUBLIC_KEY=$HOME/.ssh/vm_id_rsa.pub
Expand Down
6 changes: 4 additions & 2 deletions dev/preview/portforward-monitoring-satellite-core-dev.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
# Exposes Prometheus and Grafana's UI
#

VM_NAME="$(git symbolic-ref HEAD 2>&1 | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')"
NAMESPACE="staging-${VM_NAME}"
source ./dev/preview/util/preview-name-from-branch.sh

PREVIEW_NAME="$(preview-name-from-branch)"
NAMESPACE="staging-${PREVIEW_NAME}"

function log {
echo "[$(date)] $*"
Expand Down
4 changes: 3 additions & 1 deletion dev/preview/portforward-monitoring-satellite-harvester.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
# Exposes Prometheus and Grafana's UI
#

VM_NAME="$(git symbolic-ref HEAD 2>&1 | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')"
source ./dev/preview/util/preview-name-from-branch.sh

VM_NAME="$(preview-name-from-branch)"
NAMESPACE="preview-${VM_NAME}"

function log {
Expand Down
4 changes: 3 additions & 1 deletion dev/preview/ssh-proxy-command.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#!/usr/bin/env bash

VM_NAME="$(git symbolic-ref HEAD 2>&1 | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')"
source ./dev/preview/util/preview-name-from-branch.sh

VM_NAME="$(preview-name-from-branch)"
NAMESPACE="preview-${VM_NAME}"

while getopts n:p: flag
Expand Down
4 changes: 3 additions & 1 deletion dev/preview/ssh-vm.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

set -euo pipefail

VM_NAME="$(git symbolic-ref HEAD 2>&1 | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')"
source ./dev/preview/util/preview-name-from-branch.sh

VM_NAME="$(preview-name-from-branch)"
NAMESPACE="preview-${VM_NAME}"

PRIVATE_KEY=$HOME/.ssh/vm_id_rsa
Expand Down
21 changes: 21 additions & 0 deletions dev/preview/util/preview-name-from-branch.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/usr/bin/env bash

#
# Based on the name of the current branch this will compute the name of the associated
# preview environment.
#
# NOTE: This needs to produce the same result as the function in .werft/util/preview.ts
# See the file for implementation notes.
#
function preview-name-from-branch {
branch_name=$(git symbolic-ref HEAD 2>&1) || error "Cannot get current branch"
sanitizedd_branch_name=$(echo "$branch_name" | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')
length=$(echo -n "$sanitizedd_branch_name" | wc -c)

if [ "$length" -gt 20 ]; then
hashed=$(echo -n "${sanitizedd_branch_name}" | sha256sum)
echo "${sanitizedd_branch_name:0:10}${hashed:0:10}"
else
echo "${sanitizedd_branch_name}"
fi
}
5 changes: 3 additions & 2 deletions scripts/branch-namespace.sh
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#!/bin/bash

branch=$(git symbolic-ref HEAD 2>&1) || error "cannot set kubectl namespace: no branch"
source ./dev/preview/util/preview-name-from-branch.sh

currentContext=$(kubectl config current-context 2>&1) || error "cannot set kubectl namespace: no current context"
namespace=staging-$(echo "$branch" | awk '{ sub(/^refs\/heads\//, ""); $0 = tolower($0); gsub(/[^-a-z0-9]/, "-"); print }')
namespace="staging-$(preview-name-from-branch)"

echo "Setting kubectl namespace: $namespace"
kubectl config set-context "$currentContext" --namespace "$namespace"