Fix eks cluster PRC replace #4415
Conversation
VenelinMartinov
changed the base branch from
master
to
vvm/enable_prc_by_default
VenelinMartinov
changed the base branch from
vvm/enable_prc_by_default
to
master
VenelinMartinov
changed the base branch from
master
to
vvm/enable_prc_by_default
| @@ -2192,6 +2192,16 @@ compatibility shim in favor of the new "name" field.`) | |||
| "node_group_name": tfbridge.AutoName("nodeGroupName", 255, "-"), | |||
| }, | |||
| }, | |||
| "aws_eks_cluster": { | |||
| TransformFromState: func(_ context.Context, pm resource.PropertyMap) (resource.PropertyMap, error) { | |||
| // if the defaultOutboundAccessEnabled property is not set, set it to the default value of true | |||
There was a problem hiding this comment.
| // if the defaultOutboundAccessEnabled property is not set, set it to the default value of true | |
| // if the bootstrapSelfManagedAddons property is not set, set it to the default value of true |
| @@ -2192,6 +2192,16 @@ compatibility shim in favor of the new "name" field.`) | |||
| "node_group_name": tfbridge.AutoName("nodeGroupName", 255, "-"), | |||
| }, | |||
| }, | |||
| "aws_eks_cluster": { | |||
| TransformFromState: func(_ context.Context, pm resource.PropertyMap) (resource.PropertyMap, error) { | |||
There was a problem hiding this comment.
I might be missing something, but why is the State Upgrader not working here? Upstream configures it here:
There was a problem hiding this comment.
Ah, great find! This is likely pulumi/pulumi-terraform-bridge#2081
There was a problem hiding this comment.
That is still not rolled out to AWS. We could roll it out together with PRC.
There was a problem hiding this comment.
Can we opt in specific resources only to pulumi/pulumi-terraform-bridge#2081?
I think both are great enhancements, but rolling them out at the same time makes it harder to reason about potential issues that come up
There was a problem hiding this comment.
No, we can't currently filter which resources to enable pulumi/pulumi-terraform-bridge#2081 for.
I have confirmed that indeed that fixes the EKS replacement.
There was a problem hiding this comment.
We have so far seen no issues with either change 🤞, so it might be fine to roll out together.
There was a problem hiding this comment.
I'd vote for rolling them out separately. pulumi/pulumi-terraform-bridge#2081 Seems relatively straight forward to roll out (famous last words), so wdyt about doing that right away?
There was a problem hiding this comment.
I am unsure of the effects of pulumi/pulumi-terraform-bridge#2081 without PRC - we have not tested that anywhere, so I'd be hesitant to do that.
There was a problem hiding this comment.
There were concerns that 2081 might uncover latent panics which were hidden. The non-PRC bridge is known to transform some properties incorrectly, so running upgrades which were not run before could conceivably uncover some issues.
There was a problem hiding this comment.
If rolling the two together is a big concern, we could fix the EKS cluster as in this PR. This PR essentially re-implements the EKS cluster migration.
PRC might uncover similar issues in other resources though.
|
closing in favour of #4416, thanks @flostadler! |
|
4416 did not fix the issue: #4410 (comment) Reopening this. |
VenelinMartinov
changed the base branch from
vvm/enable_prc_by_default
to
t0yv0/prc
There was a problem hiding this comment.
Per our discussion today, the root cause is pulumi/pulumi-terraform-bridge#1667 that's not sending the right data into state migrations yes? Worth mentioning in the comment why this change is added, linking the root cause.
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
The eks:Cluster resource has received a new parameter with ForceNew and a default. When upgrading from an old version before the parameter was added this triggers a replace.
This PR adds a workaround for that - when the state is read the
bootstrapSelfManagedAddonsparameter is added with its default value if not present.covered by
TestEKSClusterUpgradepartially fixes #4410
stacked on #4403