From 884cce37a7aac0e6f868653490fcd5623f4047a3 Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Tue, 3 Sep 2024 12:16:46 +0200 Subject: [PATCH] [scd] oir upsert: factor out extension and validation of existing subscription --- pkg/scd/operational_intents_handler.go | 125 ++++++++++++++++--------- 1 file changed, 83 insertions(+), 42 deletions(-) diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index ba95a63f6..c863c8912 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -660,6 +660,81 @@ func validateKeyAndProvideConflictResponse( return nil, nil } +// dependOnExistingSubscriptionIfAllowedAndCovered ensures that: +// - the subscription exists +// - the subscription is owned by the manager +// - the subscription covers the requested geo-temporal extent +// - if the subscription is implicit, it is extended if required to cover the requested parameters +// - if the subscription is explicit and it is not covering the requested parameters, an error is returned +// +// IMPORTANT: this method currently only EXTENDS implicit subscriptions: +// +// It WILL NOT shrink an implicit subscription if that subscription covers more than the area of its dependant OIRs. +// This is relevant for situations where clients manually specify implicit subscriptions that were previously +// created when creating or updating their OIRs: they may get notifications for areas they are not interested in. +func dependOnExistingSubscriptionIfAllowedAndCovered(ctx context.Context, r repos.Repository, manager dssmodels.Manager, params *validOIRParams) (*scdmodels.Subscription, error) { + if params.subscriptionID.Empty() { + // This situation should have been caught earlier but we check again to be safe + return nil, stacktrace.NewError("Cannot depend on a non-specified Subscription") + } + + sub, err := r.GetSubscription(context.Background(), params.subscriptionID) + if err != nil { + return nil, stacktrace.Propagate(err, "Unable to get depended-upon Subscription") + } + if sub == nil { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "depended-upon Subscription %s does not exist", params.subscriptionID) + } + + if sub.Manager != manager { + return nil, + stacktrace.Propagate( + // We do a bit of wrapping gymnastics because the root error message will be sent in the response, + // and we don't want to include the effective manager in there. + stacktrace.NewErrorWithCode( + dsserr.PermissionDenied, "Specificed Subscription is owned by different client"), + // The propagation message will end in the logs and help with debugging. + "Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", + params.subscriptionID, + sub.Manager, + manager, + ) + } + updateSub := false + if sub.StartTime != nil && sub.StartTime.After(*params.uExtent.StartTime) { + if sub.ImplicitSubscription { + sub.StartTime = params.uExtent.StartTime + updateSub = true + } else { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not begin until after the OperationalIntent starts") + } + } + if sub.EndTime != nil && sub.EndTime.Before(*params.uExtent.EndTime) { + if sub.ImplicitSubscription { + sub.EndTime = params.uExtent.EndTime + updateSub = true + } else { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription ends before the OperationalIntent ends") + } + } + if !sub.Cells.Contains(params.cells) { + if sub.ImplicitSubscription { + sub.Cells = s2.CellUnionFromUnion(sub.Cells, params.cells) + updateSub = true + } else { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not cover entire spatial area of the OperationalIntent") + } + } + if updateSub { + sub, err = r.UpsertSubscription(ctx, sub) + if err != nil { + return nil, stacktrace.Propagate(err, "Failed to update existing Subscription") + } + } + + return sub, nil +} + // upsertOperationalIntentReference inserts or updates an Operational Intent. // If the ovn argument is empty (""), it will attempt to create a new Operational Intent. func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorizedManager *api.AuthorizationResult, entityid restapi.EntityID, ovn restapi.EntityOVN, params *restapi.PutOperationalIntentReferenceParameters, @@ -739,49 +814,15 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } else { - // Use existing Subscription - sub, err = r.GetSubscription(ctx, validParams.subscriptionID) + // Use existing Subscription and adapt it if needed and possible + sub, err = dependOnExistingSubscriptionIfAllowedAndCovered(ctx, r, manager, validParams) if err != nil { - return stacktrace.Propagate(err, "Unable to get Subscription") - } - if sub == nil { - return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", validParams.subscriptionID) - } - if sub.Manager != manager { - return stacktrace.Propagate( - stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Specificed Subscription is owned by different client"), - "Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", validParams.subscriptionID, sub.Manager, manager) - } - updateSub := false - if sub.StartTime != nil && sub.StartTime.After(*validParams.uExtent.StartTime) { - if sub.ImplicitSubscription { - sub.StartTime = validParams.uExtent.StartTime - updateSub = true - } else { - return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not begin until after the OperationalIntent starts") - } - } - if sub.EndTime != nil && sub.EndTime.Before(*validParams.uExtent.EndTime) { - if sub.ImplicitSubscription { - sub.EndTime = validParams.uExtent.EndTime - updateSub = true - } else { - return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription ends before the OperationalIntent ends") - } - } - if !sub.Cells.Contains(validParams.cells) { - if sub.ImplicitSubscription { - sub.Cells = s2.CellUnionFromUnion(sub.Cells, validParams.cells) - updateSub = true - } else { - return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not cover entire spatial area of the OperationalIntent") - } - } - if updateSub { - sub, err = r.UpsertSubscription(ctx, sub) - if err != nil { - return stacktrace.Propagate(err, "Failed to update existing Subscription") - } + // Propagating the code here is important for properly handling all failure cases in the caller + return stacktrace.PropagateWithCode( + err, + stacktrace.GetCode(err), + "could not depend on specified subscription: %s", + validParams.subscriptionID) } }