Skip to content

feat(server): create ziti services for apps#9

Merged
rowan-stein merged 2 commits into
mainfrom
noa/issue-8
Apr 6, 2026
Merged

feat(server): create ziti services for apps#9
rowan-stein merged 2 commits into
mainfrom
noa/issue-8

Conversation

@casey-brooks
Copy link
Copy Markdown
Contributor

Summary

  • update BSR-locked API protos and regenerate bindings
  • create Ziti services during app registration and persist service IDs
  • align enroll/delete cleanup flows and tests with new Ziti management API

Testing

  • buf generate --template buf.gen.yaml
  • go vet ./...
  • go test ./...

Closes #8

@casey-brooks
Copy link
Copy Markdown
Contributor Author

Test & Lint Summary

  • buf generate --template buf.gen.yaml
  • go vet ./...
  • go test ./...

Tests: passed 2, failed 0, skipped 0
Lint: go vet ./... (no issues)

@rowan-stein
Copy link
Copy Markdown
Collaborator

Requesting review — Phase 4 implementation: OpenZiti service creation during app registration, enrollment and deletion cleanup updates.

Key changes:

  • CreateApp: calls CreateService("app-{slug}", ["app-services"]) and stores service ID
  • EnrollApp: removed redundant pre-enrollment cleanup (Ziti Management handles idempotently); service ID from DB
  • cleanupZitiIdentity: signature changed to use platform identity_id instead of ziti_identity_id
  • DeleteApp: guard changed to check ZitiServiceID, uses identity_id for cleanup
  • Proto stubs regenerated from BSR
  • Tests updated for new behavior

CI passes.

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Overall this PR is well-structured and closely follows the issue specification. The proto regeneration, EnrollApp changes (removing pre-enrollment cleanup, sourcing service ID from app record), DeleteApp guard change, and cleanupZitiIdentity signature update all look correct.

One major issue found: the CreateApp error path leaks the newly created Ziti service when store.CreateApp fails. The cleanup only removes the authorization tuple but not the Ziti service. The corresponding test (TestCreateAppRollbackOnStoreError) also needs updating to assert proper cleanup.

Please fix the resource leak and update the test before merge.

Comment thread internal/server/server.go
@casey-brooks
Copy link
Copy Markdown
Contributor Author

Update

  • clean up Ziti service on CreateApp store failure
  • update TestCreateAppRollbackOnStoreError to assert service cleanup

Test & Lint Summary

  • go vet ./...
  • go test ./...

Tests: passed 2, failed 0, skipped 0
Lint: go vet ./... (no issues)

Copy link
Copy Markdown

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

The Ziti service leak in CreateApp is fixed — cleanupZitiIdentity is now called before cleanupAuthorization on store failure, and the test correctly validates the cleanup with identity ID and service ID assertions. All prior feedback is resolved.

LGTM — the PR fully addresses issue #8 across all specified changes: Ziti service creation at registration, updated enrollment flow, updated deletion cleanup, and comprehensive test coverage.

@rowan-stein rowan-stein merged commit 23db356 into main Apr 6, 2026
1 check passed
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.

feat: add OpenZiti service creation to registration and update identity cleanup

3 participants