feat(rest-api): add runtime JWT issuer create/delete - #4637
Conversation
Allow Provider Admins to create, list, get, and delete external JWT issuers at runtime via DB-backed API, without ConfigMap edits or pod rolls. No update API — change via delete + create. Empty issuer table keeps today's static-only behavior. - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) - [ ] **This PR contains breaking changes** - [x] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) Signed-off-by: Parham Armani <parmani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe change adds database-backed issuer persistence and administration. It combines static and database issuers, validates organization mappings, reloads issuer state at runtime, exposes Provider Admin API endpoints, and synchronizes changes through PostgreSQL notifications. ChangesRuntime issuer management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderAdmin
participant CreateIssuerHandler
participant IssuerDAO
participant Config
participant PostgreSQL
ProviderAdmin->>CreateIssuerHandler: PUT /issuer
CreateIssuerHandler->>Config: ValidateCombinedIssuers
CreateIssuerHandler->>IssuerDAO: Create issuer transaction
IssuerDAO->>PostgreSQL: INSERT issuer
PostgreSQL-->>Config: issuer_changed notification
Config->>IssuerDAO: ReloadDBIssuers
Config-->>CreateIssuerHandler: updated issuer registry
CreateIssuerHandler-->>ProviderAdmin: issuer response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (13)
rest-api/db/pkg/db/model/issuer_test.go (2)
96-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for name uniqueness and for recreation after a soft delete.
testIssuerSetupSchemacreatesissuer_name_idx, but no test exercises it. More importantly, the PR documents issuer updates as delete-then-recreate. That workflow only works because both unique indexes are partial (WHERE deleted IS NULL). No test protects that predicate today, so a future migration change that drops theWHEREclause would break the documented update path silently.Add two cases: a duplicate
namewith a different URL must fail, and a soft-deleted URL must be recreatable.💚 Proposed tests
func TestIssuerSQLDAO_UniqueConstraints(t *testing.T) { ctx := context.Background() dbSession := testIssuerInitDB(t) defer dbSession.Close() testIssuerSetupSchema(t, dbSession) dao := NewIssuerDAO(dbSession) create := func(name, url string) (*Issuer, error) { var out *Issuer err := db.WithTx(ctx, dbSession, func(tx *db.Tx) error { var derr error out, derr = dao.Create(ctx, tx, IssuerCreateInput{Name: name, IssuerURL: url, JWKSUrl: url + "/jwks"}) return derr }) return out, err } t.Run("DuplicateNameRejected", func(t *testing.T) { _, err := create("issuer-a", "https://a.example.com") require.NoError(t, err) _, err = create("issuer-a", "https://b.example.com") require.Error(t, err) assert.True(t, (&db.PostgresErrorChecker{}).IsUniqueConstraintError(err)) }) t.Run("RecreateAfterSoftDelete", func(t *testing.T) { created, err := create("issuer-c", "https://c.example.com") require.NoError(t, err) require.NoError(t, db.WithTx(ctx, dbSession, func(tx *db.Tx) error { return dao.Delete(ctx, tx, created.ID) })) // The partial indexes must allow reusing the URL and name of a deleted row. _, err = create("issuer-c", "https://c.example.com") require.NoError(t, err) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/issuer_test.go` around lines 96 - 121, Add coverage alongside TestIssuerSQLDAO_DuplicateURLRejected for both unique constraints: verify creating a second issuer with the same Name but a different IssuerURL returns a unique-constraint error, and verify an issuer deleted through dao.Delete can be recreated with the same name and URL without error. Use separate subtests or isolated setup so the cases do not share database state.
38-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree new test files use flat, comment-delimited phases instead of named
t.Runsubtests. The shared root cause is one convention gap: each test marks its cases with trailing or leading comments that already read as subtest names. Promote those comments to named subtests so a failure reports its phase and so later assertions still run when an earlier one fails.
rest-api/db/pkg/db/model/issuer_test.go#L38-L94: wrap the// Create,// GetByID,// GetAll, and// Delete (soft)phases ofTestIssuerSQLDAO_CRUDin orderedt.Runsubtests that share thecreatedvariable.rest-api/api/internal/config/issuer_test.go#L174-L240: wrap the seed, static-wins, non-overwrite, and delete-convergence phases ofTestSeedAndReloadDBIssuersin orderedt.Runsubtests that sharereganddao.rest-api/api/pkg/api/model/issuer_test.go#L70-L89: convert the eight assertions ofTestValidateStaticOnlyClaimMappingsinto a table of{name, mappings, wantErr}cases driven byt.Run, using the existing trailing comments as the case names.As per coding guidelines: "Use
testifyassertions and organize tests around the production function or method under test, with one top-levelTest...function and named table-drivent.Runsubtests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/issuer_test.go` around lines 38 - 94, In rest-api/db/pkg/db/model/issuer_test.go lines 38-94, update TestIssuerSQLDAO_CRUD to wrap the Create, GetByID, GetAll, and Delete (soft) phases in ordered t.Run subtests sharing created. In rest-api/api/internal/config/issuer_test.go lines 174-240, update TestSeedAndReloadDBIssuers with ordered t.Run subtests for seed, static-wins, non-overwrite, and delete-convergence, sharing reg and dao. In rest-api/api/pkg/api/model/issuer_test.go lines 70-89, convert TestValidateStaticOnlyClaimMappings assertions into named table-driven t.Run cases using the existing comment names and wantErr expectations.Source: Coding guidelines
rest-api/api/internal/config/issuer_test.go (1)
126-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
rolesAttributecase to the dynamic-mapping tests.
TestIssuerHasDynamicMappingcoversOrgAttributeandOrgNameonly.TestReloadSkipsDynamicDBIssuerat Line 245 describes itself as verifying a "hard security boundary" but exercisesOrgAttributeas well.Neither test covers
{orgName: "acme", rolesAttribute: "roles"}. That combination currently passes the reload filter and reaches the live registry, which is the defect I raised onrest-api/db/pkg/db/model/issuer.goLines 248-257. Add the case so the widened predicate stays enforced.Consider moving
TestIssuerHasDynamicMappingnext to the method it tests, in therest-api/db/pkg/db/modelpackage.💚 Proposed additional cases
func TestIssuerHasDynamicMapping(t *testing.T) { assert.True(t, (cdbm.Issuer{ClaimMappings: []cdbm.ClaimMapping{{OrgAttribute: "org"}}}).HasDynamicMapping()) + assert.True(t, (cdbm.Issuer{ClaimMappings: []cdbm.ClaimMapping{{OrgName: "acme", RolesAttribute: "roles"}}}).HasDynamicMapping(), + "token-driven roles are a dynamic mapping") + assert.True(t, (cdbm.Issuer{ClaimMappings: []cdbm.ClaimMapping{{OrgName: "acme", OrgDisplayAttribute: "org_display"}}}).HasDynamicMapping()) assert.False(t, (cdbm.Issuer{ClaimMappings: []cdbm.ClaimMapping{{OrgName: "acme"}}}).HasDynamicMapping()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/issuer_test.go` around lines 126 - 129, Extend TestIssuerHasDynamicMapping to assert that an Issuer with OrgName "acme" and RolesAttribute "roles" is classified as dynamic, and add the same combination to TestReloadSkipsDynamicDBIssuer to preserve the reload filter’s security boundary. If practical, relocate TestIssuerHasDynamicMapping beside HasDynamicMapping in the model package.rest-api/api/internal/config/issuer.go (3)
165-185: 🧹 Nitpick | 🔵 TrivialConsider surfacing skipped issuer rows beyond a log line.
The loop skips a row for three distinct reasons: a static conflict, a dynamic mapping, and a combined-validation failure. Each skip only produces a warning log. A Provider Admin who created the issuer through the API receives no signal that the issuer never became live, and
GET /issuersreturns the row regardless because it reads the table directly.Two options: expose a per-issuer status field derived from the last reload, or emit a counter metric labelled by skip reason so an alert can fire. The second is cheaper and requires no API change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/issuer.go` around lines 165 - 185, Instrument the issuer reload loop around the three skip branches in the issuer configuration processing flow with a counter metric labeled by issuer and skip reason, incrementing it separately for static conflicts, dynamic mappings, and validation failures. Reuse the existing metrics registration and labeling conventions, and leave the current warning logs and acceptance behavior unchanged.
259-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
computeReservedOrgNamesandTestComputeReservedOrgNames. The repository has no production caller, andbuildJwksConfigdoes not require reserved names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/issuer.go` around lines 259 - 271, Remove the unused computeReservedOrgNames function and its associated TestComputeReservedOrgNames test. Do not add replacement logic or alter buildJwksConfig, since no production caller requires reserved-name computation.
43-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-636)
Reachability: Internal · Exploitability: Difficult
Reachability path
● Entry rest-api/api/pkg/api/handler/issuer_test.go:114 TestIssuerHandler_TenantAdminForbidden: dbSession is never touched on the 403 path (the gate rejects first). │ ▼ ● Sink rest-api/api/internal/config/issuer.goFail closed when
GetOrigincannot parse the origin.This guard controls custom issuer creation. Return
truefor an unparseable origin instead of treating it as non-privileged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/issuer.go` around lines 43 - 55, The HasPrivilegedStaticIssuerOrigins method must fail closed when an issuer’s GetOrigin call returns an error. Change the error branch to return true immediately, while preserving the existing privileged-origin switch and false result when all configured origins parse successfully without matching.rest-api/db/pkg/util/testing.go (1)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument PostgreSQL 13+ for external deployments. Local and test environments already pin PostgreSQL 14.4, and the prerequisite cluster pins PostgreSQL 15. Older external servers require
pgcryptoforgen_random_uuid(), but they cannot supportDROP DATABASE ... WITH (FORCE).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/util/testing.go` around lines 93 - 96, Document PostgreSQL 13+ as a requirement for external deployments, including the pgcrypto prerequisite for older servers using gen_random_uuid(). In rest-api/db/pkg/util/testing.go lines 93-96, document that DROP DATABASE ... WITH (FORCE) requires PostgreSQL 13+; in rest-api/db/pkg/migrations/20260721150000_issuer.go lines 27-53, document the PostgreSQL version and pgcrypto requirements associated with the migration.rest-api/openapi/spec.yaml (1)
14833-14966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
examplesto the new Issuer schemas for documentation consistency.Every other schema in this file (
InfrastructureProvider,Tenant,Site, and so on) includes anexamples:block. The newIssuer,IssuerCreateRequest, andIssuerClaimMappingschemas omit it, which is a documentation-consistency gap for the generated OpenAPI docs and SDK examples.As per path instructions, "Review OpenAPI docs and examples for accuracy, deprecation clarity, client-facing compatibility, spelling, and consistency with
spec.yaml."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/openapi/spec.yaml` around lines 14833 - 14966, Add accurate examples blocks to the Issuer, IssuerCreateRequest, and IssuerClaimMapping schemas, covering the documented fields and API constraints such as custom origin, issuer URLs, static mappings, and service-account behavior. Follow the existing examples formatting and conventions used by nearby schemas in spec.yaml, without changing the schema properties or descriptions.Source: Path instructions
rest-api/api/pkg/api/handler/issuer_test.go (2)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
GetIssuerHandlerandDeleteIssuerHandlererror paths.The suite exercises create, list, and the delete concurrency race. Two behaviors of the new endpoints stay untested: retrieval by ID, including the 404 branch at
issuer.goLine 344, and the invalid-UUID 400 branch at Line 337 and Line 427. ThecreateIssuerfixture already returns the created ID, so both cases are cheap to add.Do you want me to draft those subtests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/issuer_test.go` around lines 99 - 109, Extend the issuer handler tests using the ID returned by testIssuerFixture.createIssuer to cover GetIssuerHandler and DeleteIssuerHandler error paths: assert a missing issuer returns 404, and an invalid UUID returns 400 for both endpoints. Add focused subtests alongside the existing create/list/delete coverage without changing the fixture behavior.
148-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrganize the tests around the handler under test.
This file uses six independent top-level
Test...functions coveringCreateIssuerHandlerandGetAllIssuerHandler. The repository convention is one top-levelTest...function per production function or method, with named table-drivent.Runsubtests. Group the create-rejection cases intoTestCreateIssuerHandler_Handlesubtests such asprivileged-static-origins,dynamic-claim-mappings, andforces-custom-origin.As per coding guidelines: "organize tests around the production function or method under test, with one top-level
Test...function and named table-drivent.Runsubtests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/issuer_test.go` around lines 148 - 200, Consolidate the CreateIssuerHandler rejection tests into a single top-level TestCreateIssuerHandler_Handle function using named t.Run subtests, including privileged-static-origins, dynamic-claim-mappings, and forces-custom-origin. Move the existing setup, requests, and assertions into the corresponding subtests while preserving their behavior; organize GetAllIssuerHandler coverage separately under its own handler-focused top-level test.Source: Coding guidelines
rest-api/api/pkg/api/routes_test.go (1)
169-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert all four issuer routes.
The count at Line 73 declares four issuer routes, but only the create route is asserted. A wrong path or method on the other three would still pass, because the total count stays correct. Add the missing assertions.
💚 Proposed additional assertions
issuerPath := "/org/:orgName/" + cfg.GetAPIName() + "/issuer" assertRouteExists(t, got, http.MethodPut, issuerPath) + assertRouteExists(t, got, http.MethodGet, issuerPath) + assertRouteExists(t, got, http.MethodGet, issuerPath+"/:issuerId") + assertRouteExists(t, got, http.MethodDelete, issuerPath+"/:issuerId")As per coding guidelines: "When adding routes, update the route family count, add
assertRouteExists".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/routes_test.go` around lines 169 - 170, Add assertions in the route test alongside the existing issuer assertion for all four issuer routes, covering each expected HTTP method and path variant defined by the issuer route registration. Keep the declared issuer route count and existing create-route assertion, and use assertRouteExists for every route so incorrect methods or paths are detected.Source: Coding guidelines
rest-api/api/internal/server/server.go (1)
267-282: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTie the issuer background workers to a cancellable context.
issuerCtxiscontext.Background(), so neither the reload ticker inStartIssuerReloadLoopnor theListengoroutine can ever stop.listenOnceopens a dedicated connection outside the pool, so every call toInitAPIServeradds one permanent goroutine and one permanent PostgreSQL connection. In a single long-lived process this is tolerable. In tests, or ifInitAPIServeris ever called more than once, it leaks both.Accept a context in
InitAPIServer, or create a cancellable context here and expose the cancel function to the shutdown path.♻️ Proposed lifecycle wiring
- if !cfg.GetKeycloakEnabled() { - issuerCtx := context.Background() + if !cfg.GetKeycloakEnabled() { + issuerCtx, cancelIssuerSync := context.WithCancel(ctx) + e.Server.RegisterOnShutdown(cancelIssuerSync) if err := cfg.ReloadDBIssuers(issuerCtx, dbSession); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/server/server.go` around lines 267 - 282, Update InitAPIServer’s issuer worker setup to use a cancellable lifecycle context instead of context.Background(). Thread the provided server context through ReloadDBIssuers, StartIssuerReloadLoop, and dbSession.Listen, or create and retain a cancel function that the shutdown path invokes, ensuring repeated initialization and test teardown stop both background workers and their dedicated database connection.rest-api/api/internal/config/config.go (1)
645-651: 📐 Maintainability & Code Quality | 🔵 TrivialDocument the startup-breaking validation change.
The tracked deployment and issuer examples contain no duplicate static
orgNamemappings. Existing deployments can contain external ConfigMaps. Record in the unified NICo changelog that operators must remove duplicates before upgrading becauseValidate()panics on this validation error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/config.go` around lines 645 - 651, Update the unified NICo changelog to document the startup-breaking static orgName uniqueness validation in Validate(): operators must remove duplicate mappings across issuers before upgrading, including duplicates in external ConfigMaps, because validation errors cause a panic during startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/internal/config/config.go`:
- Around line 645-658: Resolve the unreachable seenSAOrgs duplicate check in the
validation logic around mapping.OrgName: if strict global org uniqueness is
intended, remove the seenSAOrgs declaration and its service-account
tracking/check while preserving the seenStaticOrgs validation and error.
Otherwise, relax seenStaticOrgs handling to allow a service-account and
non-service-account mapping for the same normalized organization, while still
rejecting duplicate service-account mappings.
In `@rest-api/api/pkg/api/handler/issuer.go`:
- Around line 248-262: The GetAllIssuerHandler response currently omits required
pagination metadata. Reuse the common pagination helper used by other
multi-resource GetAll handlers after issuerDAO.GetAll and before returning the
JSON response, setting the resulting metadata in the X-Pagination header while
preserving the existing issuer collection and error handling.
- Around line 54-65: The issuer creation endpoint is documented and registered
as PUT but must use POST because duplicate creations return 409. Update the
method in CreateIssuerHandler’s godoc annotation, the corresponding route in
routes.go, its expectation in routes_test.go, and the OpenAPI specification;
leave the endpoint path and handler behavior unchanged.
- Around line 70-102: Extract the repeated issuer authorization checks into a
shared authorizeIssuerAdmin helper near the issuer handlers, accepting the
request context, configuration, user, organization, and logger and returning the
prepared API error or nil. Move the dbUser validation, disconnected-mode check,
static trust ownership check, org membership validation, and Provider Admin role
validation into this helper, then replace the duplicated gate in all four issuer
handlers with a single helper call that returns any error response.
- Around line 149-153: Update the error handling around ValidateCombinedIssuers
in the affected issuer handlers to distinguish database failures from semantic
validation errors. Return a static client-facing message for errors originating
from dao.GetAll, while preserving the existing detailed validation message for
semantic failures; keep server-side logging of the original error.
In `@rest-api/api/pkg/api/model/issuer_test.go`:
- Around line 21-25: Extend APIIssuerCreateRequest_Validate and its tests to
require non-empty IssuerURL and JWKSUrl values to parse as absolute URLs, adding
negative cases for malformed values. Preserve acceptance of HTTP URLs, including
http:// endpoints, and retain the existing required-field behavior.
In `@rest-api/api/pkg/api/model/issuer.go`:
- Around line 36-37: Replace cdbm.ClaimMapping in APIIssuer.ClaimMappings and
APIIssuerCreateRequest.ClaimMappings with a package-local APIClaimMapping
containing explicit camelCase JSON tags, then add conversion logic in the
relevant receiver methods to map between APIClaimMapping and cdbm.ClaimMapping.
Keep database models out of the public REST and OpenAPI contract.
- Around line 44-45: Align issuer model validation and conversion with
repository conventions: update APIIssuer validation to use ozzo built-in
validators for name and issuerUrl while preserving the cross-field claim-mapping
check through validation.Errors; replace the free NewAPIIssuer constructor with
an APIIssuer.FromDBModel receiver; and move DefaultJWKSURL and
ValidateStaticOnlyClaimMappings onto APIIssuerCreateRequest, using the receiver
state instead of two-argument helper signatures.
In `@rest-api/db/pkg/db/model/issuer.go`:
- Around line 259-288: Update Issuer.Signature to include i.Name in the JSON
payload used for hashing, alongside the other fields affecting JwksConfig.
Ensure the field has a stable JSON key so renaming an issuer changes the
signature and triggers registry reconstruction.
- Around line 248-257: Update Issuer.HasDynamicMapping to return true when any
claim mapping sets OrgAttribute, OrgDisplayAttribute, or RolesAttribute,
matching the validation rule and preventing token-controlled role assignment
through static organization mappings.
In `@rest-api/deploy/kustomize/base/api/configmap.yaml`:
- Around line 74-77: Remove the plaintext HTTP issuer and JWKS endpoints from
the shared base ConfigMap’s issuers configuration. Define the development
Keycloak HTTP settings only in the development overlay, and configure shared or
production overlays to use HTTPS with the trusted Keycloak CA certificates.
In `@rest-api/openapi/spec.yaml`:
- Around line 14741-14766: Update the operationId for the “List external JWT
issuers” GET operation from the plural form to the singular-noun convention,
using get-all-issuer. Leave the route, summary, response schema, and other
response definitions unchanged.
- Around line 14694-14740: Change the /v2/org/{org}/nico/issuer operation from
PUT to POST while keeping operationId create-issuer and its existing request and
response definitions unchanged. This endpoint is create-only, so align its
method with the repository’s POST creation convention rather than the PUT upsert
pattern.
- Around line 14833-14886: Update the Issuer schema properties id, origin,
created, and updated to declare readOnly: true, matching their server-computed
behavior and the conventions used by equivalent resource fields elsewhere in the
specification. Leave the remaining Issuer properties unchanged.
- Around line 14931-14966: Add a required declaration for orgName within the
IssuerClaimMapping schema, alongside its existing properties. Keep the current
orgName type and description unchanged, and make the requirement unconditional
for this API schema.
---
Nitpick comments:
In `@rest-api/api/internal/config/config.go`:
- Around line 645-651: Update the unified NICo changelog to document the
startup-breaking static orgName uniqueness validation in Validate(): operators
must remove duplicate mappings across issuers before upgrading, including
duplicates in external ConfigMaps, because validation errors cause a panic
during startup.
In `@rest-api/api/internal/config/issuer_test.go`:
- Around line 126-129: Extend TestIssuerHasDynamicMapping to assert that an
Issuer with OrgName "acme" and RolesAttribute "roles" is classified as dynamic,
and add the same combination to TestReloadSkipsDynamicDBIssuer to preserve the
reload filter’s security boundary. If practical, relocate
TestIssuerHasDynamicMapping beside HasDynamicMapping in the model package.
In `@rest-api/api/internal/config/issuer.go`:
- Around line 165-185: Instrument the issuer reload loop around the three skip
branches in the issuer configuration processing flow with a counter metric
labeled by issuer and skip reason, incrementing it separately for static
conflicts, dynamic mappings, and validation failures. Reuse the existing metrics
registration and labeling conventions, and leave the current warning logs and
acceptance behavior unchanged.
- Around line 259-271: Remove the unused computeReservedOrgNames function and
its associated TestComputeReservedOrgNames test. Do not add replacement logic or
alter buildJwksConfig, since no production caller requires reserved-name
computation.
- Around line 43-55: The HasPrivilegedStaticIssuerOrigins method must fail
closed when an issuer’s GetOrigin call returns an error. Change the error branch
to return true immediately, while preserving the existing privileged-origin
switch and false result when all configured origins parse successfully without
matching.
In `@rest-api/api/internal/server/server.go`:
- Around line 267-282: Update InitAPIServer’s issuer worker setup to use a
cancellable lifecycle context instead of context.Background(). Thread the
provided server context through ReloadDBIssuers, StartIssuerReloadLoop, and
dbSession.Listen, or create and retain a cancel function that the shutdown path
invokes, ensuring repeated initialization and test teardown stop both background
workers and their dedicated database connection.
In `@rest-api/api/pkg/api/handler/issuer_test.go`:
- Around line 99-109: Extend the issuer handler tests using the ID returned by
testIssuerFixture.createIssuer to cover GetIssuerHandler and DeleteIssuerHandler
error paths: assert a missing issuer returns 404, and an invalid UUID returns
400 for both endpoints. Add focused subtests alongside the existing
create/list/delete coverage without changing the fixture behavior.
- Around line 148-200: Consolidate the CreateIssuerHandler rejection tests into
a single top-level TestCreateIssuerHandler_Handle function using named t.Run
subtests, including privileged-static-origins, dynamic-claim-mappings, and
forces-custom-origin. Move the existing setup, requests, and assertions into the
corresponding subtests while preserving their behavior; organize
GetAllIssuerHandler coverage separately under its own handler-focused top-level
test.
In `@rest-api/api/pkg/api/routes_test.go`:
- Around line 169-170: Add assertions in the route test alongside the existing
issuer assertion for all four issuer routes, covering each expected HTTP method
and path variant defined by the issuer route registration. Keep the declared
issuer route count and existing create-route assertion, and use
assertRouteExists for every route so incorrect methods or paths are detected.
In `@rest-api/db/pkg/db/model/issuer_test.go`:
- Around line 96-121: Add coverage alongside
TestIssuerSQLDAO_DuplicateURLRejected for both unique constraints: verify
creating a second issuer with the same Name but a different IssuerURL returns a
unique-constraint error, and verify an issuer deleted through dao.Delete can be
recreated with the same name and URL without error. Use separate subtests or
isolated setup so the cases do not share database state.
- Around line 38-94: In rest-api/db/pkg/db/model/issuer_test.go lines 38-94,
update TestIssuerSQLDAO_CRUD to wrap the Create, GetByID, GetAll, and Delete
(soft) phases in ordered t.Run subtests sharing created. In
rest-api/api/internal/config/issuer_test.go lines 174-240, update
TestSeedAndReloadDBIssuers with ordered t.Run subtests for seed, static-wins,
non-overwrite, and delete-convergence, sharing reg and dao. In
rest-api/api/pkg/api/model/issuer_test.go lines 70-89, convert
TestValidateStaticOnlyClaimMappings assertions into named table-driven t.Run
cases using the existing comment names and wantErr expectations.
In `@rest-api/db/pkg/util/testing.go`:
- Around line 93-96: Document PostgreSQL 13+ as a requirement for external
deployments, including the pgcrypto prerequisite for older servers using
gen_random_uuid(). In rest-api/db/pkg/util/testing.go lines 93-96, document that
DROP DATABASE ... WITH (FORCE) requires PostgreSQL 13+; in
rest-api/db/pkg/migrations/20260721150000_issuer.go lines 27-53, document the
PostgreSQL version and pgcrypto requirements associated with the migration.
In `@rest-api/openapi/spec.yaml`:
- Around line 14833-14966: Add accurate examples blocks to the Issuer,
IssuerCreateRequest, and IssuerClaimMapping schemas, covering the documented
fields and API constraints such as custom origin, issuer URLs, static mappings,
and service-account behavior. Follow the existing examples formatting and
conventions used by nearby schemas in spec.yaml, without changing the schema
properties or descriptions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a0700bf-c508-4fac-9719-31bde78e4c84
⛔ Files ignored due to path filters (5)
rest-api/sdk/standard/api_issuer.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_issuer.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_claim_mapping.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_create_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (20)
rest-api/api/internal/config/config.gorest-api/api/internal/config/issuer.gorest-api/api/internal/config/issuer_test.gorest-api/api/internal/server/server.gorest-api/api/pkg/api/handler/issuer.gorest-api/api/pkg/api/handler/issuer_test.gorest-api/api/pkg/api/model/issuer.gorest-api/api/pkg/api/model/issuer_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/db/pkg/db/listener.gorest-api/db/pkg/db/listener_test.gorest-api/db/pkg/db/model/issuer.gorest-api/db/pkg/db/model/issuer_test.gorest-api/db/pkg/migrations/20260721150000_issuer.gorest-api/db/pkg/migrations/20260721160000_issuer_notify.gorest-api/db/pkg/util/testing.gorest-api/deploy/kustomize/base/api/configmap.yamlrest-api/docs/index.htmlrest-api/openapi/spec.yaml
| // Static org mapping - globally unique across all issuers; no escape hatch. | ||
| if mapping.OrgName != "" { | ||
| normalizedOrg := strings.ToLower(mapping.OrgName) | ||
| if seenStaticOrgs[normalizedOrg] && !issuer.GetAllowDuplicateStaticOrgNames() { | ||
| return fmt.Errorf("issuer %s: duplicate static org: %s", issuer.Name, mapping.OrgName) | ||
| if seenStaticOrgs[normalizedOrg] { | ||
| return fmt.Errorf("issuer %s: duplicate org name: %s (org names must be unique across all issuers)", issuer.Name, mapping.OrgName) | ||
| } | ||
| seenStaticOrgs[normalizedOrg] = true | ||
| // At most one service-account mapping per org globally. | ||
| if mapping.IsServiceAccount { | ||
| if seenSAOrgs[normalizedOrg] { | ||
| return fmt.Errorf("issuer %s: org %s already has a service account mapping", issuer.Name, mapping.OrgName) | ||
| } | ||
| seenSAOrgs[normalizedOrg] = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
seenSAOrgs is unreachable as a duplicate detector.
The seenStaticOrgs check returns an error on the second occurrence of an org name. Control never reaches the seenSAOrgs lookup with an org that was already recorded. Therefore seenSAOrgs[normalizedOrg] is always false, and the "already has a service account mapping" error is dead code.
If the intent is "one service-account mapping per org, while still allowing a second non-service-account mapping for that org", then the org-uniqueness rule must be relaxed for that case. If the intent is strict global org uniqueness, remove the redundant set.
♻️ Proposed simplification if strict org uniqueness is intended
// Static org mapping - globally unique across all issuers; no escape hatch.
if mapping.OrgName != "" {
normalizedOrg := strings.ToLower(mapping.OrgName)
if seenStaticOrgs[normalizedOrg] {
return fmt.Errorf("issuer %s: duplicate org name: %s (org names must be unique across all issuers)", issuer.Name, mapping.OrgName)
}
seenStaticOrgs[normalizedOrg] = true
- // At most one service-account mapping per org globally.
- if mapping.IsServiceAccount {
- if seenSAOrgs[normalizedOrg] {
- return fmt.Errorf("issuer %s: org %s already has a service account mapping", issuer.Name, mapping.OrgName)
- }
- seenSAOrgs[normalizedOrg] = true
- }
}Also remove the seenSAOrgs declaration at Line 554.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/internal/config/config.go` around lines 645 - 658, Resolve the
unreachable seenSAOrgs duplicate check in the validation logic around
mapping.OrgName: if strict global org uniqueness is intended, remove the
seenSAOrgs declaration and its service-account tracking/check while preserving
the seenStaticOrgs validation and error. Otherwise, relax seenStaticOrgs
handling to allow a service-account and non-service-account mapping for the same
normalized organization, while still rejecting duplicate service-account
mappings.
| // Handle godoc | ||
| // @Summary Register an external JWT issuer | ||
| // @Description Register a runtime-managed external JWT issuer (Provider Admin only) | ||
| // @Tags issuer | ||
| // @Accept json | ||
| // @Produce json | ||
| // @Security ApiKeyAuth | ||
| // @Param org path string true "Name of NGC organization" | ||
| // @Param issuer body model.APIIssuerCreateRequest true "Issuer to create" | ||
| // @Success 201 {object} model.APIIssuer | ||
| // @Router /v2/org/{org}/nico/issuer [put] | ||
| func (cih CreateIssuerHandler) Handle(c echo.Context) error { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use POST for issuer creation, not PUT.
This endpoint is a pure create. A second request with the same issuer URL or name returns 409 from Line 159, so the operation is not create-or-update and is not idempotent. The repository convention reserves PUT for create-or-update operations.
Change the method to POST in the godoc annotation, in rest-api/api/pkg/api/routes.go (Line 823), in rest-api/api/pkg/api/routes_test.go (Line 170), and in the OpenAPI specification.
As per coding guidelines: "Use POST for creation, PATCH for updates, PUT only for create-or-update operations".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/issuer.go` around lines 54 - 65, The issuer
creation endpoint is documented and registered as PUT but must use POST because
duplicate creations return 409. Update the method in CreateIssuerHandler’s godoc
annotation, the corresponding route in routes.go, its expectation in
routes_test.go, and the OpenAPI specification; leave the endpoint path and
handler behavior unchanged.
Source: Coding guidelines
| if dbUser == nil { | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve current user", nil) | ||
| } | ||
|
|
||
| // Issuer creation is only supported in disconnected mode | ||
| if !cih.cfg.GetEnvDisconnected() { | ||
| logger.Warn().Msg("Issuer creation is only supported in disconnected mode") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Issuer management via the API is only supported in disconnected mode", nil) | ||
| } | ||
|
|
||
| // Validate that the static configuration does not own issuer trust | ||
| if cih.cfg.GetKeycloakEnabled() || cih.cfg.HasDynamicConfigMapIssuers() { | ||
| logger.Warn().Msg("Issuer API is unavailable, static configuration owns issuer trust") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, issuerAPIUnavailableMessage, nil) | ||
| } | ||
|
|
||
| // Validate org | ||
| ok, err := auth.ValidateOrgMembership(dbUser, org) | ||
| if !ok { | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error validating org membership for User in request") | ||
| } else { | ||
| logger.Warn().Msg("could not validate org membership for user, access denied") | ||
| } | ||
| return cutil.NewAPIErrorResponse(c, http.StatusForbidden, fmt.Sprintf("Failed to validate membership for org: %s", org), nil) | ||
| } | ||
|
|
||
| // Validate role, Issuer management is a provider-level trust operation so only Provider Admins are allowed | ||
| ok = auth.ValidateUserRoles(dbUser, org, nil, auth.ProviderAdminRole) | ||
| if !ok { | ||
| logger.Warn().Msg("user does not have Provider Admin role, access denied") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusForbidden, "User does not have Provider Admin role with org", nil) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the repeated issuer access gate.
The same five checks appear in all four handlers: the dbUser nil check, the disconnected-mode check, the static-trust-ownership check, org membership, and the Provider Admin role check. That is roughly 130 duplicated lines. A future change to the gate must be applied in four places, and one missed site becomes an authorization defect.
Extract one helper, for example authorizeIssuerAdmin(c echo.Context, cfg *config.Config, dbUser *cdbm.User, org string, logger zerolog.Logger) error, that returns the prepared *cutil.APIError or nil, and call it from each handler.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/issuer.go` around lines 70 - 102, Extract the
repeated issuer authorization checks into a shared authorizeIssuerAdmin helper
near the issuer handlers, accepting the request context, configuration, user,
organization, and logger and returning the prepared API error or nil. Move the
dbUser validation, disconnected-mode check, static trust ownership check, org
membership validation, and Provider Admin role validation into this helper, then
replace the duplicated gate in all four issuer handlers with a single helper
call that returns any error response.
| derr = cih.cfg.ValidateCombinedIssuers(ctx, cih.dbSession, tx, &candidate, nil) | ||
| if derr != nil { | ||
| logger.Warn().Err(derr).Msg("Issuer is not valid against the combined issuer set") | ||
| return nil, cutil.NewAPIError(http.StatusBadRequest, fmt.Sprintf("Invalid issuer configuration: %s", derr), nil) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ValidateCombinedIssuers to see whether DB errors are returned to callers.
fd -g 'issuer.go' -p rest-api/api/internal/config --exec ast-grep outline {} --items all
rg -nP -A40 'func \(c \*Config\) ValidateCombinedIssuers' rest-api/api/internal/configRepository: NVIDIA/infra-controller
Length of output: 3234
Information Disclosure (CWE-209): Generation of Error Message Containing Sensitive Information
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
rest-api/api/internal/server/server.go:108
InitAPIServer: Add timeouts to prevent SLOWLORIS attacks
│
▼
● Hop
rest-api/api/pkg/api/routes.go:20
NewAPIRoutes: Metadata endpoint
│
▼
● Sink
rest-api/api/pkg/api/handler/issuer.go
Do not expose database errors in issuer API responses.
ValidateCombinedIssuers returns dao.GetAll errors unchanged, and both handlers include those errors in the client message. Return a static message for database failures while preserving semantic validation details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/issuer.go` around lines 149 - 153, Update the
error handling around ValidateCombinedIssuers in the affected issuer handlers to
distinguish database failures from semantic validation errors. Return a static
client-facing message for errors originating from dao.GetAll, while preserving
the existing detailed validation message for semantic failures; keep server-side
logging of the original error.
| issuerDAO := cdbm.NewIssuerDAO(gaih.dbSession) | ||
| issuers, err := issuerDAO.GetAll(ctx, nil, cdbm.IssuerFilterInput{}) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error retrieving Issuers from DB") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Issuers, DB error", nil) | ||
| } | ||
|
|
||
| // Create response | ||
| apiIssuers := []*model.APIIssuer{} | ||
| for i := range issuers { | ||
| apiIssuers = append(apiIssuers, model.NewAPIIssuer(&issuers[i])) | ||
| } | ||
|
|
||
| logger.Info().Msg("finishing API handler") | ||
| return c.JSON(http.StatusOK, apiIssuers) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add pagination metadata to the issuer list response.
GetAllIssuerHandler returns the full issuer collection with no X-Pagination header. Multi-resource GET responses in this API must carry pagination metadata. Reuse the common pagination helper used by the other GetAll handlers so clients see a consistent contract.
As per coding guidelines: "use ... pagination metadata in X-Pagination for multi-resource GET responses".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/issuer.go` around lines 248 - 262, The
GetAllIssuerHandler response currently omits required pagination metadata. Reuse
the common pagination helper used by other multi-resource GetAll handlers after
issuerDAO.GetAll and before returning the JSON response, setting the resulting
metadata in the X-Pagination header while preserving the existing issuer
collection and error handling.
Source: Coding guidelines
| issuers: | ||
| - name: nico-dev-keycloak | ||
| issuer: http://localhost:8082/realms/nico-dev | ||
| jwks: http://keycloak.nico-rest:8082/realms/nico-dev/protocol/openid-connect/certs |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace which overlays consume this base ConfigMap and inspect network controls.
rg -n -C 4 'configmap\.yaml|resources:|bases:|patches:|kind:\s*NetworkPolicy' rest-api/deploy
# Inspect issuer loading and JWKS retrieval behavior without executing repository code.
rg -n -C 8 'jwks|JWKS|issuer|Issuer' \
rest-api/api/internal/config/issuer.go \
rest-api/api/internal/server/server.goRepository: NVIDIA/infra-controller
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- issuer configuration ---'
sed -n '55,90p' rest-api/deploy/kustomize/base/api/configmap.yaml
printf '%s\n' '--- API and Keycloak kustomizations ---'
cat rest-api/deploy/kustomize/base/api/kustomization.yaml
cat rest-api/deploy/kustomize/overlays/api/kustomization.yaml
cat rest-api/deploy/kustomize/base/keycloak/kustomization.yaml
printf '%s\n' '--- deployment entry points ---'
rg -n -C 3 'overlays/api|base/api|kustomize build|kubectl apply' rest-api/deploy --glob '*.yaml' --glob '*.yml' --glob '*.sh' --glob 'Makefile' --glob '*.md' | head -160
printf '%s\n' '--- network policy manifests ---'
rg -l 'kind:[[:space:]]*NetworkPolicy' rest-api/deploy || true
printf '%s\n' '--- JWKS retrieval implementation ---'
rg -n -C 5 'JWKS|jwks|RemoteKeySet|NewRemote|go-jose|oidc|key.*set|http[s]?://' rest-api/api --glob '*.go' | head -240Repository: NVIDIA/infra-controller
Length of output: 31603
Security Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Keep the plaintext JWKS configuration out of the shared base.
The API overlay consumes this base directly. Move the HTTP issuer to a development-only overlay. Configure HTTPS with trusted Keycloak CA certificates for shared or production deployments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/deploy/kustomize/base/api/configmap.yaml` around lines 74 - 77,
Remove the plaintext HTTP issuer and JWKS endpoints from the shared base
ConfigMap’s issuers configuration. Define the development Keycloak HTTP settings
only in the development overlay, and configure shared or production overlays to
use HTTPS with the trusted Keycloak CA certificates.
Source: Path instructions
| '/v2/org/{org}/nico/issuer': | ||
| parameters: | ||
| - schema: | ||
| type: string | ||
| name: org | ||
| in: path | ||
| required: true | ||
| description: Name of the Org | ||
| put: | ||
| summary: Register an external JWT issuer | ||
| operationId: create-issuer | ||
| description: |- | ||
| Register a runtime-managed external JWT issuer. Provider Admin only. | ||
|
|
||
| Always sets `origin: custom`. Only static org-name claim mappings are accepted; | ||
| attribute-driven mappings (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) | ||
| are rejected with `400`. | ||
|
|
||
| Returns `400` while Keycloak is enabled or any static ConfigMap issuer carries a | ||
| dynamic claim mapping. | ||
| tags: | ||
| - Issuer | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/IssuerCreateRequest' | ||
| responses: | ||
| '201': | ||
| description: Issuer registered successfully. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Issuer' | ||
| '400': | ||
| $ref: '#/components/responses/ValidationError' | ||
| '403': | ||
| $ref: '#/components/responses/ForbiddenError' | ||
| '404': | ||
| description: Infrastructure Provider not found for the org. | ||
| $ref: '#/components/responses/NotFoundError' | ||
| '409': | ||
| description: An issuer with this URL or name already exists, or the URL/name is reserved by a static issuer. | ||
| $ref: '#/components/responses/GenericHttpError' | ||
| '500': | ||
| $ref: '#/components/responses/GenericHttpError' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use POST, not PUT, for issuer registration.
The create-issuer operation only returns 201 (created) or 409 (conflict). There is no 200 response for an existing-resource update path. This makes the operation create-only, not create-or-update.
The path instructions for rest-api/api/pkg/api/handler/**/*.go state: "Use POST for creation, PATCH for updates, PUT only for create-or-update operations, resource IDs on PATCH/GET/DELETE routes where applicable." Compare this endpoint to create-or-update-host-firmware-config, a genuine PUT upsert that returns both 200 (updated) and 201 (created). create-issuer has no equivalent 200 update branch, so it does not fit the PUT-upsert pattern and should use POST instead, consistent with the PR's stated design ("Issuer updates are handled by deleting and recreating entries; no update API is included").
🔧 Proposed fix
- put:
+ post:
summary: Register an external JWT issuer
operationId: create-issuerBased on path instructions, "Use the repository's established endpoint family, model, handler, transaction, and generation patterns instead of inventing parallel implementations," and the handler coding guideline: "Use POST for creation, PATCH for updates, PUT only for create-or-update operations."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| '/v2/org/{org}/nico/issuer': | |
| parameters: | |
| - schema: | |
| type: string | |
| name: org | |
| in: path | |
| required: true | |
| description: Name of the Org | |
| put: | |
| summary: Register an external JWT issuer | |
| operationId: create-issuer | |
| description: |- | |
| Register a runtime-managed external JWT issuer. Provider Admin only. | |
| Always sets `origin: custom`. Only static org-name claim mappings are accepted; | |
| attribute-driven mappings (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) | |
| are rejected with `400`. | |
| Returns `400` while Keycloak is enabled or any static ConfigMap issuer carries a | |
| dynamic claim mapping. | |
| tags: | |
| - Issuer | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/IssuerCreateRequest' | |
| responses: | |
| '201': | |
| description: Issuer registered successfully. | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/Issuer' | |
| '400': | |
| $ref: '#/components/responses/ValidationError' | |
| '403': | |
| $ref: '#/components/responses/ForbiddenError' | |
| '404': | |
| description: Infrastructure Provider not found for the org. | |
| $ref: '#/components/responses/NotFoundError' | |
| '409': | |
| description: An issuer with this URL or name already exists, or the URL/name is reserved by a static issuer. | |
| $ref: '#/components/responses/GenericHttpError' | |
| '500': | |
| $ref: '#/components/responses/GenericHttpError' | |
| '/v2/org/{org}/nico/issuer': | |
| parameters: | |
| - schema: | |
| type: string | |
| name: org | |
| in: path | |
| required: true | |
| description: Name of the Org | |
| post: | |
| summary: Register an external JWT issuer | |
| operationId: create-issuer | |
| description: |- | |
| Register a runtime-managed external JWT issuer. Provider Admin only. | |
| Always sets `origin: custom`. Only static org-name claim mappings are accepted; | |
| attribute-driven mappings (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) | |
| are rejected with `400`. | |
| Returns `400` while Keycloak is enabled or any static ConfigMap issuer carries a | |
| dynamic claim mapping. | |
| tags: | |
| - Issuer | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '`#/components/schemas/IssuerCreateRequest`' | |
| responses: | |
| '201': | |
| description: Issuer registered successfully. | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '`#/components/schemas/Issuer`' | |
| '400': | |
| $ref: '`#/components/responses/ValidationError`' | |
| '403': | |
| $ref: '`#/components/responses/ForbiddenError`' | |
| '404': | |
| description: Infrastructure Provider not found for the org. | |
| $ref: '`#/components/responses/NotFoundError`' | |
| '409': | |
| description: An issuer with this URL or name already exists, or the URL/name is reserved by a static issuer. | |
| $ref: '`#/components/responses/GenericHttpError`' | |
| '500': | |
| $ref: '`#/components/responses/GenericHttpError`' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 14694 - 14740, Change the
/v2/org/{org}/nico/issuer operation from PUT to POST while keeping operationId
create-issuer and its existing request and response definitions unchanged. This
endpoint is create-only, so align its method with the repository’s POST creation
convention rather than the PUT upsert pattern.
Sources: Coding guidelines, Path instructions
| get: | ||
| summary: List external JWT issuers | ||
| operationId: get-all-issuers | ||
| description: |- | ||
| List all runtime-managed external JWT issuers for the org's Infrastructure Provider. | ||
| Provider Admin only. | ||
| tags: | ||
| - Issuer | ||
| responses: | ||
| '200': | ||
| description: OK | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/Issuer' | ||
| '400': | ||
| $ref: '#/components/responses/ValidationError' | ||
| '403': | ||
| $ref: '#/components/responses/ForbiddenError' | ||
| '404': | ||
| description: Infrastructure Provider not found for the org. | ||
| $ref: '#/components/responses/NotFoundError' | ||
| '500': | ||
| $ref: '#/components/responses/GenericHttpError' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix operationId naming to match the singular-noun convention.
operationId: get-all-issuers uses a plural noun. Every other list operation in this file uses a singular noun, for example get-all-instance, get-all-sku, get-all-machine, get-all-expected-switch, and get-all-vpc-peering. This inconsistency affects generated SDK method names.
🔧 Proposed fix
- operationId: get-all-issuers
+ operationId: get-all-issuerAs per path instructions, "Update the OpenAPI specification whenever a published REST route or schema changes; keep operation IDs, summaries, handler constructors, godoc, and SDK-facing names aligned."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get: | |
| summary: List external JWT issuers | |
| operationId: get-all-issuers | |
| description: |- | |
| List all runtime-managed external JWT issuers for the org's Infrastructure Provider. | |
| Provider Admin only. | |
| tags: | |
| - Issuer | |
| responses: | |
| '200': | |
| description: OK | |
| content: | |
| application/json: | |
| schema: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/Issuer' | |
| '400': | |
| $ref: '#/components/responses/ValidationError' | |
| '403': | |
| $ref: '#/components/responses/ForbiddenError' | |
| '404': | |
| description: Infrastructure Provider not found for the org. | |
| $ref: '#/components/responses/NotFoundError' | |
| '500': | |
| $ref: '#/components/responses/GenericHttpError' | |
| get: | |
| summary: List external JWT issuers | |
| operationId: get-all-issuer | |
| description: |- | |
| List all runtime-managed external JWT issuers for the org's Infrastructure Provider. | |
| Provider Admin only. | |
| tags: | |
| - Issuer | |
| responses: | |
| '200': | |
| description: OK | |
| content: | |
| application/json: | |
| schema: | |
| type: array | |
| items: | |
| $ref: '`#/components/schemas/Issuer`' | |
| '400': | |
| $ref: '`#/components/responses/ValidationError`' | |
| '403': | |
| $ref: '`#/components/responses/ForbiddenError`' | |
| '404': | |
| description: Infrastructure Provider not found for the org. | |
| $ref: '`#/components/responses/NotFoundError`' | |
| '500': | |
| $ref: '`#/components/responses/GenericHttpError`' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 14741 - 14766, Update the
operationId for the “List external JWT issuers” GET operation from the plural
form to the singular-noun convention, using get-all-issuer. Leave the route,
summary, response schema, and other response definitions unchanged.
Source: Path instructions
| Issuer: | ||
| type: object | ||
| title: Issuer | ||
| description: A runtime-managed external JWT issuer registered via the Provider Admin API. | ||
| properties: | ||
| id: | ||
| type: string | ||
| format: uuid | ||
| description: Unique identifier of the issuer. | ||
| name: | ||
| type: string | ||
| description: Unique issuer name within the provider. | ||
| origin: | ||
| type: string | ||
| description: Token processor. Always `custom` for API-created issuers. | ||
| example: custom | ||
| issuerUrl: | ||
| type: string | ||
| description: Expected JWT `iss` claim / OIDC issuer URL. Immutable after creation. | ||
| example: 'https://idp.example.com' | ||
| jwksUrl: | ||
| type: string | ||
| description: URL from which signing keys are fetched. Defaults to `{issuerUrl}/.well-known/jwks.json`. | ||
| example: 'https://idp.example.com/.well-known/jwks.json' | ||
| jwksTimeout: | ||
| type: string | ||
| description: JWKS fetch timeout (Go duration string, e.g. `"5s"`). | ||
| example: '5s' | ||
| serviceAccount: | ||
| type: boolean | ||
| description: Enables client-credentials flow (disconnected mode only). | ||
| audiences: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: Issuer-level allowed audience set; the token must carry at least one. | ||
| scopes: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: Issuer-level required scope set; the token must carry all. | ||
| claimMappings: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/IssuerClaimMapping' | ||
| description: Org/role mapping entries. Only static mappings (`orgName`) are permitted via the API. | ||
| created: | ||
| type: string | ||
| format: date-time | ||
| description: Creation timestamp. | ||
| updated: | ||
| type: string | ||
| format: date-time | ||
| description: Last-update timestamp. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Mark server-computed Issuer fields as readOnly.
id, origin, created, and updated are all set by the server and are not accepted from the client (they are absent from IssuerCreateRequest). Nearly every other resource schema in this file marks its equivalent fields readOnly: true (for example InfrastructureProvider.id, Tenant.created, Site.id). Missing readOnly here understates the contract for generated SDK clients and API documentation, which may otherwise allow these fields in write requests.
🔧 Proposed fix
id:
type: string
format: uuid
description: Unique identifier of the issuer.
+ readOnly: true
name:
type: string
description: Unique issuer name within the provider.
origin:
type: string
description: Token processor. Always `custom` for API-created issuers.
example: custom
+ readOnly: trueand similarly for created/updated:
created:
type: string
format: date-time
description: Creation timestamp.
+ readOnly: true
updated:
type: string
format: date-time
description: Last-update timestamp.
+ readOnly: trueAs per path instructions, "Review the OpenAPI specification for request/response compatibility, schema correctness, required/nullable semantics, examples, operation naming, and consistency with implemented handlers."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Issuer: | |
| type: object | |
| title: Issuer | |
| description: A runtime-managed external JWT issuer registered via the Provider Admin API. | |
| properties: | |
| id: | |
| type: string | |
| format: uuid | |
| description: Unique identifier of the issuer. | |
| name: | |
| type: string | |
| description: Unique issuer name within the provider. | |
| origin: | |
| type: string | |
| description: Token processor. Always `custom` for API-created issuers. | |
| example: custom | |
| issuerUrl: | |
| type: string | |
| description: Expected JWT `iss` claim / OIDC issuer URL. Immutable after creation. | |
| example: 'https://idp.example.com' | |
| jwksUrl: | |
| type: string | |
| description: URL from which signing keys are fetched. Defaults to `{issuerUrl}/.well-known/jwks.json`. | |
| example: 'https://idp.example.com/.well-known/jwks.json' | |
| jwksTimeout: | |
| type: string | |
| description: JWKS fetch timeout (Go duration string, e.g. `"5s"`). | |
| example: '5s' | |
| serviceAccount: | |
| type: boolean | |
| description: Enables client-credentials flow (disconnected mode only). | |
| audiences: | |
| type: array | |
| items: | |
| type: string | |
| description: Issuer-level allowed audience set; the token must carry at least one. | |
| scopes: | |
| type: array | |
| items: | |
| type: string | |
| description: Issuer-level required scope set; the token must carry all. | |
| claimMappings: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/IssuerClaimMapping' | |
| description: Org/role mapping entries. Only static mappings (`orgName`) are permitted via the API. | |
| created: | |
| type: string | |
| format: date-time | |
| description: Creation timestamp. | |
| updated: | |
| type: string | |
| format: date-time | |
| description: Last-update timestamp. | |
| Issuer: | |
| type: object | |
| title: Issuer | |
| description: A runtime-managed external JWT issuer registered via the Provider Admin API. | |
| properties: | |
| id: | |
| type: string | |
| format: uuid | |
| description: Unique identifier of the issuer. | |
| readOnly: true | |
| name: | |
| type: string | |
| description: Unique issuer name within the provider. | |
| origin: | |
| type: string | |
| description: Token processor. Always `custom` for API-created issuers. | |
| example: custom | |
| readOnly: true | |
| issuerUrl: | |
| type: string | |
| description: Expected JWT `iss` claim / OIDC issuer URL. Immutable after creation. | |
| example: 'https://idp.example.com' | |
| jwksUrl: | |
| type: string | |
| description: URL from which signing keys are fetched. Defaults to `{issuerUrl}/.well-known/jwks.json`. | |
| example: 'https://idp.example.com/.well-known/jwks.json' | |
| jwksTimeout: | |
| type: string | |
| description: JWKS fetch timeout (Go duration string, e.g. `"5s"`). | |
| example: '5s' | |
| serviceAccount: | |
| type: boolean | |
| description: Enables client-credentials flow (disconnected mode only). | |
| audiences: | |
| type: array | |
| items: | |
| type: string | |
| description: Issuer-level allowed audience set; the token must carry at least one. | |
| scopes: | |
| type: array | |
| items: | |
| type: string | |
| description: Issuer-level required scope set; the token must carry all. | |
| claimMappings: | |
| type: array | |
| items: | |
| $ref: '`#/components/schemas/IssuerClaimMapping`' | |
| description: Org/role mapping entries. Only static mappings (`orgName`) are permitted via the API. | |
| created: | |
| type: string | |
| format: date-time | |
| description: Creation timestamp. | |
| readOnly: true | |
| updated: | |
| type: string | |
| format: date-time | |
| description: Last-update timestamp. | |
| readOnly: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 14833 - 14886, Update the Issuer
schema properties id, origin, created, and updated to declare readOnly: true,
matching their server-computed behavior and the conventions used by equivalent
resource fields elsewhere in the specification. Leave the remaining Issuer
properties unchanged.
Source: Path instructions
| IssuerClaimMapping: | ||
| type: object | ||
| title: IssuerClaimMapping | ||
| description: |- | ||
| One entry in an issuer's claim mapping array. | ||
|
|
||
| Via the API, only `orgName` (required), `roles` (optional static list), and | ||
| `isServiceAccount` (disconnected mode only) may be set. Attribute-driven fields | ||
| (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) are reserved for | ||
| ConfigMap-defined issuers and will be rejected with `400` if supplied. | ||
|
|
||
| Org names are globally unique across all issuers (both ConfigMap and API-created). | ||
| At most one `isServiceAccount: true` mapping may exist per org across all issuers. | ||
| properties: | ||
| orgName: | ||
| type: string | ||
| description: Fixed org name for this mapping (required for API-created issuers). Globally unique across all issuers. | ||
| orgDisplayName: | ||
| type: string | ||
| description: Display name for the org. | ||
| roles: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: Static role list. Omit when `isServiceAccount` is true (auto-assigns PROVIDER_ADMIN + TENANT_ADMIN). | ||
| audiences: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: Optional per-mapping audience filter. | ||
| isServiceAccount: | ||
| type: boolean | ||
| description: |- | ||
| When true, auto-assigns PROVIDER_ADMIN and TENANT_ADMIN roles (disconnected mode only). | ||
| Cannot be combined with `roles`, `rolesAttribute`, or `orgAttribute`. | ||
| At most one service-account mapping is permitted per org across all issuers. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Declare orgName as required on IssuerClaimMapping.
The description states orgName is "required for API-created issuers," but the schema has no required list enforcing this. Since this schema serves only the Issuer API (which rejects attribute-driven mappings entirely), the constraint applies unconditionally here and should be captured in the schema for accurate client-side validation and codegen.
🔧 Proposed fix
IssuerClaimMapping:
type: object
title: IssuerClaimMapping
description: |-
One entry in an issuer's claim mapping array.
...
+ required:
+ - orgName
properties:
orgName:As per path instructions, "Review the OpenAPI specification for request/response compatibility, schema correctness, required/nullable semantics, examples, operation naming, and consistency with implemented handlers."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IssuerClaimMapping: | |
| type: object | |
| title: IssuerClaimMapping | |
| description: |- | |
| One entry in an issuer's claim mapping array. | |
| Via the API, only `orgName` (required), `roles` (optional static list), and | |
| `isServiceAccount` (disconnected mode only) may be set. Attribute-driven fields | |
| (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) are reserved for | |
| ConfigMap-defined issuers and will be rejected with `400` if supplied. | |
| Org names are globally unique across all issuers (both ConfigMap and API-created). | |
| At most one `isServiceAccount: true` mapping may exist per org across all issuers. | |
| properties: | |
| orgName: | |
| type: string | |
| description: Fixed org name for this mapping (required for API-created issuers). Globally unique across all issuers. | |
| orgDisplayName: | |
| type: string | |
| description: Display name for the org. | |
| roles: | |
| type: array | |
| items: | |
| type: string | |
| description: Static role list. Omit when `isServiceAccount` is true (auto-assigns PROVIDER_ADMIN + TENANT_ADMIN). | |
| audiences: | |
| type: array | |
| items: | |
| type: string | |
| description: Optional per-mapping audience filter. | |
| isServiceAccount: | |
| type: boolean | |
| description: |- | |
| When true, auto-assigns PROVIDER_ADMIN and TENANT_ADMIN roles (disconnected mode only). | |
| Cannot be combined with `roles`, `rolesAttribute`, or `orgAttribute`. | |
| At most one service-account mapping is permitted per org across all issuers. | |
| IssuerClaimMapping: | |
| type: object | |
| title: IssuerClaimMapping | |
| description: |- | |
| One entry in an issuer's claim mapping array. | |
| Via the API, only `orgName` (required), `roles` (optional static list), and | |
| `isServiceAccount` (disconnected mode only) may be set. Attribute-driven fields | |
| (`orgAttribute`, `orgDisplayAttribute`, `rolesAttribute`) are reserved for | |
| ConfigMap-defined issuers and will be rejected with `400` if supplied. | |
| Org names are globally unique across all issuers (both ConfigMap and API-created). | |
| At most one `isServiceAccount: true` mapping may exist per org across all issuers. | |
| required: | |
| - orgName | |
| properties: | |
| orgName: | |
| type: string | |
| description: Fixed org name for this mapping (required for API-created issuers). Globally unique across all issuers. | |
| orgDisplayName: | |
| type: string | |
| description: Display name for the org. | |
| roles: | |
| type: array | |
| items: | |
| type: string | |
| description: Static role list. Omit when `isServiceAccount` is true (auto-assigns PROVIDER_ADMIN + TENANT_ADMIN). | |
| audiences: | |
| type: array | |
| items: | |
| type: string | |
| description: Optional per-mapping audience filter. | |
| isServiceAccount: | |
| type: boolean | |
| description: |- | |
| When true, auto-assigns PROVIDER_ADMIN and TENANT_ADMIN roles (disconnected mode only). | |
| Cannot be combined with `roles`, `rolesAttribute`, or `orgAttribute`. | |
| At most one service-account mapping is permitted per org across all issuers. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 14931 - 14966, Add a required
declaration for orgName within the IssuerClaimMapping schema, alongside its
existing properties. Keep the current orgName type and description unchanged,
and make the requirement unconditional for this API schema.
Source: Path instructions
2 participants
feat(rest-api): add runtime JWT issuer create/delete
Allow Provider Admins to create, list, get, and delete external JWT issuers at runtime via DB-backed API, without ConfigMap edits or pod rolls. No update API — change via delete + create. Empty issuer table keeps today's static-only behavior.