Skip to content
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

Pulumi preview/up --diff for kubernetes resources does not display diffs of properties in the same level of nesting if one references a fully-encrypted resource by its name #1576

Closed
bob-bins opened this issue May 13, 2021 · 14 comments
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec last-applied-configuration Issues related to the last-applied-configuration annotation resolution/fixed This issue was fixed

Comments

@bob-bins
Copy link

bob-bins commented May 13, 2021

Pulumi preview/up --diff does not display diffs of properties in the same level of nesting if one references a fully-encrypted resource by its name (or likely any other of its properties).

Expected behavior

If I change a deployment image I expect to see:

      ~ spec: {
          ~ template: {
              ~ spec: {
                  ~ containers: [
                      ~ [0]: {
                            ~ image: "oldimagename" => "newimagename"
                            }
                    ]
                }
            }
        }

Current behavior

If I change a deployment image I actually see:

    ~ kubernetes:apps/v1:Deployment: (update)
      ~ spec: {
          ~ template: {
              ~ spec: {
                  ~ containers: [
                      ~ [0]: {
                            }
                    ]
                }
            }
        }

Steps to reproduce

Create these resources

const secret = new Secret("secret", {
  stringData: {
    LALALA: pulumi.secret("lalalala"),
  },
})

const deployment = new Deployment("nginx", {
  metadata: {
    name: "nginx",
  },
  spec: {
    replicas: 1,
    selector: {
      matchLabels: { app: "nginx" },
    },
    template: {
      metadata: {
        labels: { app: "nginx" },
      },
      spec: {
        containers: [
          {
            envFrom: [
              {
                secretRef: { name: secret.metadata.name },
              },
            ],
            env: [
              {
                name: "LOLOLO",
                value: "lolololo",
              },
            ],
            name: "nginx",
            image: "nginx",
          },
        ],
      },
    },
  },
})

Then change "lolololo" to any other string. See that the preview diff does not contain useful info.

Then remove secretRef: { name: secret.metadata.name },. Apply the change. Then change the env var in the deployment again and see that the diff now displays correctly

Workaround

Wrap the "false" secrets in unsecret()

@lblackstone lblackstone transferred this issue from pulumi/pulumi May 13, 2021
@lblackstone lblackstone added the kind/bug Some behavior is incorrect or out of spec label May 13, 2021
@lblackstone
Copy link
Member

I think this issue may be specific to the k8s provider diffing, and could also be related to client-side preview.

@bob-bins Do you see the same behavior if you use the enableDryRun Provider flag?

@bob-bins
Copy link
Author

bob-bins commented May 14, 2021

same output when I enable that flag and run a preview after it is enabled

@bob-bins
Copy link
Author

bob-bins commented May 21, 2021

Final findings (I also updated the ticket for proper steps to reproduce):

  • If a Deployment references a Secret (e.g. in envFrom) that is fully encrypted in the statefile (does not have the kubectl.kubernetes.io/last-applied-configuration annotation in plaintext), the Deployment's state at that level of nesting will also be encrypted even though it contains NO secret values. This is what causes the preview diff to be useless.
    • There is no good workaround for this. If you instead reference the secret by a hardcoded string, the statefile and diff will be correct, but then the deployment won't automatically replace the pods when values in the secret change
  • This doesn't contribute toward the bug in this ticket but is a residual bug I came across: If you create a k8s resource without marking a value as a secret, and then later change it to a secret without changing any values in that resource, the statefile will correctly encrypt its value BUT it will NOT remove or encrypt kubectl.kubernetes.io/last-applied-configuration which will have it in plaintext. The user will need to make some change to the resource to remove that annotation from the statefile.

@bob-bins bob-bins changed the title Pulumi preview/up --diff does not display diffs of properties in arrays Pulumi preview/up --diff does not display diffs of properties referencing a fully-encrypted resource by its name May 21, 2021
@bob-bins bob-bins changed the title Pulumi preview/up --diff does not display diffs of properties referencing a fully-encrypted resource by its name Pulumi preview/up --diff does not display diffs of properties in the same level of nesting if one references a fully-encrypted resource by its name May 21, 2021
@lblackstone
Copy link
Member

Ah, thanks for narrowing that down. I'm hoping that we will be able to fix this as a part of #1556

@bob-bins
Copy link
Author

bob-bins commented Jun 16, 2021

@lblackstone it looks like #1556 may be delayed. Wondering if it's possible to put a resource on this ticket, as literally every one of our production previews results in a useless diff since all of our services have at least one secret.

@lblackstone
Copy link
Member

Sorry for the delay. This is still high on the priority list, and I expect to continue work on it soon.

@bob-bins bob-bins changed the title Pulumi preview/up --diff does not display diffs of properties in the same level of nesting if one references a fully-encrypted resource by its name Pulumi preview/up --diff for kubernetes resources does not display diffs of properties in the same level of nesting if one references a fully-encrypted resource by its name Jul 9, 2021
@nesl247
Copy link

nesl247 commented Jul 28, 2021

Even when using server side diff (enabled manually), I'm still having this problem. To be clear, what is happening is this:

I have a Service that uses deployment.metadata.labels as its selector. The Deployment references a secret for imagePullSecrets via imagePullSecret.metadata.name.

Because of this, the selector is showing up as [secret] in the diffs, which obviously doesn't make any sense.

@lblackstone
Copy link
Member

Even when using server side diff (enabled manually), I'm still having this problem.

This is expected until #1659 is done.

@bob-bins
Copy link
Author

bob-bins commented Aug 5, 2021

Just discovered that a workaround for this is wrapping the "non-secret" secret value in unsecret()

@shinebayar-g
Copy link

Any progress on this? Eagerly waiting for this specific feature.

@lblackstone
Copy link
Member

We're still making progress on this, and have been fixing a number of bugs related to server-side apply support, which is needed to complete #1556

Once that issue is done, we should be able to fix #1659 and close this issue.

@aaronlevy
Copy link

Any updates on this? We're seeing this (or a similar issue). The part that is making it hard to track down is that some manifests have no env var secret references, but still display this behavior?

@lblackstone
Copy link
Member

@aaronlevy It's still in progress, but my understanding is that you can use the unsecret() function around any .metadata.name reference to work around in the meantime.

The reason this is happening is fairly involved:

  1. Pulumi's k8s provider currently uses the last-applied-configuration annotation (issue Remove last-applied-configuration annotation for resources managed with server-side diff #1659)
  2. For Secret resources, we automatically encrypt inputs to avoid leaking sensitive values into the state and that annotation.
  3. Any secret value in a map or array causes Pulumi to treat the entire map or array as secret since the ordering is indeterminate.
  4. As a result of (3), If the last-applied-configuration annotation contains a secret value, then all of the other metadata is also marked as secret. This includes the .metadata.name property.
  5. If another resource references the .metadata.name property, then it transitively becomes secret as well. This unfortunately leads to the poor diff behavior you're seeing since every field in the spec map becomes a "secret".

@lblackstone
Copy link
Member

Fixed by #2445

@lblackstone lblackstone added the last-applied-configuration Issues related to the last-applied-configuration annotation label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec last-applied-configuration Issues related to the last-applied-configuration annotation resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

5 participants