Make literal $ character work in set $location_path #3194
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
@bshelton229 please rebase to pass CI |
bshelton229
force-pushed
the
literal-dollar-character
branch
from
95a5617 to
3dc131b
Compare
| // geo $literal_dollar { | ||
| // default "$"; | ||
| // } | ||
| func escapeLocationPathVar(input interface{}) string { |
There was a problem hiding this comment.
This is not necessarily about location path var and can be useful for other variables that might need to include $ in the value. What about escapeLiteralDollar?
| @@ -834,3 +834,16 @@ func TestBuildUpstreamName(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestEscapeLocationPathVar(t *testing.T) { | |||
| escapedPath := escapeLocationPathVar("/$") | |||
There was a problem hiding this comment.
Can you also add a test case for when there are multiple dollar signs in the string?
|
@ElvinEfendi thanks! I made the suggested updates. I still haven't dug up a nicer way to escape dollar signs in nginx variables. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bshelton229, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Thanks @bshelton229 ! I don't know any better option offhand either. So let's get this merged and we can change later when we find a better way of doing it. |
What this PR does / why we need it:
This is one option of making the template work when the literal
$character is in a path. We ran into this testing the new regex functionality where we have paths using regexes that contain a$.I had a hard time finding another way to get a literal
$in a set variable in nginx. This idea was found in this thread - https://forum.nginx.org/read.php?2,218536,218559 .Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #3191Special notes for your reviewer:
Using the geo module definitely feels really strange. When we were using a custom template to facilitate regex, before testing the
use-regexfunctionality that will come in 0.20.0, we just removed theset $location_pathdeclaration as it didn't appear to be used by the template anywhere else that we could see and we didn't need to use it anywhere custom. We also considered a string replace with something like%24rather than this type of hack. There is possibly a better way to facilitate getting a$character in a set that I can't find.