-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat: allow interpolation of cluster generator values #9254
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super interesting, thanks for writing it up! Could you add some docs about how it would be used?
5bcf40c
to
1c16a4a
Compare
Thanks for finding it useful 😄 I wrote some basic usage docs in |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 lol9
There 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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>
c9a163e
to
28f0846
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for your patience on the review!
Allow the interpolation of
values
found 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: