From d7b23b84f0fdcc31b3c90435819dd7f1cdd35e40 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 21 Dec 2020 18:35:09 -0500 Subject: [PATCH 1/2] fix: allow renaming old applications to new format 0.22.0 changed allowed values for app names, making it more difficult for users to fix their app names - you need to either downgrade+rename or recreate the app. The `apps:rename` command can now rename these old applications to the new standard. Also drop some app name verification from internal commands. Duplicate verification doesn't help much, and though it isn't excessively slow, it does prevent us from running commands against old apps that exist. Refs #4267 --- plugins/apps/functions.go | 11 +++++++++++ plugins/apps/subcommands.go | 20 ++++++-------------- plugins/apps/triggers.go | 13 ------------- plugins/common/common.go | 27 ++++++++++++++++++++++----- plugins/common/functions | 32 +++++++++++++++++++++++++------- plugins/config/config.go | 10 +++++++--- plugins/config/environment.go | 4 ++++ plugins/network/go.mod | 2 +- tests/unit/apps_1.bats | 2 +- 9 files changed, 77 insertions(+), 44 deletions(-) diff --git a/plugins/apps/functions.go b/plugins/apps/functions.go index 867c3536b..437706328 100644 --- a/plugins/apps/functions.go +++ b/plugins/apps/functions.go @@ -83,6 +83,17 @@ func destroyApp(appName string) error { forceCleanup := true common.DockerCleanup(appName, forceCleanup) + + // remove contents for apps that are symlinks to other folders + if err := os.RemoveAll(fmt.Sprintf("%v/", common.AppRoot(appName))); err != nil { + common.LogWarn(err.Error()) + } + + // then remove the folder and/or the symlink + if err := os.RemoveAll(common.AppRoot(appName)); err != nil { + common.LogWarn(err.Error()) + } + return nil } diff --git a/plugins/apps/subcommands.go b/plugins/apps/subcommands.go index 2c73a234b..fe23dce64 100644 --- a/plugins/apps/subcommands.go +++ b/plugins/apps/subcommands.go @@ -18,7 +18,7 @@ func CommandClone(oldAppName string, newAppName string, skipDeploy bool, ignoreE return errors.New("Please specify an new app name") } - if err := common.IsValidAppName(oldAppName); err != nil { + if err := common.VerifyAppName(oldAppName); err != nil { return err } @@ -26,10 +26,6 @@ func CommandClone(oldAppName string, newAppName string, skipDeploy bool, ignoreE return err } - if err := appExists(oldAppName); err != nil { - return err - } - if err := appExists(newAppName); err == nil { if ignoreExisting { common.LogWarn("Name is already taken") @@ -61,8 +57,8 @@ func CommandClone(oldAppName string, newAppName string, skipDeploy bool, ignoreE // CommandCreate creates app via command line func CommandCreate(appName string) error { - if appName == "" { - return errors.New("Please specify an app to run the command on") + if err := common.IsValidAppName(appName); err != nil { + return err } return createApp(appName) @@ -70,8 +66,8 @@ func CommandCreate(appName string) error { // CommandDestroy destroys an app func CommandDestroy(appName string, force bool) error { - if appName == "" { - return errors.New("Please specify an app to run the command on") + if err := common.VerifyAppName(appName); err != nil { + return err } if force { @@ -153,7 +149,7 @@ func CommandRename(oldAppName string, newAppName string, skipDeploy bool) error return errors.New("Please specify an new app name") } - if err := common.IsValidAppName(oldAppName); err != nil { + if err := common.VerifyAppName(oldAppName); err != nil { return err } @@ -161,10 +157,6 @@ func CommandRename(oldAppName string, newAppName string, skipDeploy bool) error return err } - if err := appExists(oldAppName); err != nil { - return err - } - if err := appExists(newAppName); err == nil { return errors.New("Name is already taken") } diff --git a/plugins/apps/triggers.go b/plugins/apps/triggers.go index 45426436f..49f53d9c9 100644 --- a/plugins/apps/triggers.go +++ b/plugins/apps/triggers.go @@ -1,9 +1,6 @@ package apps import ( - "fmt" - "os" - "github.com/dokku/dokku/plugins/common" ) @@ -31,16 +28,6 @@ func TriggerAppMaybeCreate(appName string) error { func TriggerPostDelete(appName string) error { imageRepo := common.GetAppImageRepo(appName) - // remove contents for apps that are symlinks to other folders - if err := os.RemoveAll(fmt.Sprintf("%v/", common.AppRoot(appName))); err != nil { - common.LogWarn(err.Error()) - } - - // then remove the folder and/or the symlink - if err := os.RemoveAll(common.AppRoot(appName)); err != nil { - common.LogWarn(err.Error()) - } - imagesByAppLabel, err := listImagesByAppLabel(appName) if err != nil { common.LogWarn(err.Error()) diff --git a/plugins/common/common.go b/plugins/common/common.go index 5ef60c4ed..4fc019cea 100644 --- a/plugins/common/common.go +++ b/plugins/common/common.go @@ -416,10 +416,10 @@ func UcFirst(str string) string { return "" } -// IsValidAppName verifies app name format +// IsValidAppName verifies that the app name matches naming restrictions func IsValidAppName(appName string) error { if appName == "" { - return fmt.Errorf("APP must not be null") + return errors.New("Please specify an app to run the command on") } r, _ := regexp.Compile("^[a-z0-9][^/:_A-Z]*$") @@ -430,10 +430,27 @@ func IsValidAppName(appName string) error { return errors.New("App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, colons, or underscores") } -// VerifyAppName verifies app name format and app existence" +// isValidAppNameOld verifies that the app name matches the old naming restrictions +func isValidAppNameOld(appName string) error { + if appName == "" { + return errors.New("Please specify an app to run the command on") + } + + r, _ := regexp.Compile("^[a-z0-9][^/:A-Z]*$") + if r.MatchString(appName) { + return nil + } + + return errors.New("App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, or colons") +} + +// VerifyAppName checks if an app conforming to either the old or new +// naming conventions exists func VerifyAppName(appName string) error { - if err := IsValidAppName(appName); err != nil { - return err + newErr := IsValidAppName(appName) + oldErr := isValidAppNameOld(appName) + if newErr != nil && oldErr != nil { + return newErr } appRoot := AppRoot(appName) diff --git a/plugins/common/functions b/plugins/common/functions index 88146de85..dc721a5de 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -181,9 +181,9 @@ dokku_container_log_verbose_quiet() { } is_valid_app_name() { - declare desc="verify app name format" + declare desc="verify that the app name matches naming restrictions" local APP="$1" - [[ -z "$APP" ]] && dokku_log_fail "APP must not be null" + [[ -z "$APP" ]] && dokku_log_fail "Please specify an app to run the command on" if [[ "$APP" =~ ^[a-z].* ]] || [[ "$APP" =~ ^[0-9].* ]]; then if [[ ! $APP =~ [A-Z] ]] && [[ ! $APP =~ [:] ]] && [[ ! $APP =~ [_] ]]; then return 0 @@ -193,10 +193,31 @@ is_valid_app_name() { dokku_log_fail "App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, colons, or underscores" } +is_valid_app_name_old() { + declare desc="verify that the app name matches the old naming restrictions" + local APP="$1" + [[ -z "$APP" ]] && dokku_log_fail "Please specify an app to run the command on" + if [[ "$APP" =~ ^[a-z].* ]] || [[ "$APP" =~ ^[0-9].* ]]; then + if [[ ! $APP =~ [A-Z] ]] && [[ ! $APP =~ [:] ]]; then + return 0 + fi + fi + + dokku_log_fail "App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, or colons" +} + verify_app_name() { declare desc="verify app name format and app existence" - local APP="$1" - is_valid_app_name "$APP" + declare APP="$1" + local VALID_NEW=false + local VALID_OLD=false + is_valid_app_name "$APP" 2>/dev/null && VALID_NEW=true + is_valid_app_name_old "$APP" 2>/dev/null && VALID_OLD=true + + if [[ "$VALID_NEW" == "false" ]] && [[ "$VALID_OLD" == "false" ]]; then + dokku_log_fail "App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, colons, or underscores" + fi + [[ ! -d "$DOKKU_ROOT/$APP" ]] && dokku_log_fail "App $APP does not exist" return 0 @@ -216,7 +237,6 @@ get_app_image_repo() { declare desc="central definition of image repo pattern" local APP="$1" local IMAGE_REPO="dokku/$APP" - is_valid_app_name "$APP" echo "$IMAGE_REPO" } @@ -225,7 +245,6 @@ get_deploying_app_image_name() { local APP="$1" local IMAGE_TAG="$2" IMAGE_REPO="$3" - is_valid_app_name "$APP" local IMAGE_REMOTE_REPOSITORY=$(plugn trigger deployed-app-repository "$APP") local NEW_IMAGE_TAG=$(plugn trigger deployed-app-image-tag "$APP") @@ -246,7 +265,6 @@ get_app_image_name() { local APP="$1" local IMAGE_TAG="$2" IMAGE_REPO="$3" - is_valid_app_name "$APP" if [[ -z "$IMAGE_REPO" ]]; then IMAGE_REPO=$(get_app_image_repo "$APP") diff --git a/plugins/config/config.go b/plugins/config/config.go index 37734261a..71feb4788 100644 --- a/plugins/config/config.go +++ b/plugins/config/config.go @@ -1,7 +1,6 @@ package config import ( - "errors" "fmt" "os" "regexp" @@ -145,9 +144,14 @@ func getEnvironment(appName string, merged bool) (env *Env) { } func getAppNameOrGlobal(appName string, global bool) (string, error) { - if appName == "" && !global { - return appName, errors.New("Please specify an app to run the command on or --global") + if global { + return appName, nil } + + if err := common.VerifyAppName(appName); err != nil { + return appName, err + } + return appName, nil } diff --git a/plugins/config/environment.go b/plugins/config/environment.go index a4bb1063f..23e639eda 100644 --- a/plugins/config/environment.go +++ b/plugins/config/environment.go @@ -56,6 +56,10 @@ func newEnvFromString(rep string) (env *Env, err error) { //LoadAppEnv loads an environment for the given app func LoadAppEnv(appName string) (env *Env, err error) { + err = common.VerifyAppName(appName) + if err != nil { + return + } appfile, err := getAppFile(appName) if err != nil { return diff --git a/plugins/network/go.mod b/plugins/network/go.mod index 9208e00ad..8858202ab 100644 --- a/plugins/network/go.mod +++ b/plugins/network/go.mod @@ -7,7 +7,7 @@ require ( github.com/codeskyblue/go-sh v0.0.0-20190412065543-76bd3d59ff27 github.com/dokku/dokku/plugins/common v0.0.0-00010101000000-000000000000 github.com/dokku/dokku/plugins/config v0.0.0-00010101000000-000000000000 - github.com/spf13/pflag v1.0.5 // indirect + github.com/spf13/pflag v1.0.5 // indirect ) replace github.com/dokku/dokku/plugins/common => ../common diff --git a/tests/unit/apps_1.bats b/tests/unit/apps_1.bats index 28ababc7b..79cea4238 100644 --- a/tests/unit/apps_1.bats +++ b/tests/unit/apps_1.bats @@ -179,7 +179,7 @@ teardown() { echo "status: $status" assert_success - run /bin/bash -c "dokku --trace apps:rename $TEST_APP great-test-name" + run /bin/bash -c "dokku apps:rename $TEST_APP great-test-name" echo "output: $output" echo "status: $status" assert_success From 59a81ad3be92663afcc923b2fd4af9c15301c3f0 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Tue, 22 Dec 2020 12:26:44 -0500 Subject: [PATCH 2/2] fix: cleanup apps and containers before each test - remove all nginx configurations since they may be faulty - move cleanup commands to helper functions - fix a few shellcheck violations --- tests/unit/test_helper.bash | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_helper.bash b/tests/unit/test_helper.bash index 1cdf008ab..379bb184f 100644 --- a/tests/unit/test_helper.bash +++ b/tests/unit/test_helper.bash @@ -21,14 +21,26 @@ global_setup() { [[ ! -f "${BATS_PARENT_TMPNAME}.skip" ]] || skip "$SKIPPED_TEST_ERR_MSG" free -m + cleanup_apps + cleanup_containers } global_teardown() { [[ -n "$BATS_TEST_COMPLETED" ]] || touch "${BATS_PARENT_TMPNAME}.skip" + cleanup_apps + cleanup_containers +} + +cleanup_apps() { + rm -rf $DOKKU_ROOT/*/nginx.conf + apps=$(dokku --quiet apps:list) if [[ -n "${apps}" ]]; then dokku --quiet apps:list | xargs -n1 dokku --force apps:destroy fi +} + +cleanup_containers() { containers=$(docker container ls --quiet) if [[ -n "$containers" ]]; then docker container ls --quiet | xargs -n1 docker container rm -f || true @@ -132,7 +144,7 @@ assert_output_contains() { local found=0 until [ "${input/$expected/}" = "$input" ]; do input="${input/$expected/}" - let found+=1 + found=$((found + 1)) done assert_equal "$count" "$found" } @@ -208,7 +220,7 @@ add_domain() { # shellcheck disable=SC2119 check_urls() { local PATTERN="$1" - run /bin/bash -c "dokku --quiet urls $TEST_APP | grep -E \"${1}\"" + run /bin/bash -c "dokku --quiet urls $TEST_APP | grep -E \"${PATTERN}\"" echo "output: $output" echo "status: $status" assert_success @@ -238,7 +250,7 @@ assert_nonssl_domain() { assert_app_domain() { local domain=$1 run /bin/bash -c "dokku domains:report $TEST_APP --domains-app-vhosts | tr \" \" \"\n\" | grep -xF ${domain}" - echo "app domains: $(dokku domains:report $TEST_APP --domains-app-vhosts | tr \" \" \"\n\")" + echo "app domains: $(dokku domains:report "$TEST_APP" --domains-app-vhosts | tr \" \" \"\n\")" echo "output: $output" echo "status: $status" assert_output "${domain}" @@ -315,7 +327,8 @@ deploy_app() { } setup_client_repo() { - local TMP=$(mktemp -d "/tmp/dokku.me.XXXXX") + local TMP + TMP=$(mktemp -d "/tmp/dokku.me.XXXXX") rmdir "$TMP" && cp -r "${BATS_TEST_DIRNAME}/../../tests/apps/nodejs-express" "$TMP" cd "$TMP" || exit 1 git init