diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index 5d8c2ac2d..6d2ef8a28 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -16,6 +16,37 @@ import ( "github.com/interuss/stacktrace" ) +// subscriptionIsImplicitAndOnlyAttachedToOIR will check if: +// - the subscription is defined and is implicit +// - the subscription is attached to the specified operational intent +// - the subscription is not attached to any other operational intent +// +// This is to be used in contexts where an implicit subscription may need to be cleaned up: if true is returned, +// the subscription can be safely removed after the operational intent is deleted or attached to another subscription. +// +// NOTE: this should eventually be pushed down to CRDB as part of the queries being executed in the callers of this method. +// +// See https://github.com/interuss/dss/issues/1059 for more details +func subscriptionIsImplicitAndOnlyAttachedToOIR(ctx context.Context, r repos.Repository, oirID *dssmodels.ID, subscription *scdmodels.Subscription) (bool, error) { + if subscription == nil { + return false, nil + } + if !subscription.ImplicitSubscription { + return false, nil + } + // Get the Subscription's dependent OperationalIntents + dependentOps, err := r.GetDependentOperationalIntents(ctx, subscription.ID) + if err != nil { + return false, stacktrace.Propagate(err, "Could not find dependent OperationalIntents") + } + if len(dependentOps) == 0 { + return false, stacktrace.NewError("An implicit Subscription had no dependent OperationalIntents") + } else if len(dependentOps) == 1 && dependentOps[0] == *oirID { + return true, nil + } + return false, nil +} + // DeleteOperationalIntentReference deletes a single operational intent ref for a given ID at // the specified version. func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *restapi.DeleteOperationalIntentReferenceRequest, @@ -68,30 +99,20 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest "Current version is %s but client specified version %s", old.OVN, ovn) } - // Get the Subscription supporting the OperationalIntent, if one is defined - var sub *scdmodels.Subscription - removeImplicitSubscription := false + var previousSubscription *scdmodels.Subscription if old.SubscriptionID != nil { - sub, err = r.GetSubscription(ctx, *old.SubscriptionID) + previousSubscription, err = r.GetSubscription(ctx, *old.SubscriptionID) if err != nil { return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") } - if sub == nil { + if previousSubscription == nil { return stacktrace.NewError("OperationalIntent's Subscription missing from repo") } + } - if sub.ImplicitSubscription { - // Get the Subscription's dependent OperationalIntents - dependentOps, err := r.GetDependentOperationalIntents(ctx, sub.ID) - if err != nil { - return stacktrace.Propagate(err, "Could not find dependent OperationalIntents") - } - if len(dependentOps) == 0 { - return stacktrace.NewError("An implicit Subscription had no dependent OperationalIntents") - } else if len(dependentOps) == 1 { - removeImplicitSubscription = true - } - } + removeImplicitSubscription, err := subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, &id, previousSubscription) + if err != nil { + return stacktrace.Propagate(err, "Could not determine if Subscription can be removed") } // Find Subscriptions that may overlap the OperationalIntent's Volume4D @@ -130,7 +151,7 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest // removeImplicitSubscription is only true if the OIR had a subscription defined if removeImplicitSubscription { // Automatically remove a now-unused implicit Subscription - err = r.DeleteSubscription(ctx, sub.ID) + err = r.DeleteSubscription(ctx, *old.SubscriptionID) if err != nil { return stacktrace.Propagate(err, "Unable to delete associated implicit Subscription") } @@ -437,9 +458,9 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid state for initial version: `%s`", params.State) } - subscriptionID := dssmodels.ID("") + specifiedSubscriptionID := dssmodels.ID("") if params.SubscriptionId != nil { - subscriptionID, err = dssmodels.IDFromOptionalString(string(*params.SubscriptionId)) + specifiedSubscriptionID, err = dssmodels.IDFromOptionalString(string(*params.SubscriptionId)) if err != nil { return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format for Subscription ID: `%s`", *params.SubscriptionId) } @@ -448,7 +469,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize // Check if a subscription is required for this request: // OIRs in an accepted state do not need a subscription. if state.RequiresSubscription() && - subscriptionID.Empty() && + specifiedSubscriptionID.Empty() && (params.NewSubscription == nil || params.NewSubscription.UssBaseUrl == "") { return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Provided Operational Intent Reference state `%s` requires either a subscription ID or information to create an implicit subscription", state) @@ -471,6 +492,8 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize if err != nil { return stacktrace.Propagate(err, "Could not get OperationalIntent from repo") } + + var previousSubscription *scdmodels.Subscription if old != nil { if old.Manager != manager { return stacktrace.NewErrorWithCode(dsserr.PermissionDenied, @@ -482,6 +505,13 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } version = int32(old.Version) + + if old.SubscriptionID != nil { + previousSubscription, err = r.GetSubscription(ctx, *old.SubscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") + } + } } else { if ovn != "" { return stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", ovn) @@ -490,12 +520,23 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize version = 0 } - var sub *scdmodels.Subscription - if subscriptionID.Empty() { - // Create an implicit subscription if the implicit subscription params are set: - // for situations where these params are required but have not been set, - // an error will have been returned earlier. - // If they are not set at this point, continue without creating an implicit subscription. + // Determine if the previous subscription needs to be cleaned up + previousSubIsBeingReplaced := previousSubscription != nil && specifiedSubscriptionID != previousSubscription.ID + removePreviousImplicitSubscription := false + if previousSubIsBeingReplaced { + removePreviousImplicitSubscription, err = subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, &id, previousSubscription) + if err != nil { + return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed") + } + } + + // effectiveSubscription is the subscription that will end up being attached to the OIR + // it defaults to the previous subscription, but may be updated if required by the parameters + effectiveSubscription := previousSubscription + if specifiedSubscriptionID.Empty() { + // No subscription ID was provided: + // We will check if parameters to create a new implicit subscription were provided, + // otherwise we will do nothing. if params.NewSubscription != nil && params.NewSubscription.UssBaseUrl != "" { if !a.AllowHTTPBaseUrls { err := scdmodels.ValidateUSSBaseURL(string(params.NewSubscription.UssBaseUrl)) @@ -504,6 +545,9 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } + // Note: parameters for a new implicit subscription have been passed, so we will create + // a new implicit subscription even if another subscription was attaches to this OIR before, + // (and regardless of whether it was an implicit subscription or not). subToUpsert := scdmodels.Subscription{ ID: dssmodels.ID(uuid.New().String()), Manager: manager, @@ -520,53 +564,57 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize subToUpsert.NotifyForConstraints = *params.NewSubscription.NotifyForConstraints } - sub, err = r.UpsertSubscription(ctx, &subToUpsert) + effectiveSubscription, err = r.UpsertSubscription(ctx, &subToUpsert) if err != nil { return stacktrace.Propagate(err, "Failed to create implicit subscription") } } } else { - // Use existing Subscription - sub, err = r.GetSubscription(ctx, subscriptionID) - 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", subscriptionID) + // A subscription has been specified: + // If it is different from the previous subscription, we need to fetch it from the store + // in order to ensure it correctly covers the OIR. + if effectiveSubscription == nil || previousSubIsBeingReplaced { + effectiveSubscription, err = r.GetSubscription(ctx, specifiedSubscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to get requested Subscription from store") + } + if effectiveSubscription == nil { + return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", specifiedSubscriptionID) + } } - if sub.Manager != dssmodels.Manager(manager) { + if effectiveSubscription.Manager != dssmodels.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", subscriptionID, sub.Manager, manager) + "Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", specifiedSubscriptionID, effectiveSubscription.Manager, manager) } updateSub := false - if sub.StartTime != nil && sub.StartTime.After(*uExtent.StartTime) { - if sub.ImplicitSubscription { - sub.StartTime = uExtent.StartTime + if effectiveSubscription.StartTime != nil && effectiveSubscription.StartTime.After(*uExtent.StartTime) { + if effectiveSubscription.ImplicitSubscription { + effectiveSubscription.StartTime = 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(*uExtent.EndTime) { - if sub.ImplicitSubscription { - sub.EndTime = uExtent.EndTime + if effectiveSubscription.EndTime != nil && effectiveSubscription.EndTime.Before(*uExtent.EndTime) { + if effectiveSubscription.ImplicitSubscription { + effectiveSubscription.EndTime = uExtent.EndTime updateSub = true } else { return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription ends before the OperationalIntent ends") } } - if !sub.Cells.Contains(cells) { - if sub.ImplicitSubscription { - sub.Cells = s2.CellUnionFromUnion(sub.Cells, cells) + if !effectiveSubscription.Cells.Contains(cells) { + if effectiveSubscription.ImplicitSubscription { + effectiveSubscription.Cells = s2.CellUnionFromUnion(effectiveSubscription.Cells, 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) + effectiveSubscription, err = r.UpsertSubscription(ctx, effectiveSubscription) if err != nil { return stacktrace.Propagate(err, "Failed to update existing Subscription") } @@ -601,7 +649,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize // Identify Constraints missing from the key var missingConstraints []*scdmodels.Constraint - if sub != nil && sub.NotifyForConstraints { + if effectiveSubscription != nil && effectiveSubscription.NotifyForConstraints { constraints, err := r.SearchConstraints(ctx, uExtent) if err != nil { return stacktrace.Propagate(err, "Unable to SearchConstraints") @@ -654,8 +702,8 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize // in such cases the subscription ID on scdmodels.OperationalIntent will be nil // and will be replaced with the 'NullV4UUID' when sent over to a client. var subID *dssmodels.ID = nil - if sub != nil { - subID = &sub.ID + if effectiveSubscription != nil { + subID = &effectiveSubscription.ID } // Construct the new OperationalIntent @@ -706,6 +754,14 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize return stacktrace.Propagate(err, "Failed to upsert OperationalIntent in repo") } + // Check if the previously attached subscription should be removed + if removePreviousImplicitSubscription { + err = r.DeleteSubscription(ctx, previousSubscription.ID) + if err != nil { + return stacktrace.Propagate(err, "Unable to delete previous implicit Subscription") + } + } + // Find Subscriptions that may need to be notified allsubs, err := r.SearchSubscriptions(ctx, notifyVol4) if err != nil {