diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index 5ef0db534..b390806a9 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -701,55 +701,20 @@ 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, - ) - } +// ensureSubscriptionCoversOIR ensures that the subscription covers the requested geo-temporal extent, extending it if both possible and required, +// or failing otherwise. +// After this method returns successfully, the subscription will cover the requested geo-temporal extent. +func ensureSubscriptionCoversOIR(ctx context.Context, r repos.Repository, sub *scdmodels.Subscription, params *validOIRParams) (*scdmodels.Subscription, error) { + 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.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 @@ -767,10 +732,11 @@ func dependOnExistingSubscriptionIfAllowedAndCovered(ctx context.Context, r repo } } if updateSub { - sub, err = r.UpsertSubscription(ctx, sub) + upsertedSub, err := r.UpsertSubscription(ctx, sub) if err != nil { return nil, stacktrace.Propagate(err, "Failed to update existing Subscription") } + return upsertedSub, nil } return sub, nil @@ -830,15 +796,34 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } } else { - // Use existing Subscription and adapt it if needed and possible - sub, err = dependOnExistingSubscriptionIfAllowedAndCovered(ctx, r, manager, validParams) + // Attempt to rely on the specified subscription + sub, err = r.GetSubscription(ctx, validParams.subscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to get depended-upon Subscription") + } + if sub == nil { + return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Depended-upon Subscription %s does not exist", validParams.subscriptionID) + } + + // We need to confirm that it is owned by the calling manager + if sub.Manager != manager { + return 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", + validParams.subscriptionID, + sub.Manager, + manager, + ) + } + + // We need to ensure the subscription covers the OIR's geo-temporal extent + sub, err = ensureSubscriptionCoversOIR(ctx, r, sub, validParams) if err != nil { - // 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) + return stacktrace.Propagate(err, "Failed to ensure subscription covers OIR") } }