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

Fix for __inputs secrets #2300 #2301

Merged
merged 7 commits into from Feb 6, 2023
Merged

Fix for __inputs secrets #2300 #2301

merged 7 commits into from Feb 6, 2023

Conversation

thomas11
Copy link
Contributor

@thomas11 thomas11 commented Feb 3, 2023

checkpointObject checks for secrets in the input, but in this case the input is of Kind "Secret" but ContainsSecret() is false. annotateSecrets(inputsPM, fromInputs) wouldn't do anything in any case because in this Read() call, fromInputs is empty.

I'm not quite sure yet that this is the best fix but it works in my local repro (described in the linked issue).

Fixes #2300

checkpointObject checks for secrets in the input, but in this case
the input is of Kind=="Secret" but ContainsSecret() is false.

I'm not quite sure yet that this is the best fix but it works in my
local repro.
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@@ -2955,6 +2955,11 @@ func checkpointObject(inputs, live *unstructured.Unstructured, fromInputs resour
}
}

inputsCopy := resource.NewObjectProperty(inputsPM)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than changing this logic, I think it may be better to change the behavior of the annotateSecrets method to handle the case where the input map is empty. That way we don't split the secret handling logic.

The logic block above may need to be adapted as well. The intent is that the data and stringData fields are always encrypted in the following places:

  1. The property fields on the resource
  2. The pulumi.com/lastAppliedConfiguration annotation on the resource. This is a string, so the entire value would be encrypted for Secret resources.
  3. The __inputs map that appears in the checkpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lblackstone I'm realizing your comment isn't quite enough for me to make changes.

The empty input map is only one issue. Another is that ContainsSecret() is false here, so that would require further changes to annotateSecrets. We need to check GetKind() but annotateSecrets works with property maps that don't even have this method.

As for data and stringdata, I don't see what needs to be changed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced offline - latest revision should reflect all the input. Thank you!

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@thomas11 thomas11 changed the title Speculative fix for __inputs secrets #2300 Fix for __inputs secrets #2300 Feb 6, 2023
@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment, and then LGTM

provider/pkg/provider/provider.go Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@thomas11 thomas11 enabled auto-merge (squash) February 6, 2023 22:47
@thomas11 thomas11 merged commit 2e094b6 into master Feb 6, 2023
@thomas11 thomas11 deleted the tkappler/inputs-secrets branch February 6, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secret.get does has the Secret's contents in plaintext in the statefile
3 participants