Allow pulumi.Output in EKS Addon configurationValues - #1632
Conversation
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
rquitales
left a comment
rquitales
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR and helping clean up our codebase! Overall the PR looks like a great addition. I'll test this change on the side and merge when possible.
| extends Omit<aws.eks.AddonArgs, "resolveConflicts" | "clusterName" | "configurationValues"> { | ||
| cluster: Cluster; | ||
| configurationValues?: object; | ||
| configurationValues?: pulumi.Input<object>; |
There was a problem hiding this comment.
Note: This should be a safe change as the API is currently modeled as an Input already (ref) so this isn't a breaking change.
There was a problem hiding this comment.
Indeed! The types were telling me that an Input should be fine but then it put Calling [toJSON] on an [Output<T>] is not supported in my JSON blob
| ...args, | ||
| clusterName: cluster.core.cluster.name, | ||
| configurationValues: JSON.stringify(args.configurationValues), | ||
| configurationValues: stringifyAddonConfiguration(args.configurationValues), |
There was a problem hiding this comment.
We might need to be more careful here. stringifyAddonConfiguration changes the ordering, and may cause replacements for existing users. We'll need to test this out.
There was a problem hiding this comment.
In order to avoid changing the order for users who are already utilising this, would it be safer to just use pulumi.jsonStringify(args.configurationValues) ?
I think it would be safe to assume that if the user required a deterministic order, they would have ran into this issue already and either worked around the wrapper component or implemented their own ordering solution, so it's a bit too late to add now.
|
I was just doing some refactoring of my component that handles EKS addons and ran into this issue again. I don't think it's been fixed elsewhere. @rquitales could we have a look at this again? |
|
@pulumi-eon please review |
Review Verdict: Comment OnlyThe maintainer's own unresolved review comment flags that switching to the key-sorting stringifyAddonConfiguration helper (vs. the prior order-preserving JSON.stringify) will produce an update diff for existing Addon users on upgrade; I confirmed via the upstream AWS provider source that configuration_values isn't ForceNew (so no replacement), but that specific tradeoff was never explicitly confirmed or resolved in the thread, so a maintainer should sign off on it before merging. View session · Was this review helpful? Yes · No |
![eon-pulumi-agent[bot]](https://avatars.githubusercontent.com/in/4080224?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a small, well-scoped fix: it widens AddonOptions.configurationValues to pulumi.Input<object> and routes it through the already-existing, already-tested stringifyAddonConfiguration helper (previously only used by VpcCniAddon/cluster.ts), and switches the super(...) options merge to pulumi.mergeOptions for more robust option merging. I traced mergeOptions's actual implementation and confirmed it preserves the old spread's behavior for this call site (parent always wins, other opts pass through untouched), and confirmed JSON.stringify(undefined) and stringifyAddonConfiguration(undefined) both yield undefined — no regression there. Security, correctness, and AGENTS.md/REVIEW.md compliance passes found no issues (no AGENTS.md/REVIEW.md exists in this repo), and CI has not run (this repo requires a maintainer to trigger acceptance tests on fork PRs, and none has been triggered yet on this PR).
One point from the existing review thread is still open and worth a maintainer's attention before merging: stringifyAddonConfiguration sorts JSON keys deterministically, whereas the previous JSON.stringify preserved insertion order. @rquitales flagged this ("may cause replacements for existing users... we'll need to test this out") and @Makeshift suggested using plain pulumi.jsonStringify instead to avoid changing order for existing users — neither concern was followed up on in code or a later comment. I checked the upstream AWS provider resource definition: configuration_values is Optional/Computed but not ForceNew, so this will not cause a resource replacement as originally worried — but it will cause a one-time UpdateAddon API call (a real diff in pulumi preview/up) for any existing Addon user whose configurationValues keys aren't already alphabetically sorted, immediately after upgrading to a version containing this fix. The same tradeoff was already shipped previously for cni-addon.ts/cluster.ts, so this isn't a novel risk, but it is a genuine, previously-raised, still-unresolved behavior change for the base Addon class specifically, and the maintainer's stated intent to "test this change on the side" before merging doesn't appear to have concluded. Flagging as Important so a maintainer can explicitly confirm this tradeoff is acceptable (or preserve ordering) before merging.
Also noted two non-blocking test-coverage nits: the constructor's use of pulumi.mergeOptions and stringifyAddonConfiguration with an Input-typed value are not directly exercised by any test that instantiates Addon (only the standalone stringifyAddonConfiguration helper is unit tested).
| ...args, | ||
| clusterName: cluster.core.cluster.name, | ||
| configurationValues: JSON.stringify(args.configurationValues), | ||
| configurationValues: stringifyAddonConfiguration(args.configurationValues), |
There was a problem hiding this comment.
Important — logic
Switching from JSON.stringify(args.configurationValues) to stringifyAddonConfiguration(args.configurationValues) changes the key ordering of the resulting JSON string (stringifyAddonConfiguration sorts keys deterministically; JSON.stringify preserves insertion order). This was raised by rquitales in review ("may cause replacements for existing users. We'll need to test this out.") and Makeshift proposed using plain pulumi.jsonStringify instead to avoid changing order for users already relying on this. Neither concern was addressed in a follow-up commit or comment.
Either confirm (and note in the PR) that the one-time reconciliation update is acceptable — the same tradeoff is already shipped for cni-addon.ts/cluster.ts call sites — or preserve key order for the base Addon class (e.g. via pulumi.jsonStringify without the sorting replacer) as Makeshift suggested, so existing users are not surprised by an unannounced diff on upgrade.
Why this matters
For every existing Addon whose configurationValues keys are not already alphabetically sorted, upgrading to this version will produce a differently-ordered (but semantically identical) configuration_values string. Checking the upstream AWS provider resource, configuration_values is Optional/Computed but not ForceNew, so this triggers an UpdateAddon API call (not a resource replacement) on the next pulumi up for every affected existing deployment — a real, previously-flagged behavior change that was never confirmed as safe or documented.
|
|
||
| super("eks:index:Addon", name, args, { | ||
| ...opts, | ||
| super("eks:index:Addon", name, args, pulumi.mergeOptions(opts, { |
There was a problem hiding this comment.
Nit — tests
The Addon constructor's super(...) call now uses pulumi.mergeOptions(opts, { parent: cluster }) instead of a plain object spread. No test instantiates new Addon(...) to verify caller-supplied opts (e.g. provider, dependsOn) still merge correctly with the injected parent.
Consider adding a lightweight test that constructs new Addon(...) with representative opts and asserts construction succeeds with the expected parent/provider.
Why this matters
mergeOptions has different merge semantics than a plain spread for collection-valued options (dependsOn, aliases, providers), so a future regression in how opts combine with parent would not be caught by any existing test.
| ...args, | ||
| clusterName: cluster.core.cluster.name, | ||
| configurationValues: JSON.stringify(args.configurationValues), | ||
| configurationValues: stringifyAddonConfiguration(args.configurationValues), |
There was a problem hiding this comment.
Nit — tests
configurationValues now accepts pulumi.Input and is passed through stringifyAddonConfiguration, but no test instantiates Addon with an Output/Promise-wrapped configurationValues to confirm the constructor forwards it correctly end-to-end.
Consider adding a test that creates new Addon(...) with an Output-wrapped configurationValues and asserts the underlying aws.eks.Addon receives the expected JSON string.
Why this matters
This is the actual behavioral fix the PR delivers; stringifyAddonConfiguration itself is well-tested in isolation, but the constructor wiring that motivated this PR has no direct test.
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
@pulumi-eon rereview |
Review Verdict: Comment OnlyThe latest push (fc73ab7) correctly resolves the previously-open Important finding (key-ordering regression) — verified independently that pulumi.jsonStringify without a replacer preserves insertion order — but it also reverted the addon.test.ts additions, so the exact behavior this PR fixes (Output-wrapped configurationValues forwarding) and the mergeOptions opts-merging now have zero automated test coverage, and the acceptance-test CI workflow hasn't run against this commit yet (fork PR awaiting maintainer trigger). That's weak verification evidence for a behavioral fix, so I'm asking for tests/CI confirmation rather than approving. View session · Was this review helpful? Yes · No |
24c2a22 to
fc73ab7
Compare
|
PR is now waiting for a maintainer to run the acceptance tests. |
There was a problem hiding this comment.
This is a re-review. Since my last pass, the PR was updated again (now at commit fc73ab7) with a different, more targeted fix than what I last reviewed:
- The previously-open Important finding — switching to the key-sorting
stringifyAddonConfigurationhelper would cause a spurious update diff for existing users — is now resolved. The constructor was changed to callpulumi.jsonStringify(args.configurationValues)directly (no sorting replacer), which I independently verified preserves the original key insertion order (matching the oldJSON.stringifybehavior) while still correctly resolving nestedOutput/Promisevalues. This directly addresses the concern raised earlier in the thread about an unannounced ordering change for existing users. - However, the same push also reverted the
addon.test.tsadditions from the prior commit, so the two test-coverage Nits I flagged previously (no test exercisesAddonwith caller-suppliedopts, no test exercisesAddonwith anOutput-wrappedconfigurationValues) are effectively open again — the tests that closed them are gone. I've left inline notes on the relevant lines. Worth noting: one of the removed tests (for opts-merging) was also vacuous — it asserted onclusterName, which doesn't actually depend on the mergedopts— so a straight re-add of the old test wouldn't fully close that gap either. - I independently verified the implementation itself is correct:
tsc --noEmitcompiles cleanly, the existingstringifyAddonConfigurationunit tests pass, and I confirmed via a standalone check thatpulumi.jsonStringifywithout a replacer preserves key order and resolves nested Promises/Outputs as expected. - Security, correctness (control flow / error handling), and AGENTS.md/REVIEW.md compliance passes found no other issues (no AGENTS.md or REVIEW.md exists in this repo).
- CI: the full acceptance-test workflow (lint/build/test/Sentinel) hasn't run against this latest commit yet — this repo requires a maintainer to trigger it on fork PRs via
/run-acceptance-tests, and it hasn't been re-triggered since this last push. Only the lightweight automatic checks (PR comment bot, changelog check) have completed so far.
Given the fix now correctly resolves both the original bug (Output not supported in configurationValues) and the key-ordering regression from the intermediate commit, but the specific behavior it fixes currently has zero automated test coverage and the acceptance-test suite hasn't run against this commit yet, I'm leaving this as a comment rather than approving outright — a maintainer should confirm CI passes and ideally restore test coverage for the Output-wrapped-configurationValues path before merging.
| pulumi.mergeOptions(opts, { | ||
| parent: cluster, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Nit — tests
The Addon constructor's super(...) call uses pulumi.mergeOptions(opts, { parent: cluster }) to merge caller-supplied opts with the injected parent, but there is currently no test that instantiates new Addon(...) with opts (e.g. dependsOn, protect, retainOnDelete) and asserts those options are actually honored alongside the injected parent. A test covering this was added earlier in this PR's history and then removed in the latest push.
Recommend re-adding a test that constructs new Addon(...) with representative opts and asserts on something that actually depends on the merge succeeding (not just that clusterName resolves, which is independent of opts entirely and wouldn't catch a broken merge).
<details><summary>Why this matters</summary>
mergeOptions has different semantics than a plain object spread for collection-valued options (dependsOn, aliases, providers); a future regression in how caller opts combine with the injected parent would not be caught by any existing test.
| configurationValues: JSON.stringify(args.configurationValues), | ||
| configurationValues: args.configurationValues | ||
| ? pulumi.jsonStringify(args.configurationValues) | ||
| : undefined, |
There was a problem hiding this comment.
Nit — tests
configurationValues now accepts pulumi.Input<object> and is forwarded via args.configurationValues ? pulumi.jsonStringify(args.configurationValues) : undefined, which is the actual bug fix this PR delivers (previously JSON.stringify on an Output threw Calling [toJSON] on an [Output<T>] is not supported). No test instantiates Addon with an Output/Promise-wrapped configurationValues to confirm this works end-to-end. A test covering exactly this was added earlier in this PR's history and then removed when the implementation switched from the stringifyAddonConfiguration helper to this inline jsonStringify call.
Recommend re-adding a test that creates new Addon(...) with an Output-wrapped configurationValues and asserts the underlying aws.eks.Addon receives the expected JSON string with keys in their original (unsorted) order — this is the core behavior this PR is meant to fix, and it currently has zero automated coverage.
Why this matters
I independently verified that pulumi.jsonStringify without a replacer does correctly preserve object key insertion order and resolves nested Promises/Outputs, so the implementation itself appears correct. But without a test exercising this exact path, a future refactor could silently reintroduce either the original bug (Output not supported) or the key-reordering regression that was just fixed, with nothing to catch it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KJBbgW1iMuzNPhFfEF9Cah
fc73ab7 to
fc22f3c
Compare
|
PR is now waiting for a maintainer to run the acceptance tests. |
4 participants
Proposed changes
Someone went through the effort of creating a stringify function and forgot to use it 😁
Edit by @guineveresaenger: To preserve ordering I took the author's suggestion and used
pulumi.jsonStringify. This should fix up the PR enough to be mergeable. If a reviewer would like ordering-specific testing, we can do so; however this is a minor enough refactor that I do not think explicit test is warranted.