Do not expand escaped sequences while expanding variables sent to Runner
What does this MR do?
When we enable the :variable_inside_variable
FF, users will run into issues if they are leveraging escaping in their expressions. Let's say a user has defined the variable VAR_A=$$HOME
, intending for the value to be expanded to $HOME
by GitLab, and then to the actual HOME
environment variable value by the Runner. In the current state, since ExpandVariables.VARIABLES_REGEXP
is not smart enough to ignore escaped sequences, the value will get expanded to $
plus the value of $HOME
by GitLab. The user will get a value for $HOME
that is not the expected one (the one on the Runner where the build will be happening).
A user has come across this situation when we enabled the FF.
This MR follows up on !48627 (merged) and adds support for escape characters so that we pass-through the escaped sequences to Runner.
Performance benchmark
Note: I ran the benchmarks twice to warm up the cache.
[1] pry(main)> coll=Gitlab::Ci::Variables::Collection.new([{key: 'VAR_A', value: 'value'}, {key: 'VAR_B', value: '$$VAR_A'}, {key: 'VAR_C', value: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. $$$VAR_B'}])
[2] pry(main)> require 'benchmark'
[3] pry(main)> include Benchmark # we need the CAPTION and FORMAT constants
[4] pry(main)> n = 5000000
=> 5000000
[5] pry(main)> Benchmark.benchmark(CAPTION, 7, FORMAT, ">total:", ">avg:") do |x|
tf = x.report("value-VAR_C:") { n.times do coll.expand_value("value-VAR_C"); end }
tt = x.report("value-$VAR_C:") { n.times do coll.expand_value("value-$VAR_C"); end }
tu = x.report("value-$$VAR_C:") { n.times do coll.expand_value("value-$$VAR_C"); end }
end
ExpandVariables::VARIABLES_REGEXP
)
On master (using value | user | system | total | real |
---|---|---|---|---|
value-VAR_A : |
1.989302 | 0.034963 | 2.024265 | 2.028721 |
value-$VAR_A : |
22.908994 | 0.734235 | 23.643229 | 23.731605 |
value-$$VAR_A : |
24.298558 | 0.731617 | 25.030175 | 25.130424 |
ExpandVariables::VARIABLES_REGEXP
)
With this branch (using new value | user | system | total | real |
---|---|---|---|---|
value-VAR_C : |
2.589652 | 0.073374 | 2.663026 | 2.670081 |
value-$VAR_C : |
14.910547 | 0.306363 | 15.216910 | 15.232970 |
value-$$VAR_C : |
15.230110 | 0.351373 | 15.581483 | 15.608158 |
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Closes #321997 (closed)