Prevent double dollar expansion when it is not followed by an actual variable reference

[MartinKanters]

/kind bug
/kind api-change

What this PR does / why we need it:

It prevents the unneeded expansion of double dollars signs when they are not part of a complete variable reference.
#101137

Which issue(s) this PR fixes:

Fixes #101137

Does this PR introduce a user-facing change?

- Double dollar signs, when they are not part of an actual variable reference, will not be escaped anymore in a container's args, env and command.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

[k8s-ci-robot]

@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]

Welcome @MartinKanters!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @MartinKanters. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MartinKanters
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• third_party/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[MartinKanters]

I signed it

[fejta-bot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[fedebongio]

/sig node

[ehashman]

This is not SIG Node. Arch, maybe?

/sig architecture
/remove-sig node
/cc @dims

[sftim]

I'm concerned that this new behavior makes it more difficult to document how a conformant implementation behaves and to clearly express that to people reading that documentation.

The existing behavior is simpler and I like simplicity.

[sftim]

This PR does not add many tests for single dollar signs inside environment values etc, and I think it should - that would exercise the code paths being altered.

(those tests would be useful regardless of whether we accept this change, actually)

[rouke-broersma]

I'm concerned that this new behavior makes it more difficult to document how a conformant implementation behaves and to clearly express that to people reading that documentation.

The existing behavior is simpler and I like simplicity.

I think the current documentation is pretty clear as to expected behavior and is therefore incorrect with the current behavior. As a user when reading the current documentation:

The $(VAR_NAME) syntax can be escaped with a double $$, ie: $$(VAR_NAME).

I do not expect $ to be escaped by $$. I expect $(VAR_NAME) to be escaped by $$(VAR_NAME).

There are currently unit tests that test the escaping of $$$ to $$ so clearly someone expected this behavior, however this is not documented and is imo not reasonable. If this PR is not accepted the docs need to be modified to make this behavior explicit. The current behavior is a major gotcha, something you only figure out at best when your deployment goes wrong and at worst when you're missing data at runtime. And once you figure out that it's happening, it's also not trivial to figure out why this is happening or that you can fix it by adding another $ because only the first $ is escaped.

[sftim]

There are currently unit tests that test the escaping of $$$ to $$ so clearly someone expected this behavior

Let's make sure that after this merges, we test setting environment variable values to demonstrate what's expected including cases where the final value matches the input.

Whether this PR goes through or not, it sounds like a docs clarification would help too.

[MartinKanters]

I agree with both of you, the documentation is really lacking here. I believe the solution of this PR is the intended (and logical) behavior. I was surprised to see testcases for the, in my opinion, illogical case. For example:

• $$(VAR) is escaped, so will become: $(VAR)
• $$$(VAR) only escapes the first dollar signs, rest is parsed as variable, resulting in: $foo (given VAR=foo)
• $$$$(VAR) is escaped twice: $$(VAR)

@sftim Do you mean that we should add other tests after this MR is merged, or do you mean that I should add extra unit tests? If so, could you give me a pointer to where I could add them?

[sftim]

My interest in test cases is from SIG Docs' perspective: I'd like the set of tests to make it unambiguous what the defined behavior is, so that someone reading just the test code could accurately tech-review the related docs.

[thockin]

I disagree with this change for three reasons:

1. 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).

2. After this change, there's no way to represent a single literal $ - or am I missing that?

3. 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, even if my (1) and (2) arguments are defeated.

[rouke-broersma]

I disagree with this change for three reasons:

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

While that might be true that's not really a reason to have this behavior. Make has no relevance to this as far as I can tell so there is no reason to model the behavior after Make.

1. After this change, there's no way to represent a single literal $ - or am I missing that?

The change makes it so the escaping only happens when $ is combined with variable syntax (). So a literal $ should be fine? Unless there is a mistake in the change of course, but I don't know Go well enough to tell. It's at least not intended for literal $ to not be allowed.

2. 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, even if my (1) and (2) arguments are defeated.

It's a choice between current behavior that silently breaks applications and a breaking change to existing applications that might apply a workaround for this behavior, agreed. If this change is not allowed due to it being a breaking change, do you have any suggestions for improving the transparency and visibility of this behavior? The only reason we found why this was happening is because we opened a bug report, that's less than ideal.

[MartinKanters]

I agree with @rouke-broersma . And literal dollars should be fine: https://github.com/kubernetes/kubernetes/pull/101254/files#diff-a211abbb9a984354a16747c2ed3916eb807052dc6a9808089c797e3b117cc147R193-R195.

I don't mind too much whether we merge this PR or not, I understand that breaking stuff for existing apps is never fun, but if this does not gets merged we should really try to improve the documentation a lot. It's not clear currently.

[thockin]

@rouke-broersma

Make has no relevance to this as far as I can tell so there is no reason to model the behavior after Make.

We chose a model and syntax that was "familiar" and simple enough to understand, so it does have relevance. If it is meaningfully different, we have to explain why.

The change makes it so the escaping only happens when $ is combined with variable syntax ()

Ahh, I missed that. It's still a breaking change.

It's a choice between current behavior that silently breaks applications and a breaking change to existing applications that might apply a workaround for this behavior, agreed

If I can rephrase: "It's a choice between current behavior that silently breaks SOME NEW applications and a breaking change to existing applications"

Given this choice, I'll take "don't break existing users" every day of the week, and twice on Sunday.

If this change is not allowed due to it being a breaking change, do you have any suggestions for improving the transparency and visibility of this behavior? The only reason we found why this was happening is because we opened a bug report, that's less than ideal.

Maybe it's too buried in docs. We can bring it forward more and be clearer on this quiet feature.

We could do something like add a Condition or event or something that indicates "expansions were applied", but users would still have to seek this out (e.g. kubectl describe).

Let's get creative - when it happened, where did you look? What would have been a good signal?

[sftim]

If this does turn into primarily a discussion about docs, I recommend opening an actual issue - https://github.com/kubernetes/website/issues/new/choose

[MartinKanters]

If I can rephrase: "It's a choice between current behavior that silently breaks SOME NEW applications and a breaking change to existing applications"

Given this choice, I'll take "don't break existing users" every day of the week, and twice on Sunday.
...
Maybe it's too buried in docs. We can bring it forward more and be clearer on this quiet feature.

Major versions can contain breaking changes, right? I agree that we should not introduce breaking changes for no reason, but this is a bugfix to me. The docs clearly say The $(VAR_NAME) syntax can be escaped with a double $$, ie: $$(VAR_NAME). (Which is how this PR is implemented). As an end-user, this looks like a logical way to escape variables.

If we would have to describe the current behavior, it would become something like this: The $ sign can be escaped with a double $$. This has to be done regardless of it being part of the variable syntax, i.e. '$ and $$ and $$$' becomes '$, $ and $$'. When the double dollar signs are part of the variable syntax, it won't be replaced.. And this does not even mention the case where the variable syntax will be replaced, when it's preceded by an even number of dollar signs: $$$(VAR).

We could do something like add a Condition or event or something that indicates "expansions were applied", but users would still have to seek this out (e.g. kubectl describe).

Let's get creative - when it happened, where did you look? What would have been a good signal?

For us, it happened when we were using Tekton Pipelines and a user tried to pass a parameter containing two dollar signs (not related to the var syntax). Tekton uses that in the args section when spinning up a containers for the pipeline, and the user suddenly got a single dollar sign. Folks at Tekton are now building a workaround using the downward API.

As end users of a product built upon Kubernetes it was really hard to find the root cause. Especially because the docs are inconsistent with the behavior.

---

In the end, I understand that you want to keep this behavior for the sake of not breaking existing users, but I hope that we made it clear that the current behavior is not logical. Especially for people not familiar with Make.

[thockin]

Major versions can contain breaking changes, right?

Since there has never been one, we do not have firm doctrine. You can queue this up for kubernetes v2.0, but there are no plans to make a v2.0

If we would have to describe the current behavior,

We do have to describe the current behavior. I'm very sympathetic to the idea that this is a bug, but it has existed in the wild for over half a decade. https://www.hyrumslaw.com/

Let's get creative - when it happened, where did you look? What would have been a good signal?

For us, it happened when we were using Tekton Pipelines and a user tried to pass a parameter containing two dollar signs (not related to the var syntax). Tekton uses that in the args section when spinning up a containers for the pipeline, and the user suddenly got a single dollar sign. Folks at Tekton are now building a workaround using the downward API.

And where would you expect to look to find some feedback about this?

In the end, I understand that you want to keep this behavior for the sake of not breaking existing users, but I hope that we made it clear that the current behavior is not logical. Especially for people not familiar with Make.

I understand your point, and I am never happy to be forced to keep something
that causes friction and feels like a bug. But the project is committed to not
breaking users and in this case, we haav no reaally good choice.

We can add technical mechanisms to raise visibility (e.g. events) but ultimately it is mainly a docs bug (and we should do as @sftim suggests)

[MartinKanters]

We do have to describe the current behavior. I'm very sympathetic to the idea that this is a bug, but it has existed in the wild for over half a decade. https://www.hyrumslaw.com/

I understand, thanks for recognizing that it is/feels like a bug.

And where would you expect to look to find some feedback about this?

In this case you would have to ask the Tekton developers, but when I would encounter it myself, I guess I would start with googling and eventually perhaps end up on the Kubernetes reference documentation. An event which shows up in describe could help.

We can add technical mechanisms to raise visibility (e.g. events) but ultimately it is mainly a docs bug (and we should do as @sftim suggests)

I agree. Not sure if I'm able to help soon if at all. I'm quite busy and it doesn't help that I'm not a native speaker. Perhaps you could consider my suggestion in my last comment ;)

[thockin]

The API-doc changes are in now. Should we close this?

[MartinKanters]

Sure, fine by me!