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.

Sign up for GitHub

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

Jump to bottom

auth/jwt: adds user_claim_json_pointer and max_age to roles #1478

Merged
merged 6 commits into from
Jun 2, 2022
Merged

auth/jwt: adds user_claim_json_pointer and max_age to roles #1478

merged 6 commits into from
Jun 2, 2022

Conversation

austingebauer
Copy link
Contributor

@austingebauer austingebauer commented Jun 2, 2022 •
edited
Loading

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes: N/A

Release note for CHANGELOG:

* `resource/vault_jwt_auth_backend`: Adds support for `user_claim_json_pointer` and `max_age` (#1478)

Output from acceptance testing:

$ make testacc TESTARGS='-count=1 -run=TestAccJWTAuth'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -v -count=1 -run=TestAccJWTAuth -timeout 30m ./...
=== RUN   TestAccJWTAuthBackendRole_import
--- PASS: TestAccJWTAuthBackendRole_import (1.62s)
=== RUN   TestAccJWTAuthBackendRole_basic
--- PASS: TestAccJWTAuthBackendRole_basic (1.07s)
=== RUN   TestAccJWTAuthBackendRole_update
--- PASS: TestAccJWTAuthBackendRole_update (1.91s)
=== RUN   TestAccJWTAuthBackendRole_full
--- PASS: TestAccJWTAuthBackendRole_full (1.10s)
=== RUN   TestAccJWTAuthBackendRoleOIDC_full
--- PASS: TestAccJWTAuthBackendRoleOIDC_full (1.57s)
=== RUN   TestAccJWTAuthBackendRoleOIDC_disableParsing
--- PASS: TestAccJWTAuthBackendRoleOIDC_disableParsing (1.24s)
=== RUN   TestAccJWTAuthBackendRole_fullUpdate
--- PASS: TestAccJWTAuthBackendRole_fullUpdate (2.81s)
=== RUN   TestAccJWTAuthBackend
--- PASS: TestAccJWTAuthBackend (3.89s)
=== RUN   TestAccJWTAuthBackendProviderConfig
--- PASS: TestAccJWTAuthBackendProviderConfig (1.11s)
=== RUN   TestAccJWTAuthBackend_OIDC
--- PASS: TestAccJWTAuthBackend_OIDC (1.17s)
=== RUN   TestAccJWTAuthBackend_invalid
--- PASS: TestAccJWTAuthBackend_invalid (0.30s)
=== RUN   TestAccJWTAuthBackend_missingMandatory
--- PASS: TestAccJWTAuthBackend_missingMandatory (1.91s)
=== RUN   TestAccJWTAuthBackendProviderConfigConversionBool
--- PASS: TestAccJWTAuthBackendProviderConfigConversionBool (0.00s)
=== RUN   TestAccJWTAuthBackendProviderConfigConversionInt
--- PASS: TestAccJWTAuthBackendProviderConfigConversionInt (0.00s)
=== RUN   TestAccJWTAuthBackendProviderConfig_negative
    resource_jwt_auth_backend_test.go:380: true
--- SKIP: TestAccJWTAuthBackendProviderConfig_negative (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-vault/vault     21.384s

@austingebauer
Copy link
Contributor Author

Looks like the TestAccJWTAuthBackendRole_import test failed in CI. This is likely because the test is using Vault 1.10 and the new role parameter is not released yet. The test passed when ran locally.

@vinay-gopalan vinay-gopalan changed the base branch from main to release/vault-next June 2, 2022 08:24
@vinay-gopalan
Copy link
Contributor

Looks like the TestAccJWTAuthBackendRole_import test failed in CI. This is likely because the test is using Vault 1.10 and the new role parameter is not released yet. The test passed when ran locally.

Ah, yes. This is expected until we have access to Vault builds built off of the main branch running in containers for our CI!

For 1.10, we made use of the testutil.SkipTestEnvSet util function to skip tests and then populated an environment variable in CI. I believe we'll be doing something similar for 1.11 tests :)

benashz
benashz approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looking good. A few minor suggestions/comments. Then +1

vault/resource_jwt_auth_backend_role.go Outdated Show resolved Hide resolved
vault/resource_jwt_auth_backend_role.go Outdated Show resolved Hide resolved
vault/resource_jwt_auth_backend_role.go Outdated Show resolved Hide resolved
vault/resource_jwt_auth_backend_role_test.go Show resolved Hide resolved
@github-actions github-actions bot added size/M and removed size/S labels Jun 2, 2022
@austingebauer
Copy link
Contributor Author

@vinay-gopalan - Thanks for the explanation about the test skipping. Makes sense 👍 Any immediate suggestion? I could also hold on merging this until things are set up / ready.

@benashz
Copy link
Contributor

benashz commented Jun 2, 2022 •
edited
Loading

@vinay-gopalan - Thanks for the explanation about the test skipping. Makes sense 👍 Any immediate suggestion? I could also hold on merging this until things are set up / ready.

We use this env var to work around the lack of a vault dev instance to test against:

terraform-provider-vault/testutil/testutil.go

Line 23 in 7205d99

EnvVarSkipVaultNext = "SKIP_VAULT_NEXT_TESTS"

You can use that with

terraform-provider-vault/testutil/testutil.go

Line 45 in 7205d99

func SkipTestEnvSet(t *testing.T, envVars ...string) []string {

@austingebauer
Copy link
Contributor Author

@benashz - Thanks, that's what I was looking for. I've updated the tests that depend on a Vault 1.11 build to use that env var in 2fd9a5b.

@benashz benashz self-requested a review June 2, 2022 19:38
benashz
benashz approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

LGTM!

@austingebauer austingebauer merged commit 4fcdefb into release/vault-next Jun 2, 2022
@austingebauer austingebauer deleted the resource/auth_jwt_user_claim_max_age branch June 2, 2022 20:12
@benashz benashz added this to the 3.8.0 milestone Jun 14, 2022
marcboudreau pushed a commit to marcboudreau/terraform-provider-vault that referenced this pull request Nov 6, 2022
@austingebauer
auth/jwt: adds user_claim_json_pointer and max_age to roles (hashicor…
30186c4 
…p#1478)

* auth/jwt: adds user_claim_json_pointer and max_age to roles

* remove type assertions

* assert default false for user_claim_json_pointer when missing from config

* skip vault next in tests

* use d.Get() instead of d.GetOkExists()

* move SkipTestEnvSet out of PreCheck; set SKIP_VAULT_NEXT_TESTS=true in ci tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinay-gopalan vinay-gopalan vinay-gopalan approved these changes

@benashz benashz benashz approved these changes

Assignees
No one assigned
Labels
documentation feature-parity size/M
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@austingebauer @vinay-gopalan @benashz