Skip to content
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

Double dollar signs in a container's args or env section are incorrectly treated as escape characters #101137

Closed
MartinKanters opened this issue Apr 15, 2021 · 9 comments · Fixed by #101916
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node.

Comments

@MartinKanters
Copy link
Contributor

What happened:

I'm trying to pass two dollar sign in the args or env fields of a pod, but they are reduced to one dollar sign.

What you expected to happen:

The two dollar signs should be propagated to the container.

How to reproduce it (as minimally and precisely as possible):

  1. Apply this pod:
apiVersion: v1
kind: Pod
metadata:
  name: dollar-dollar-example
  namespace: default
spec:
  containers:
    - name: client-container
      image: k8s.gcr.io/busybox
      env: 
      - name: DOLLAR_DOLLAR_ENV
        value: "$$"
      command: ["sh", "-c"]
      args: 
      - "echo 'dollar dollar from args: $$';
         echo 'dollar dollar from env:' $DOLLAR_DOLLAR_ENV"
  1. Check logs
kubectl logs dollar-dollar-example
dollar dollar from args: $
dollar dollar from env: $

Anything else we need to know?:

Kubernetes can expand variables used in the args and env section with the following syntax: $(var). $$(var) is a way of escaping this variable, resulting in it not being expanded. I don't think '$$' should be regarded as something to be escaped, though.
Here's the expansion code, I believe.

I can take a shot at fixing it, but I first want to know for sure that this is not intended behavior.

Environment:

  • Kubernetes version (use kubectl version):
Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.4", GitCommit:"e87da0bd6e03ec3fea7933c4b5263d151aafd07c", GitTreeState:"clean", BuildDate:"2021-02-22T19:05:30Z", GoVersion:"go1.15.8", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.7", GitCommit:"14f897abdc7b57f0850da68bd5959c9ee14ce2fe", GitTreeState:"clean", BuildDate:"2021-01-22T17:29:38Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}
  • Cloud provider or hardware configuration: Azure
  • OS (e.g: cat /etc/os-release):
    Ubuntu over WSL
NAME="Ubuntu"
VERSION="18.04.4 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.4 LTS"
VERSION_ID="18.04"
@MartinKanters MartinKanters added the kind/bug Categorizes issue or PR as related to a bug. label Apr 15, 2021
@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 15, 2021
@k8s-ci-robot
Copy link
Contributor

@MartinKanters: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 15, 2021
@pacoxu
Copy link
Member

pacoxu commented Apr 15, 2021

@rouke-broersma
Copy link

rouke-broersma commented Apr 15, 2021

For env, see #75907 (comment)

See documentation at https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#envvar-v1-core.

Hi @pacoxu

The docs do say that $$ can be used to escape vars, but in this case we are not using vars. ANY $$ is escaped to just $, which seems incorrect. Without any knowledge of kubernetes internals I would say it would be simple to detect $$() and only escape this sequence, instead of all $$ sequences.

@neolit123
Copy link
Member

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 16, 2021
@MartinKanters
Copy link
Contributor Author

/assign

@fedebongio
Copy link
Contributor

/remove-sig api-machinery
/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 20, 2021
@thockin
Copy link
Member

thockin commented Apr 26, 2021

For posterity, copying comment from the PR:

I disagree that this is a bug (or at worst is a docs bug).

The behavior and syntax was modelled after make and is mostly consistent with that:

$ cat /tmp/ma
VAR := val

all:
	@echo '$(VAR)'
	@echo '$$(VAR)'
	@echo '$$$(VAR)'
	@echo '$$$$(VAR)'
	@echo '$VAR'
	@echo '$$VAR'
	@echo '$$$VAR'
	@echo '$$$$VAR'

$ make -f /tmp/ma
val
$(VAR)
$val
$$(VAR)
AR
$VAR
$AR
$$VAR

$$ -> $

The only place we are different, I think, is that we don't eat the V in $V (I think, can't run a test right now).

Worse, "fixing" this is a breaking change. Someone out there is depending on the current behavior, and to fix one app you silently break another. We can't do that.

@sftim
Copy link
Contributor

sftim commented May 11, 2021

I recommend opening a new issue in this repo that covers updating API documentation such as

// Optional: The docker image's entrypoint is used if this is not provided; cannot be updated.
// Variable references $(VAR_NAME) are expanded using the container's environment. If a variable
// cannot be resolved, the reference in the input string will be unchanged. The $(VAR_NAME) syntax
// can be escaped with a double $$, ie: $$(VAR_NAME). Escaped references will never be expanded,
// regardless of whether the variable exists or not.

@MartinKanters
Copy link
Contributor Author

Thanks @sftim , I've created a PR.

giorgiosironi added a commit to sciety/sciety that referenced this issue Sep 1, 2021
`kubectl run ... --env=NAME=VALUE` interprets `$$` in VALUE as a (broken) interpolation of variables.

Values containing `$$` need to be escaped as `$$$$` as `$$` is turned
into `$`.

`sed` does this job, but specifies `$` as `\$`; `$` on its own means
end-of-the-string.

Since the `sed` shell command is inside the Makefile, all `$` further
double in number to avoid interpolation as a Make variable.

See kubernetes/kubernetes#101137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
8 participants