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

add new import type that is only resolved during rotation #439

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Jan 28, 2025

Design doc

Add an additional rotateOnly metadata for imports, (similar to merge.) Imports with rotateOnly set to true will only be evaluated during rotation, not during open, and are otherwise treated as unknown.

Additionally, fn::rotate now tolerates unknown inputs so that open will succeed when using values from such imports.

Combining these allows rotation credentials to be imported from a restricted environment, while enabling the rotated environment to be consumed by lower-privileged users.

imports:
- foo: {rotateOnly: true}

values:
  example:
    fn::rotate::provider:
      inputs: ${some_creds_from_foo}
      state: {}

Resolves https://github.com/pulumi/pulumi-service/issues/25240

nyobe added 8 commits January 28, 2025 11:08
imports can now either be a list, or an object representing multiple import types:

  imports:
    open:
    - a # imports resolved during open
    rotate:
    - b # imports only resolved during rotate

when imports is a list, it represents "open" imports, preserving existing behavior.
This reverts commit 3c61343.
This reverts commit e5bd5d9.
@nyobe nyobe force-pushed the claire/new-import-type branch from 7753126 to 73169d2 Compare January 29, 2025 22:09
@nyobe nyobe changed the title sketch new import type that is only resolved during rotation add new import type that is only resolved during rotation Jan 29, 2025
@nyobe nyobe force-pushed the claire/new-import-type branch from 73169d2 to a092b78 Compare January 29, 2025 22:33
@nyobe nyobe force-pushed the claire/new-import-type branch from a092b78 to 704a8a8 Compare January 29, 2025 22:38
@nyobe nyobe marked this pull request as ready for review January 29, 2025 22:52
@@ -482,6 +486,9 @@ func (e *evalContext) evaluateImport(myImports map[string]*value, decl *ast.Impo
return
}
val = imported.value
} else if rotateOnly && !e.rotating {
// this import should only be evaluated during rotation, and we're not rotating, so resolve it as unknown
val = &value{def: newMissingExpr("", nil), schema: schema.Always(), unknown: true}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know we talked about adding a new type of missingExpr for this, but I haven't quite wrapped my head around how it would work.

I think we need to keep track of the imported environment name somewhere? I'd like for a rotation provider to be able to return an error saying it tried to use an unknown value / you may not have permission to access , but I'm not sure how to recover that information from 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.

Ah, I'd put that in the evaluator itself--if the unknown value is accessed when not rotating, the evaluator should error. This could be done via a patch to evaluateExprAccess to handle the distinguished expression type in its type switch.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, though I suppose that wouldn't work, since it's not clear whether or not the value is actually used by the provider. Ick.

Copy link
Member

Choose a reason for hiding this comment

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

I almost wonder if we want rotators to have two input schemas? one that's user-facing and one that's internal? that would at least allow the evaluator to handle this instead of the rotator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the evaluator doesn't have the context to know if the value is used or not.

adding a second input schema can help give us this context, we could have one input schema that is validated during open and another that is validated during rotate.

this almost works, except that it seems like unknown values are not being treated as missing for required properties, so it's still validating successfully 😕 65c34a5

@nyobe nyobe mentioned this pull request Feb 7, 2025
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.

2 participants