feat: allow interpolation of cluster generator values #9254
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9254 +/- ##
=======================================
Coverage 45.78% 45.79%
=======================================
Files 227 227
Lines 26904 26920 +16
=======================================
+ Hits 12318 12327 +9
- Misses 12908 12911 +3
- Partials 1678 1682 +4
Continue to review full report at Codecov.
|
blakepettersson
force-pushed
the
feature/enable-interpolation-for-cluster-generator-values
branch
from
5bcf40c
to
1c16a4a
Compare
Thanks for finding it useful 😄 I wrote some basic usage docs in |
blakepettersson
force-pushed
the
feature/enable-interpolation-for-cluster-generator-values
branch
from
1c16a4a
to
9b746b6
Compare
|
@crenshaw-dev is there anything else I should do on this PR? |
|
@blakepettersson ah! No, besides pinging me to remind me to review. Thanks. :-) |
applicationset/generators/cluster.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| params[fmt.Sprintf("values.%s", key)] = replacedTmplStr |
There was a problem hiding this comment.
I'm a little worried about setting directly to params. At this point, AppSet is admin-only, but in the future I'd like it to see it made available to lower-privileged users. In that case, I'd be concerned about a billion-laughs attack.
The attack would work like this:
values:
lol1: lol
lol2: '{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}{{values.lol1}}'
lol3: '{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}'
# ...etc. through lol9There are probably other/easier ways to attack the AppSet controller, but I'd rather not add another one.
What if we just keep track of an interpolatedParams map which isn't merged back with params until all the other interpolation is finished?
There was a problem hiding this comment.
What if we just keep track of an interpolatedParams map which isn't merged back with params until all the other interpolation is finished?
Ah, good catch! I updated the PR to do just that, and added a test case for it as well.
blakepettersson
force-pushed
the
feature/enable-interpolation-for-cluster-generator-values
branch
from
3f64a40
to
c9a163e
Compare
Allow the interpolation of `values` found in the cluster generator.
This allows interpolation of `{{name}}`, `{{server}}`,
`{{metadata.labels.*}}` and `{{metadata.annotations.*}}`. See
argoproj/applicationset#371.
This interpolation could potentially be extended to the list and
duck-type generators if desired.
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Add a basic example of how values interpolation can be used with the cluster generator. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
The previous implementation was vulnerable to a billion-laughs attack,
where someone could interpolate values based upon other values,
something like:
```yaml
values:
lol1: lol
lol2: '{{values.lol1}}{{values.lol1}}' #
lol3: '{{values.lol2}}{{values.lol2}}{{values.lol2}}{{values.lol2}}'
```
To counteract that, instead of directly manipulating the `params` map,
we create a map to keep track of the interpolated values, and only
template the values which have been previously whitelisted. Once we go
through all the values, we then merge the interpolated values map back
to the `params` map.
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
blakepettersson
force-pushed
the
feature/enable-interpolation-for-cluster-generator-values
branch
from
c9a163e
to
28f0846
Compare
blakepettersson
deleted the
feature/enable-interpolation-for-cluster-generator-values
branch
Allow the interpolation of
valuesfound in the cluster generator.This allows interpolation of
{{name}},{{server}},{{metadata.labels.*}}and{{metadata.annotations.*}}. Seeargoproj/applicationset#371.
This interpolation could potentially be extended to the list and
duck-type generators if desired.
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: