-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Support X509 Federation #11493
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
Support X509 Federation #11493
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
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.
@EZIOJQ This looks good overall. Just wanted to double-check that this field is GA and doesn't require an allowlist? The linked docs say it's in preview and that you need to contact folks to get it enabled - if that's still the case, we shouldn't add it to the provider yet.
...rd_party/terraform/services/iambeta/resource_iam_workload_identity_pool_provider_test.go.erb
Outdated
Show resolved
Hide resolved
16d6879
to
03cee96
Compare
This feature will be GA soon, and we will remove the allowlist in next few weeks. We can merge the change after the allowlist is removed |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
...rd_party/terraform/services/iambeta/resource_iam_workload_identity_pool_provider_test.go.erb
Show resolved
Hide resolved
03cee96
to
fa469eb
Compare
@melinath, we have this allowlist for half a year now. Are we okay to merge the change given it's long-term allowlist? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
fa469eb
to
39816a2
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 18 Click here to see the affected service packages
View the build log |
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.
Since the allowlist is about to come off anyway, I'd rather just wait until it's removed.
after discussion, we've decided to move forward with the allowlist in place. Marking for review |
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.
Seems straightforward and the tests are passing.
Co-authored-by: Jieqing(Jay) Chen <[email protected]>
Co-authored-by: Jieqing(Jay) Chen <[email protected]>
This PR adds support for X.509 federation support on workload identity pool. It fixes hashicorp/terraform-provider-google#18812.
For more details about how this feature works, check out the public doc here https://cloud.google.com/iam/docs/workload-identity-federation-with-x509-certificates