Feat/delete intent #10
Conversation
Manager: - Add DELETE /api/v1/strategies to delete schedule strategy with cascade delete of associated intents - Add DELETE /api/v1/intents to batch delete schedule intents - Add new permissions: schedule_strategy.delete, schedule_intent.delete - Add Repository methods: DeleteStrategy, DeleteIntents, DeleteIntentsByStrategyID - Add Service methods: DeleteScheduleStrategy, DeleteScheduleIntents - Add integration tests for delete operations Decision Maker: - Add DELETE /api/v1/intents to delete scheduling intents - Support delete by PodID, by PodID+PID, or delete all - Add Service methods: DeleteIntentByPodID, DeleteIntentByPID, DeleteAllIntents
There was a problem hiding this comment.
Pull request overview
This pull request implements deletion functionality for scheduling strategies and intents across both the Manager and Decision Maker services. The feature allows users to delete strategies (which cascades to delete associated intents) and to delete individual intents.
Changes:
- Added DELETE endpoints for strategies and intents in both Manager and Decision Maker services
- Implemented authorization checks to ensure users can only delete their own resources
- Added cascade deletion logic to remove associated intents when a strategy is deleted
- Included integration tests for the new delete functionality
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| manager/service/strategy_svc.go | Added DeleteScheduleStrategy and DeleteScheduleIntents service methods with authorization checks |
| manager/rest/strategy_hdl.go | Added REST handlers for DELETE /strategies and DELETE /intents endpoints |
| manager/rest/routes.go | Registered new DELETE routes with appropriate auth middleware |
| manager/repository/strategy_repo.go | Added repository methods for deleting strategies and intents |
| manager/domain/interface.go | Extended Repository and Service interfaces with delete methods |
| manager/domain/enums.go | Added new permission constants for delete operations |
| manager/rest/strategy_hdl_test.go | Added integration tests for delete operations |
| decisionmaker/service/service.go | Added methods to delete intents from in-memory storage by PodID, PID, or all |
| decisionmaker/rest/intent_handler.go | Added DELETE handler for removing intents from decision maker |
| decisionmaker/rest/handler.go | Registered DELETE /intents route |
| .github/copilot-instructions.md | Updated documentation from BSS to Gthulhu API architecture |
| func (suite *HandlerTestSuite) TestIntegrationDeleteIntentsHandler() { | ||
| adminUser, adminPwd := config.GetManagerConfig().Account.AdminEmail, config.GetManagerConfig().Account.AdminPassword | ||
| adminToken := suite.login(adminUser, adminPwd.Value(), http.StatusOK) | ||
|
|
||
| strategyReq := rest.CreateScheduleStrategyRequest{ | ||
| LabelSelectors: []rest.LabelSelector{ | ||
| { | ||
| Key: "test", Value: "test", | ||
| }, | ||
| }, | ||
| Priority: 100, | ||
| ExecutionTime: 100, | ||
| } | ||
|
|
||
| // Create strategy | ||
| suite.MockK8SAdapter.EXPECT().QueryPods(mock.Anything, mock.Anything).Return([]*domain.Pod{{PodID: "Test1", Labels: map[string]string{"test": "test"}, NodeID: "test"}, {PodID: "Test2", Labels: map[string]string{"test": "test"}, NodeID: "test"}}, nil).Once() | ||
| suite.MockK8SAdapter.EXPECT().QueryDecisionMakerPods(mock.Anything, mock.Anything).Return([]*domain.DecisionMakerPod{{Host: "dm-host", NodeID: "test", Port: 8080}}, nil).Once() | ||
| suite.MockDMAdapter.EXPECT().SendSchedulingIntent(mock.Anything, mock.Anything, mock.Anything).Return(nil).Times(1) | ||
| suite.createStrategy(adminToken, &strategyReq, http.StatusOK) | ||
|
|
||
| intents := suite.listSelfIntents(adminToken, http.StatusOK) | ||
| suite.Require().Len(intents.Intents, 2, "Expected two intents") | ||
|
|
||
| // Delete one intent | ||
| suite.deleteIntents(adminToken, []string{intents.Intents[0].ID.Hex()}, http.StatusOK) | ||
|
|
||
| // Verify only one intent remains | ||
| intents = suite.listSelfIntents(adminToken, http.StatusOK) | ||
| suite.Require().Len(intents.Intents, 1, "Expected one intent after deletion") | ||
| } |
There was a problem hiding this comment.
Similar to TestIntegrationDeleteStrategyHandler, this test only validates the happy path. It should also test the authorization check by attempting to delete intents created by a different user, which should fail. Due to the bug in the repository layer (missing CreatorIDs filter), this authorization check would currently fail to prevent unauthorized access.
| queryOpt := &domain.QueryStrategyOptions{ | ||
| IDs: []bson.ObjectID{strategyObjID}, | ||
| CreatorIDs: []bson.ObjectID{operatorID}, | ||
| } | ||
| err = svc.Repo.QueryStrategies(ctx, queryOpt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(queryOpt.Result) == 0 { | ||
| return errs.NewHTTPStatusError(http.StatusNotFound, "strategy not found or you don't have permission to delete it", nil) | ||
| } |
There was a problem hiding this comment.
Authorization bypass vulnerability: The DeleteScheduleStrategy method queries strategies with both IDs and CreatorIDs filters to verify ownership, but the repository's QueryStrategies method does not implement the CreatorIDs filter. This means a user could delete any strategy by ID, regardless of who created it. The ownership check on line 129 will incorrectly pass if any strategy with that ID exists, even if it belongs to a different user.
The same issue exists in DeleteScheduleIntents (line 173) where QueryIntents is called with CreatorIDs but the repository doesn't filter by this field.
| func (svc *Service) DeleteScheduleStrategy(ctx context.Context, operator *domain.Claims, strategyID string) error { | ||
| strategyObjID, err := bson.ObjectIDFromHex(strategyID) | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid strategy ID %s", strategyID) | ||
| } | ||
|
|
||
| operatorID, err := operator.GetBsonObjectUID() | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid operator ID %s", operator.UID) | ||
| } | ||
|
|
||
| // Check if strategy exists and belongs to the operator | ||
| queryOpt := &domain.QueryStrategyOptions{ | ||
| IDs: []bson.ObjectID{strategyObjID}, | ||
| CreatorIDs: []bson.ObjectID{operatorID}, | ||
| } | ||
| err = svc.Repo.QueryStrategies(ctx, queryOpt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(queryOpt.Result) == 0 { | ||
| return errs.NewHTTPStatusError(http.StatusNotFound, "strategy not found or you don't have permission to delete it", nil) | ||
| } | ||
|
|
||
| // Delete associated intents first | ||
| err = svc.Repo.DeleteIntentsByStrategyID(ctx, strategyObjID) | ||
| if err != nil { | ||
| return fmt.Errorf("delete intents by strategy ID: %w", err) | ||
| } | ||
|
|
||
| // Delete the strategy | ||
| err = svc.Repo.DeleteStrategy(ctx, strategyObjID) | ||
| if err != nil { | ||
| return fmt.Errorf("delete strategy: %w", err) | ||
| } | ||
|
|
||
| logger.Logger(ctx).Info().Msgf("deleted strategy %s and its associated intents", strategyID) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When deleting a strategy and its associated intents, the decision makers are not notified to remove the corresponding in-memory scheduling intents. The CreateScheduleStrategy method sends intents to decision makers via DMAdapter.SendSchedulingIntent, but DeleteScheduleStrategy only deletes from the database without notifying decision makers. This will cause decision makers to continue applying deleted scheduling policies until they are restarted or the intents are manually deleted via the decision maker's DELETE /api/v1/intents endpoint.
| func (svc *Service) DeleteScheduleIntents(ctx context.Context, operator *domain.Claims, intentIDs []string) error { | ||
| if len(intentIDs) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| operatorID, err := operator.GetBsonObjectUID() | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid operator ID %s", operator.UID) | ||
| } | ||
|
|
||
| intentObjIDs := make([]bson.ObjectID, 0, len(intentIDs)) | ||
| for _, id := range intentIDs { | ||
| objID, err := bson.ObjectIDFromHex(id) | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid intent ID %s", id) | ||
| } | ||
| intentObjIDs = append(intentObjIDs, objID) | ||
| } | ||
|
|
||
| // Check if intents exist and belong to the operator | ||
| queryOpt := &domain.QueryIntentOptions{ | ||
| IDs: intentObjIDs, | ||
| CreatorIDs: []bson.ObjectID{operatorID}, | ||
| } | ||
| err = svc.Repo.QueryIntents(ctx, queryOpt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(queryOpt.Result) != len(intentIDs) { | ||
| return errs.NewHTTPStatusError(http.StatusNotFound, "one or more intents not found or you don't have permission to delete them", nil) | ||
| } | ||
|
|
||
| // Delete the intents | ||
| err = svc.Repo.DeleteIntents(ctx, intentObjIDs) | ||
| if err != nil { | ||
| return fmt.Errorf("delete intents: %w", err) | ||
| } | ||
|
|
||
| logger.Logger(ctx).Info().Msgf("deleted %d intents", len(intentIDs)) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Similar to DeleteScheduleStrategy, when deleting individual intents, the decision makers are not notified to remove the corresponding in-memory scheduling intents. This will cause decision makers to continue applying deleted scheduling policies. You should either add a method to DecisionMakerAdapter for deleting intents, or query the affected intents to determine which decision maker pods to notify before deletion.
| func (suite *HandlerTestSuite) TestIntegrationDeleteStrategyHandler() { | ||
| adminUser, adminPwd := config.GetManagerConfig().Account.AdminEmail, config.GetManagerConfig().Account.AdminPassword | ||
| adminToken := suite.login(adminUser, adminPwd.Value(), http.StatusOK) | ||
|
|
||
| strategyReq := rest.CreateScheduleStrategyRequest{ | ||
| LabelSelectors: []rest.LabelSelector{ | ||
| { | ||
| Key: "test", Value: "test", | ||
| }, | ||
| }, | ||
| Priority: 100, | ||
| ExecutionTime: 100, | ||
| } | ||
|
|
||
| // Create strategy | ||
| suite.MockK8SAdapter.EXPECT().QueryPods(mock.Anything, mock.Anything).Return([]*domain.Pod{{PodID: "Test", Labels: map[string]string{"test": "test"}, NodeID: "test"}}, nil).Once() | ||
| suite.MockK8SAdapter.EXPECT().QueryDecisionMakerPods(mock.Anything, mock.Anything).Return([]*domain.DecisionMakerPod{{Host: "dm-host", NodeID: "test", Port: 8080}}, nil).Once() | ||
| suite.MockDMAdapter.EXPECT().SendSchedulingIntent(mock.Anything, mock.Anything, mock.Anything).Return(nil).Times(1) | ||
| suite.createStrategy(adminToken, &strategyReq, http.StatusOK) | ||
|
|
||
| strategies := suite.listSelfStrategies(adminToken, http.StatusOK) | ||
| suite.Require().Len(strategies.Strategies, 1, "Expected one strategy") | ||
|
|
||
| // Delete the strategy | ||
| suite.deleteStrategy(adminToken, strategies.Strategies[0].ID.Hex(), http.StatusOK) | ||
|
|
||
| // Verify strategy and intents are deleted | ||
| strategies = suite.listSelfStrategies(adminToken, http.StatusOK) | ||
| suite.Require().Len(strategies.Strategies, 0, "Expected no strategies after deletion") | ||
|
|
||
| intents := suite.listSelfIntents(adminToken, http.StatusOK) | ||
| suite.Require().Len(intents.Intents, 0, "Expected no intents after strategy deletion") | ||
| } |
There was a problem hiding this comment.
The test only validates the happy path where an admin user deletes their own strategy. It should also test the authorization check by attempting to delete a strategy created by a different user, which should fail with a 404 error. However, due to the bug in the repository layer (missing CreatorIDs filter), this test would currently pass incorrectly if implemented.
- Add CreatorIDs filter support in QueryStrategies and QueryIntents repository methods - Improve ownership verification in DeleteScheduleIntents service method - Fix sync.Map concurrent delete issue in DecisionMaker's DeleteAllIntents - Use EmptyResponse instead of nil in delete handlers - Add DeleteSchedulingIntents to notify DecisionMakers when deleting strategies/intents
No description provided.