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

fix(swagger/oidc): fix swagger and oidc docs #1159

Merged
merged 11 commits into from
Sep 6, 2024
Merged

Conversation

wass3r
Copy link
Collaborator

@wass3r wass3r commented Jul 14, 2024

depends on go-vela/types#385

includes the following changes:

  • oidc and jwks endpoints don't need auth (fixed in swagger annotation)
  • fix swagger doc creation
    • this must have been broken for a while, it doesn't actually generate types, only references with x-go-package fields. apparently it's a bug with the release binaries(?!). using the go install variant of installing go-swagger the spec is produced properly with the same command (see No struct definition in swagger generate go-swagger/go-swagger#3107)
  • after the fixing the previous, i noticed that JWKSet was being documented as producing a Len and Clear field?! I changed up the type to match the response properly. maybe there's a better way yet.

includes the following changes:
- oidc and jwks endpoints don't need auth
- proper documentation for JWKSet type
- fix swagger doc creation
@wass3r wass3r requested a review from a team as a code owner July 14, 2024 05:30
Copy link

codecov bot commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.81%. Comparing base (372a430) to head (6fd48d4).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1159   +/-   ##
=======================================
  Coverage   52.81%   52.81%           
=======================================
  Files         551      551           
  Lines       19211    19211           
=======================================
  Hits        10146    10146           
  Misses       8501     8501           
  Partials      564      564           
Files with missing lines Coverage Δ
api/jwks.go 0.00% <ø> (ø)
api/oi_config.go 0.00% <ø> (ø)

@wass3r wass3r self-assigned this Jul 14, 2024
@wass3r
Copy link
Collaborator Author

wass3r commented Jul 14, 2024

I noticed it's still not quite right because now we have swagger model definitions in two places due to the types transition and it's still favoring go-vela/types and/or it's undeterministic :/

Note to self: also consider adding swagger flatten --with-flatten=remove-unused to remove unused definitions.

@wass3r wass3r marked this pull request as draft July 14, 2024 17:40
wass3r added a commit to go-vela/types that referenced this pull request Jul 14, 2024
the modified model annotations have moved to go-vela/server/api/types
where a duplicate of these annotations can be found. this change is
needed for accurate swagger doc generation. see
go-vela/server#1159
@plyr4
Copy link
Contributor

plyr4 commented Aug 2, 2024

so we're looking to drop usage of "github.com/lestrrat-go/jwx/v2/jwk" for the jwk.Set ?
it was kind of nice to not have to manage that type

@wass3r
Copy link
Collaborator Author

wass3r commented Aug 3, 2024

@plyr4 without this change, JWKSet is being documented as the following via go-swagger (which is very wrong):

    "JWKSet": {
      "type": "object",
      "title": "JWKSet is a wrapper of lestrrat-go/jwx/jwk.Set for API Swagger gen.",
      "properties": {
        "Clear": {
          "description": "Clear resets the list of keys associated with this set, emptying the\ninternal list of `jwk.Key`s, as well as clearing any other non-key\nfields",
          "type": "string"
        },
        "Len": {
          "description": "Len returns the number of keys in the set",
          "type": "integer",
          "format": "int64"
        }
      },
      "x-go-package": "github.com/go-vela/server/api/types"
    },

i looked through the package to see if there is an alternative, usable type, as i think @ecrupper likely also did - to no avail.

if we want to properly document what this produces in the swagger spec, we have to maintain something manually somewhere.

ideally, there is either a change in go-swagger or jwkset that would allow proper automatic documentation.

open to better ideas here, if there are any.

@wass3rw3rk wass3rw3rk marked this pull request as ready for review August 26, 2024 15:41
Copy link
Contributor

@plyr4 plyr4 left a comment

Choose a reason for hiding this comment

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

sure, if it still works for key discovery then it works for me!

@ecrupper ecrupper merged commit e0c2f7e into main Sep 6, 2024
11 of 13 checks passed
@ecrupper ecrupper deleted the fix/swagger-oidc branch September 6, 2024 15:27
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.

4 participants