Skip to content

Conversation

rohilsurana
Copy link
Member

@rohilsurana rohilsurana commented Sep 30, 2025

Summary

Migrate 6 policy APIs from gRPC to Connect RPC format as part of the IAM API standardization initiative.

APIs to Migrate

  • CreatePolicy - ✅ COMPLETED (commit: 23aa904)
  • GetPolicy - ✅ COMPLETED (commit: 7d19c25)
  • ListPolicies - ✅ COMPLETED (commit: c721241)
  • UpdatePolicy - ✅ COMPLETED (commit: 7067de7)
  • DeletePolicy - ✅ COMPLETED (commit: 62ae3f2)
  • CreatePolicyForProject - ✅ COMPLETED (commit: 23ad5d1)

Progress: 6/6 APIs Complete (100%) 🎉

Implementation Details

✅ CreatePolicy (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/policy.go - Connect RPC implementation
  • internal/api/v1beta1connect/policy_test.go - Comprehensive test suite with 8 scenarios
  • internal/api/v1beta1connect/mocks/policy_service.go - Mock service interface

Key Features:

  • ✅ Connect RPC request/response wrapping with connect.Request[T] and connect.Response[T]
  • ✅ Proper error handling with Connect error codes (InvalidArgument, Internal)
  • ✅ Namespace splitting for resource and principal validation
  • ✅ Metadata handling for policy attributes
  • ✅ Audit logging for policy creation events
  • ✅ Identical business logic preservation from gRPC version
  • ✅ 8 comprehensive test scenarios covering all edge cases

Test Coverage:

  • Invalid resource/principal namespace splitting errors
  • Role ID validation errors
  • Policy detail validation errors
  • Service layer error handling
  • Successful policy creation (basic data)
  • Successful policy creation (with metadata)
  • Group principal support
  • Metadata transformation errors

✅ GetPolicy (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/errors.go - Added ErrPolicyNotFound error constant
  • internal/api/v1beta1connect/policy.go - Added GetPolicy Connect RPC implementation
  • internal/api/v1beta1connect/policy_test.go - Added comprehensive test suite

Key Features:

  • ✅ Connect RPC request/response wrapping
  • ✅ Proper error mapping for policy retrieval scenarios
  • ✅ Connect error codes: CodeNotFound for missing policies, CodeInternal for service errors
  • ✅ Policy transformation with metadata support
  • ✅ Identical business logic preservation from gRPC version
  • ✅ 6 comprehensive test scenarios covering all edge cases

Test Coverage:

  • Internal server error when policy service fails
  • Not found error when ID is empty (ErrInvalidID)
  • Not found error when ID is not UUID (ErrInvalidUUID)
  • Not found error when policy doesn't exist (ErrNotExist)
  • Successful policy retrieval with all fields
  • Metadata transformation error handling

✅ ListPolicies (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/policy.go - Added ListPolicies Connect RPC implementation and resolveFilter helper
  • internal/api/v1beta1connect/policy_test.go - Added comprehensive test suite

Key Features:

  • ✅ Connect RPC request/response wrapping
  • ✅ Policy filtering with entity resolution for non-UUID filters (org, role, project, user)
  • ✅ Connect error codes: CodeInvalidArgument for filter errors, CodeInternal for service errors
  • ✅ Policy transformation with metadata support
  • ✅ Identical business logic preservation from gRPC version including resolveFilter functionality
  • ✅ 4 comprehensive test scenarios covering all edge cases

Test Coverage:

  • Internal server error when policy service fails
  • Successful policy listing with multiple policies
  • Empty results handling (nil slice behavior)
  • Metadata transformation error handling

✅ UpdatePolicy (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/policy.go - Added UpdatePolicy Connect RPC implementation
  • internal/api/v1beta1connect/policy_test.go - Added test suite

Key Features:

  • ✅ Connect RPC request/response wrapping
  • ✅ Unimplemented behavior matching original gRPC version
  • ✅ Connect error code: CodeUnimplemented with proper error message
  • ✅ Identical unimplemented status preservation from gRPC version
  • ✅ 1 test scenario verifying unimplemented behavior

Test Coverage:

  • Unimplemented error response validation
  • Correct Connect error code verification
  • Error message format confirmation

✅ DeletePolicy (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/policy.go - Added DeletePolicy Connect RPC implementation
  • internal/api/v1beta1connect/policy_test.go - Added comprehensive test suite

Key Features:

  • ✅ Connect RPC request/response wrapping
  • ✅ Comprehensive error handling with proper Connect error codes
  • ✅ Connect error codes: CodeNotFound for missing policies, CodeInvalidArgument for invalid details, CodeAlreadyExists for conflicts, CodeInternal for service errors
  • ✅ Audit logging preservation for successful deletions
  • ✅ Identical business logic preservation from gRPC version
  • ✅ 8 comprehensive test scenarios covering all edge cases

Test Coverage:

  • Policy not found scenarios (3 test cases: ErrNotExist, ErrInvalidID, ErrInvalidUUID)
  • Invalid argument scenarios (2 test cases: ErrInvalidDetail, namespace.ErrNotExist)
  • Conflict scenario (1 test case: ErrConflict)
  • Internal server error scenario (1 test case: unknown errors)
  • Successful deletion scenario (1 test case: with audit logging)

✅ CreatePolicyForProject (COMPLETED)

Files Changed:

  • internal/api/v1beta1connect/errors.go - Added ErrProjectNotFound error constant
  • internal/api/v1beta1connect/policy.go - Added CreatePolicyForProject Connect RPC implementation
  • internal/api/v1beta1connect/policy_test.go - Added comprehensive test suite

Key Features:

  • ✅ Connect RPC request/response wrapping with proper proto message types
  • ✅ Request body validation (role ID and principal required)
  • ✅ Principal namespace splitting and validation
  • ✅ Project lookup and validation with proper error handling
  • ✅ Connect error codes: CodeInvalidArgument for validation errors, CodeNotFound for missing projects, CodeInternal for service errors
  • ✅ Policy creation with project as resource scope
  • ✅ Audit logging preservation for successful creations
  • ✅ Identical business logic preservation from gRPC version
  • ✅ 9 comprehensive test scenarios covering all edge cases

Test Coverage:

  • Invalid argument errors (nil body, empty role ID, empty principal)
  • Principal namespace splitting validation
  • Project not found scenario
  • Role ID validation errors
  • Policy detail validation errors
  • Internal server errors
  • Successful policy creation for project

Testing

  • ✅ All existing tests pass
  • ✅ New Connect RPC tests added with comprehensive coverage
  • ✅ Build verification completed
  • ✅ Lint checks passed

Migration Pattern

Each API follows the established Connect RPC migration pattern:

  1. Request/response type wrapping
  2. Access request data via request.Msg.GetFieldName()
  3. Wrap responses with connect.NewResponse()
  4. Convert errors to Connect error format
  5. Preserve identical business logic
  6. Comprehensive test migration

Status

MIGRATION COMPLETE - All 6 policy APIs successfully migrated to Connect RPC!

Copy link

vercel bot commented Sep 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
frontier Ready Ready Preview Comment Oct 1, 2025 6:46am

@coveralls
Copy link

coveralls commented Sep 30, 2025

Pull Request Test Coverage Report for Build 18153861202

Details

  • 165 of 187 (88.24%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 34.982%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/v1beta1connect/policy.go 165 187 88.24%
Totals Coverage Status
Change from base Build 18153836563: 0.3%
Covered Lines: 13088
Relevant Lines: 37413

💛 - Coveralls

- Migrate CreatePolicy from gRPC to Connect RPC format
- Add comprehensive test coverage with 8 scenarios
- Preserve identical business logic and error handling
- Add Connect RPC error mappings for role.ErrInvalidID and policy.ErrInvalidDetail
- Include metadata handling and audit logging functionality
- Migrate GetPolicy from gRPC to Connect RPC format
- Add comprehensive test coverage with 6 scenarios
- Preserve identical business logic and error handling
- Map policy errors (ErrNotExist, ErrInvalidUUID, ErrInvalidID) to ErrPolicyNotFound
- Use Connect error codes (NotFound, Internal) for proper HTTP status mapping
- Include metadata transformation error handling
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates 6 policy APIs from gRPC to Connect RPC format, completing the IAM API standardization initiative. The migration follows established patterns for Connect RPC implementation while preserving identical business logic.

  • Migration of all 6 policy APIs to Connect RPC with proper request/response wrapping
  • Addition of comprehensive test coverage for all migrated APIs
  • Implementation of proper error handling with Connect error codes

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/api/v1beta1connect/relation.go Removed unused ErrNamespaceSplitNotation error variable
internal/api/v1beta1connect/policy_test.go Added comprehensive test suite for all 6 policy Connect RPC endpoints
internal/api/v1beta1connect/policy.go Implemented all 6 policy Connect RPC endpoints with proper error handling
internal/api/v1beta1connect/mocks/policy_service.go Generated mock service interface for policy service
internal/api/v1beta1connect/errors.go Added policy-specific error constants

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


resourceType, resourceID, err := schema.SplitNamespaceAndResourceID(request.Msg.GetBody().GetResource())
if err != nil {
return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation)
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

ErrNamespaceSplitNotation is being referenced but was removed from relation.go. This will cause a compile error since the error is not defined in this file.

Copilot uses AI. Check for mistakes.

}
principalType, principalID, err := schema.SplitNamespaceAndResourceID(request.Msg.GetBody().GetPrincipal())
if err != nil {
return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation)
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

ErrNamespaceSplitNotation is being referenced but was removed from relation.go. This will cause a compile error since the error is not defined in this file.

Copilot uses AI. Check for mistakes.


principalType, principalID, err := schema.SplitNamespaceAndResourceID(request.Msg.GetBody().GetPrincipal())
if err != nil {
return nil, connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation)
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

ErrNamespaceSplitNotation is being referenced but was removed from relation.go. This will cause a compile error since the error is not defined in this file.

Copilot uses AI. Check for mistakes.

},
}),
want: nil,
wantErr: ErrNamespaceSplitNotation,
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

ErrNamespaceSplitNotation is being referenced in the test but was removed from relation.go. This will cause a compile error since the error is not defined.

Suggested change
wantErr: ErrNamespaceSplitNotation,
wantErr: ErrBadRequest,

Copilot uses AI. Check for mistakes.

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.

2 participants