Improve JWTAuthenticator Status #1851

benjaminapetersen · 2024-01-25T22:03:43Z

Improving the status messages of the JWT Authenticator.

# .Status was previously an empty field.
# full success .Status now looks like this:
status:
  conditions:
  - lastTransitionTime: "2024-02-23T19:31:36Z"
    message: authenticator initialized
    observedGeneration: 1
    reason: Success
    status: "True"
    type: AuthenticatorValid
  - lastTransitionTime: "2024-02-23T19:31:33Z"
    message: discovery performed successfully
    observedGeneration: 1
    reason: Success
    status: "True"
    type: DiscoveryURLValid
  - lastTransitionTime: "2024-02-23T19:31:33Z"
    message: issuer is a valid URL
    observedGeneration: 1
    reason: Success
    status: "True"
    type: IssuerURLValid
  - lastTransitionTime: "2024-02-23T19:31:36Z"
    message: successfully fetched jwks
    observedGeneration: 1
    reason: Success
    status: "True"
    type: JWKSFetchValid
  - lastTransitionTime: "2024-02-23T19:31:33Z"
    message: jwks_uri is a valid URL
    observedGeneration: 1
    reason: Success
    status: "True"
    type: JWKSURLValid
  - lastTransitionTime: "2024-02-23T19:31:36Z"
    message: the JWTAuthenticator is ready
    observedGeneration: 1
    reason: Success
    status: "True"
    type: Ready
  - lastTransitionTime: "2024-02-23T19:31:33Z"
    message: successfully parsed specified CA bundle
    observedGeneration: 1
    reason: Success
    status: "True"
    type: TLSConfigurationValid
  phase: Ready

Release note:

Adds meaningful .Status to JWTAuthenticator resource.  Improves validations.

codecov · 2024-02-12T22:21:59Z

Codecov Report

Attention: Patch coverage is 76.04167% with 92 lines in your changes are missing coverage. Please review.

Project coverage is 38.45%. Comparing base (f18d731) to head (f498cb3).

Files	Patch %	Lines
...ler/authenticator/jwtcachefiller/jwtcachefiller.go	84.22%	48 Missing and 2 partials ⚠️
test/testlib/client.go	0.00%	39 Missing ⚠️
internal/controllermanager/prepare_controllers.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1851      +/-   ##
==========================================
+ Coverage   38.14%   38.45%   +0.31%     
==========================================
  Files         346      347       +1     
  Lines       43638    43959     +321     
==========================================
+ Hits        16646    16906     +260     
- Misses      26477    26540      +63     
+ Partials      515      513       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cfryanr · 2024-02-13T19:07:36Z

@benjaminapetersen Please don't forget to add integration test coverage for the new feature. You can probably add coverage to some of the existing integration tests. To see an example of similar coverage, look for usages of the WaitForFederationDomainStatusConditions test helper in the integration tests.

test/integration/concierge_jwtauthenticator_status_test.go

benjaminapetersen · 2024-02-15T21:55:13Z

Pushed a fix for this failing integration test: https://ci.pinniped.dev/builds/2327880

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

test/testlib/client.go

cfryanr · 2024-02-20T20:20:43Z

@benjaminapetersen It seems like maybe you don't have the project's git hooks configured on your development workstation, because you did not update the copyright dates as enforced by the git hooks. Please see https://github.com/vmware-tanzu/pinniped/blob/main/CONTRIBUTING.md#pre-commit-hooks.

test/integration/concierge_jwtauthenticator_status_test.go

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

test/integration/concierge_jwtauthenticator_status_test.go

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

benjaminapetersen · 2024-02-26T20:09:21Z

Squashed.

- "Ready" condition & supporting conditions - Legacy "Phase" for convenience - Refactor newCachedJWTAuthenticator() func to improve ability to provide additional conditions - Update JWTAuthenticator.Status type - Update RBAC for SA to get/watch/update JWTAuthenticator.Status - Update logger to plog, add tests for logs & statuses - update Sync() to reduce enqueue when error is config/user managed, perhaps remove validateJWKSResponse()

- some format updates - add timestamp to test - fix order of expect,actual in some assertions - remove some commented code no longer needed

- paired with changes to e2e_test.go, adds Status.Condition assertions around JWTAuthenticators

- Add test to verify timestamps are particularly updated - Improve diff output in tests for actions - Make jwtauthenticator status tests parallel - Update copyright headers in multiple files

vmwclabot added the cla-not-required label Jan 25, 2024

benjaminapetersen changed the title ~~Improve JWTAuthenticator Status~~ WIP: Improve JWTAuthenticator Status Jan 25, 2024

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch from 6e9fd00 to 300f86f Compare January 31, 2024 16:44

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch 3 times, most recently from 3505ab7 to 9005d3d Compare February 12, 2024 21:55

benjaminapetersen changed the title ~~WIP: Improve JWTAuthenticator Status~~ Improve JWTAuthenticator Status Feb 12, 2024

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch 2 times, most recently from 0ca59e8 to f84183e Compare February 12, 2024 22:13

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch from 7075321 to d8545ac Compare February 13, 2024 19:02

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch 3 times, most recently from 181c2b8 to b22d4cb Compare February 15, 2024 17:15

benjaminapetersen commented Feb 15, 2024

View reviewed changes

test/integration/concierge_jwtauthenticator_status_test.go Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch from b22d4cb to 199f057 Compare February 15, 2024 21:55

benjaminapetersen commented Feb 16, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

benjaminapetersen commented Feb 16, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

benjaminapetersen commented Feb 16, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go Outdated Show resolved Hide resolved

benjaminapetersen commented Feb 16, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/testlib/client.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/testlib/client.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/integration/concierge_jwtauthenticator_status_test.go Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/integration/concierge_jwtauthenticator_status_test.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/integration/concierge_jwtauthenticator_status_test.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 20, 2024

View reviewed changes

test/integration/concierge_jwtauthenticator_status_test.go Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

cfryanr reviewed Feb 23, 2024

View reviewed changes

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch from 90afd5c to 13360eb Compare February 26, 2024 20:08

benjaminapetersen force-pushed the ben/status/jwt-authenticator branch from 13360eb to 73a20e2 Compare February 26, 2024 20:13

cfryanr approved these changes Feb 26, 2024

View reviewed changes

cfryanr force-pushed the ben/status/jwt-authenticator branch 2 times, most recently from 0eb7389 to c4c66a3 Compare February 27, 2024 19:02

benjaminapetersen and others added 12 commits February 27, 2024 15:45

ldap upstream watcher: rename local var for clarity

fd14a57

extract status comparison test helpers

4763934

Improve jwtcachefiller tests

084c311

- some format updates - add timestamp to test - fix order of expect,actual in some assertions - remove some commented code no longer needed

fix comment in testlib/client.go

09bd51f

add WaitForJWTAuthenticatorStatusPhase() integration helper

1a633ad

Add integration tests for JWTAuthenticators

73e4d31

- paired with changes to e2e_test.go, adds Status.Condition assertions around JWTAuthenticators

Update copyright year in modified files

1c7e704

Add Status & tests for jwks key fetching

42acf8d

Update jwk authenticator status integration tests

868ff9e

Update jwtauthenticator unit tests to check actions

e8482ab

- Add test to verify timestamps are particularly updated - Improve diff output in tests for actions - Make jwtauthenticator status tests parallel - Update copyright headers in multiple files

wait for JWTAuthenticator to be phase=ready in supervisor warnings test

f498cb3

cfryanr force-pushed the ben/status/jwt-authenticator branch from c4c66a3 to f498cb3 Compare February 27, 2024 23:45

cfryanr enabled auto-merge February 27, 2024 23:56

cfryanr merged commit 9978144 into main Feb 28, 2024
38 checks passed

cfryanr deleted the ben/status/jwt-authenticator branch February 28, 2024 00:41

benjaminapetersen mentioned this pull request Mar 13, 2024

WebhookAuthenticator .Status and validation improvements #1894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve JWTAuthenticator Status #1851

Improve JWTAuthenticator Status #1851

benjaminapetersen commented Jan 25, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

cfryanr commented Feb 13, 2024

benjaminapetersen commented Feb 15, 2024

cfryanr commented Feb 20, 2024

benjaminapetersen commented Feb 26, 2024

Improve JWTAuthenticator Status #1851

Improve JWTAuthenticator Status #1851

Conversation

benjaminapetersen commented Jan 25, 2024 • edited Loading

codecov bot commented Feb 12, 2024 • edited Loading

Codecov Report

cfryanr commented Feb 13, 2024

benjaminapetersen commented Feb 15, 2024

cfryanr commented Feb 20, 2024

benjaminapetersen commented Feb 26, 2024

benjaminapetersen commented Jan 25, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading