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

provider rotation #432

Merged
merged 30 commits into from
Jan 23, 2025
Merged

provider rotation #432

merged 30 commits into from
Jan 23, 2025

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Jan 15, 2025

Introduces a new provider type and verb to support rotating static credentials.

Rotator providers have an additional state input which is used as a write-back target. The result of invoking Rotate is persisted back into the environment to this key. This provides a stable location to write to without accidentally clobbering interpolations that might be used for other inputs.

The new fn::rotate function type is used to invoke this new type of provider. It behaves similarly to existing providers during Open, but during rotation the evaluator will invoke the Rotate methods and collect their outputs, which are returned to the caller. The caller is expected to persist these updates back into the environment as a new revision.

# long form
fn::rotate:
  rotator: provider
  inputs:
    # any config the provider needs to use during rotation
  state:
    # persisted state

# short form
fn::rotate::provider:
  inputs:
    # any config the provider needs to use during rotation
  state:
    # persisted state

Closes https://github.com/pulumi/pulumi-service/issues/24986

@nyobe nyobe force-pushed the claire/provider-rotation branch from be5df41 to 4e22ea2 Compare January 16, 2025 00:18
eval/patch.go Outdated Show resolved Hide resolved
provider.go Show resolved Hide resolved
eval/eval.go Outdated Show resolved Hide resolved
@nyobe nyobe marked this pull request as ready for review January 16, 2025 23:34
@nyobe nyobe requested a review from pgavlin January 16, 2025 23:35
eval/eval.go Outdated Show resolved Hide resolved
provider.go Show resolved Hide resolved
eval/eval.go Outdated Show resolved Hide resolved
eval/eval.go Outdated Show resolved Hide resolved
provider.go Show resolved Hide resolved
provider.go Show resolved Hide resolved
provider.go Show resolved Hide resolved
eval/eval.go Outdated Show resolved Hide resolved
eval/eval.go Outdated Show resolved Hide resolved
@nyobe nyobe force-pushed the claire/provider-rotation branch from 823ab99 to 9648bb6 Compare January 21, 2025 20:52
@nyobe nyobe force-pushed the claire/provider-rotation branch from 2e67fef to a208a03 Compare January 21, 2025 22:11
@nyobe nyobe force-pushed the claire/provider-rotation branch from 3a38658 to 2aefb1c Compare January 22, 2025 18:42
@nyobe nyobe force-pushed the claire/provider-rotation branch from 2aefb1c to 53ccc3c Compare January 22, 2025 18:51
@pgavlin
Copy link
Member

pgavlin commented Jan 22, 2025

I don't love the short form intermixing state and inputs. I would prefer that the short form just lifts provider from a property to part of the function name and retains both inputs and state as properties. This allows users to write things like:

  commonInputs:
    ...
  foo:
    fn::rotate::provider:
      inputs: ${commonInputs}
      state: ...

I think this is kind of a fundamental issue, FWIW. Even if we store state out-of-band, unless we reference it by path (which kills the ability to refactor via cut-and-paste), we'd need something in the provider inputs that serves as an identifier for the state.

ast/expr.go Show resolved Hide resolved
ast/expr.go Outdated
Comment on lines 805 to 808
if !ok && state != nil {
diags := syntax.Diagnostics{ExprError(stateExpr, "rotation state must be an object literal")}
return RotateSyntax(node, name, args, nil, nil, nil), diags
}
Copy link
Member

Choose a reason for hiding this comment

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

Just want to check, we are ok if there is no initial state set on the rotator? (This makes sense but I vaguely recall us mentioning that users would set at least current on this when adding a new rotator)

Copy link
Contributor Author

@nyobe nyobe Jan 22, 2025

Choose a reason for hiding this comment

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

I think it depends on the Rotator- the schema can be used to enforce it being present or optional. But I think it's also fine to make the key required 🤷‍♀️

eval/eval.go Outdated Show resolved Hide resolved
Comment on lines +1017 to +1018
inputs.export("").Value.(map[string]esc.Value),
state.export("").Value.(map[string]esc.Value),
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own knowledge, what does export("") do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is supposed to be the environment name which I think is just used for annotating source ranges?

Copy link
Member

Choose a reason for hiding this comment

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

yep that's right

@nyobe
Copy link
Contributor Author

nyobe commented Jan 22, 2025

I don't love the short form intermixing state and inputs. I would prefer that the short form just lifts provider from a property to part of the function name and retains both inputs and state as properties.

Yeah, hoisting the state out of inputs for the short form is a bit cheeky. I've removed it: 53dc66c

@nyobe nyobe force-pushed the claire/provider-rotation branch from 7333173 to 58732fa Compare January 22, 2025 22:05
@nyobe nyobe requested a review from pgavlin January 22, 2025 22:31
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

one nit on naming, but LGTM otherwise

ast/expr.go Outdated Show resolved Hide resolved
ast/expr.go Outdated Show resolved Hide resolved
ast/expr.go Outdated Show resolved Hide resolved
Comment on lines +1017 to +1018
inputs.export("").Value.(map[string]esc.Value),
state.export("").Value.(map[string]esc.Value),
Copy link
Member

Choose a reason for hiding this comment

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

yep that's right

eval/eval.go Outdated Show resolved Hide resolved
provider.go Outdated Show resolved Hide resolved
Co-authored-by: Pat Gavlin <[email protected]>
@nyobe nyobe force-pushed the claire/provider-rotation branch from aa8712f to d5f036c Compare January 23, 2025 00:11
@nyobe nyobe merged commit 6198f06 into main Jan 23, 2025
6 checks passed
@nyobe nyobe deleted the claire/provider-rotation branch January 23, 2025 00:26
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.

4 participants