Incorrect command created when using `values-to-override`, but still continues as if there's no error.

[c00]

Orb version

2.0.1

What happened

Halfway through the logs I get a "/bin/bash: line 34: image.tag=[redacted-git-sha]: command not found". But then the task does continue on its merry way as if nothing happened.

At the end the new settings are not applied.

#!/bin/bash -eo pipefail
if [ -n "${ORB_PARAM_NAMESPACE}" ]; then
  set -- "$@" --namespace="${ORB_PARAM_NAMESPACE}"
fi
if [ -n "${TIMEOUT}" ]; then
  set -- "$@" --timeout "${TIMEOUT}"
fi
if [ -n "${NO_HOOKS}" ]; then
  set -- "$@" --no-hooks="${NO_HOOKS}"
fi
if [ "${RECREATE_PODS}"  == "true" ]; then
  set -- "$@" --recreate-pods
fi
if [ "${ATOMIC}" == "true" ]; then
  set -- "$@" --atomic
fi
if [ "${ORB_PARAM_WAIT}" == "true" ]; then
  set -- "$@" --wait
fi
if [ -n "${DEVEL}" ]; then
  set -- "$@" --devel "${DEVEL}"
fi
if [ "${DRY_RUN}" == "true" ]; then
  set -- "$@" --dry-run
fi
if [ "${RESET_VALUES}" == "true" ]; then
  set -- "$@" --reset-values
fi
if [ "${REUSE_VALUES}" == "true" ]; then
  set -- "$@" --reuse-values
fi
if [ -n "${VALUES}" ]; then
  set -- "$@" --values "$(eval ${VALUES})"
fi
if [ -n "${VALUES_TO_OVERRIDE}" ]; then
  set -- "$@" --set "$(eval ${VALUES_TO_OVERRIDE})"
fi
if [ -n "${VERSION}" ]; then
  set -- "$@" --version="${VERSION}"
fi

helm repo add "${ORB_PARAM_RELEASE_NAME}" "${ORB_PARAM_REPO}"
helm repo update

helm upgrade --install "${ORB_PARAM_RELEASE_NAME}" "${ORB_PARAM_CHART}" "$@"

/bin/bash: line 34: image.tag=[redacted-git-sha]: command not found
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/circleci/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/circleci/.kube/config
"keycloak" has been added to your repositories
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/circleci/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/circleci/.kube/config
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "keycloak" chart repository
Update Complete. ⎈Happy Helming!⎈
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /home/circleci/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /home/circleci/.kube/config
Release "keycloak" has been upgraded. Happy Helming!
NAME: keycloak
LAST DEPLOYED: Mon Mar  6 05:50:57 2023
NAMESPACE: keycloak-test
STATUS: deployed
REVISION: 202
TEST SUITE: None
NOTES:
***********************************************************************
*                                                                     *
*                Keycloak Helm Chart by codecentric AG                *
*                                                                     *
***********************************************************************

Keycloak was installed with an Ingress and can be reached at the following URL(s):

  - https://accounts.test.nqap.de/

CircleCI received exit code 0

The shortened version of the cci config:

jobs:
  deploy:
    parameters:
      env-name:
        description: Environment name
        type: string
    docker:
      - image: cimg/base:stable
    working_directory: ~/app
    steps:
      - checkout
      - kubernetes/install-kubectl:
          kubectl-version: "v1.21.3"
      - kubernetes/install-kubeconfig
      - helm/install-helm-client:
          version: v3.9.4
      - helm/upgrade-helm-chart:
          add-repo: https://codecentric.github.io/helm-charts
          namespace: << parameters.env-name >>
          release-name: keycloak
          chart: keycloak/keycloak
          reuse-values: true
          values-to-override: image.tag=<< pipeline.git.revision >>

orbs:
  helm: circleci/helm@2.0.1
  kubernetes: circleci/kubernetes@1.3.1

workflows:
  build-master:
    jobs:
      - deploy:
          name: Deploy to Keycloak Test
          context: some-context
          env-name: keycloak-test

Expected behavior

1. If an error is encountered, the bash script should exit with a non-zero value.
2. There should not be an error with the given configuration.

[a1ex-var1amov]

Yes, ++. v 2.0.0 works fine with the overrides.

[c00]

Interesting detail @a1ex-var1amov ! Looking at the diff between 2.0.0 and 2.0.1 we can see that indeed, an eval was added at the line where stuff goes wrong.

Looking at the changelog:

Previously users were able to use environment variable strings within parameters, this functionality was removed in 2.0.0 and has been added back with this change.

The reason for this change was to make sure that environment variables within parameters are updated. Which is a cool thing. However, under the values-to-override thing, the value isn't just a string that can be eval'd, but rather a key=value string, which when evals, just fails.

On top of that, this is arguably a security issue. eval just executes a command, so in theory any command could be injected here. If the only purpose is to replace env variables, I would suggest using something like envsubst that simply takes an input, and replaces env variables where they are found.

Using a envsubst would achieve the same thing in terms of env variable replacement, but without the execution of arbitrary code.

[a1ex-var1amov]

@c00 yes, 100%.

Just in case, I'll put the example of a wrong eval usage here:

❯ MYTEST="123"
❯ eval $MYTEST
zsh: command not found: 123`

And the proper one:

❯ mkdir test
❯ MYTEST="cd test"
❯ eval $MYTEST
❯ pwd
<omitted>/test

[KyleTryon]

I agree @c00, we run into this issue all the time. envsubst would absolutely be the key here, but it is also another dependency that users would need to ensure is present on their image in order to use the orb. It may be worth doing though, based on the good examples you have shared here.

[c00]

but it is also another dependency that users would need to ensure is present on their image

Yeah this is a problem. There may be some fancy sed alternative that's doable (as sed is available pretty much everywhere). My first thought was a regex that extracts the env variables, and then a loop with sed that does the substitutions, but I can't really get a working example.

Alternatively you could consider checking for the existence of envsubst, output a warning/error if it's not on the container. The error would make sense along with a flag like substitute-env-vars or something.

Considering this is a CCI orb and will probably often be used with CCI base images, this would really only make sense if envsubst was part of the CCI base images, which I do not know. (edit: They don't seem to include it.)

You could also considering installing envsubst if it doesn't exist... But it feels kinda dirty.

[brivu]

Hey @c00,

I've added circleci env subst to the parameter in PR #68. I'm working on closing out the remaining issues for this orb and will get a new version published as soon as we can.

Thank for you flagging! -Brian