Skip to content

Conversation

@VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Feb 5, 2025

This PR allows bridged providers to return secrets from data sources. This is a follow up from pulumi/pulumi#12710 to support this in the engine.

fixes #1051

@codecov
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.77%. Comparing base (908e7f7) to head (b6e6279).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2887      +/-   ##
==========================================
+ Coverage   67.75%   67.77%   +0.02%     
==========================================
  Files         328      329       +1     
  Lines       42132    42179      +47     
==========================================
+ Hits        28545    28586      +41     
- Misses      11997    12000       +3     
- Partials     1590     1593       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VenelinMartinov VenelinMartinov requested a review from a team February 6, 2025 15:28
@VenelinMartinov VenelinMartinov marked this pull request as ready for review February 6, 2025 15:29
@t0yv0
Copy link
Member

t0yv0 commented Feb 6, 2025

Can we include tests for Plugin Framework data sources?

ret, err = plugin.MarshalProperties(
props,
plugin.MarshalOptions{Label: fmt.Sprintf("%s.returns", label)})
plugin.MarshalOptions{
Copy link
Member

Choose a reason for hiding this comment

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

Do the tests still pass without this change?

Copy link
Contributor Author

@VenelinMartinov VenelinMartinov Feb 6, 2025

Choose a reason for hiding this comment

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

no, this is the change which fixes the issue

@t0yv0
Copy link
Member

t0yv0 commented Feb 6, 2025

Did the schema has already marked these as secret, or is this coming with this PR (and we have a test gap)? We should have schema mark these as secret.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 6, 2025

I don't think there is anything in the Pulumi schema for secret-ness, this seems to be a runtime concern.

Edit: There is a schema option for secretness: https://www.pulumi.com/docs/iac/using-pulumi/pulumi-packages/schema/#property but we don't use it anywhere in bridged providers. Resources and provider config too.

EDIT2: AWS uses it: https://github.com/pulumi/pulumi-aws/blob/c18a42565a455738d11a189512203d1bcd245a7e/provider/cmd/pulumi-resource-aws/schema.json#L172510

Cloudflare doesn't: https://github.com/pulumi/pulumi-cloudflare/blob/abcb69c6a01bc5656bc3f42924133658693138cf/provider/cmd/pulumi-resource-cloudflare/schema.json#L2

GCP uses that property too, I'll add a test for the schema. We seem to have been doing it correctly before though.

@t0yv0
Copy link
Member

t0yv0 commented Feb 6, 2025

You can just link an existing test from here if there is one that shows the PacakgeSpec has a secret flag as we expect, cool thanks.

@t0yv0 t0yv0 self-requested a review February 7, 2025 15:59
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

Blocking per convo with @Zaid-Ajaj as to need to double-check breaking/non-breaking concerns.


Anton
  11 minutes ago
as a user I have a program that calls listStorageAccountKeys() and suppose it also calls listStorageAccountKeysOutput()


Anton
  [11 minutes ago](https://pulumi.slack.com/archives/C9SGX9QA1/p1738943378695449?thread_ts=1738867572.305319&cid=C9SGX9QA1)
THis program used to work, but the provider didn't mark the results as secret.


Anton
  10 minutes ago
Now I'm upgrading to v-next of the provider, and it will now mark some of the results as secret.


Anton
  10 minutes ago
What happens to my program? Does error out? Does it warn? Specifically the listStorageAccountKeys() bit is suspect I think because it returns a plan promise.


Anton
  9 minutes ago
I need to know what happens accross every language :upside_down_face:

We have a suspicion that for languages like Go this will start breaking previously working programs. We need to have a conversation on whether this is a change we want to take directly or in a provider's next major. Yes it does implicate security but note also there may be users who worked around this by wrapping the output values in explicit secret flags, that will now break.

There's a few rollout options to consider.

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.

Functions return secret values as plain

2 participants