-
Notifications
You must be signed in to change notification settings - Fork 155
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
CI tests for OIDC authentication #3074
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
af1b52d
to
1c96bd6
Compare
c6b0e4d
to
edac482
Compare
711c018
to
1cb2033
Compare
@@ -538,25 +538,22 @@ func arrayValue(vars resource.PropertyMap, prop resource.PropertyKey, envs []str | |||
return vals | |||
} | |||
|
|||
func durationFromConfig(vars resource.PropertyMap, prop resource.PropertyKey, envs []string) (time.Duration, error) { | |||
// returns a pointer so we can distinguish between a zero value and a missing value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: slight preference of adding more Go files in under ./provider instead of growing "resources.go" it's very big already, but certainly not blocking on this!
version: v2.4.0 | ||
- name: Make upstream | ||
run: make upstream | ||
- name: Run selected tests with manual web identity/OIDC auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this works I guess but do we have any alternatives that don't create a brand new test target for this particular test to keep the workflows a little simper?
What are the extra requirements for the test? I see you need OIDC_ROLE_ARN but perhaps this could have been exposed to all tests? Then you got aws-actions/configure-aws-credentials@v4
that's assuming the role with role-to-assume
. Is that the only way to do it?
Can Pulumi program under test assume this role, perhaps by using explicit provider configuration?
If it can't, can the test harness take care of it in the Go code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a copy of the test
job because it runs aws-actions/configure-aws-credentials
and I want to run it, too, but with a different configuration. If I could edit this file directly, I could just append the steps to the existing test
job but in ci-mgmt, I can't.
This new OIDC test job runs two go test
runs:
- with manual OIDC configuration
- with
aws-actions/configure-aws-credentials
I believe I could port (1) to the existing test job. Since (1) proves that OIDC works, (2) mainly tests the OIDC part of aws-actions/configure-aws-credentials
which we don't own. So I guess we could just drop it, although I would have liked to have an e2e test for a common user scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. It makes sense to have an e2e test for a common user scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a few suggestions to simplify but not blocking, this looks super useful.
eed21b6
to
884c6d9
Compare
This reverts commit 385a96a.
Test web identity (OIDC) authentication in CI for regression testing.
The approach is to pick one of the existing tests in
examples
and run it two additional times, authenticating via web identity/OIDC:aws-actions/configure-aws-credentials
I don't love the duplication of much of the workflow definition for YAML but maybe that's the trade-off here for using ci-mgmt?
Related: #3084, pulumi/registry#3567