diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index 533b6f5b2..d8f6f9cc1 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -17,7 +17,7 @@ import ( ) // subscriptionIsOnlyAttachedToOIR will check if: -// - the subscription exists and is implicit +// - 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 // @@ -27,29 +27,22 @@ import ( // 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 subscriptionIsOnlyAttachedToOIR(ctx context.Context, r repos.Repository, oirID, subscriptionID *dssmodels.ID) (bool, error) { - // Get the Subscription supporting the OperationalIntent, if one is defined - if subscriptionID != nil { - sub, err := r.GetSubscription(ctx, *subscriptionID) - if err != nil { - return false, stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") - } - if sub == nil { - return false, 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 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 - } - } +func subscriptionIsOnlyAttachedToOIR(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 } @@ -94,7 +87,18 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest "OperationalIntent owned by %s, but %s attempted to delete", old.Manager, *req.Auth.ClientID) } - removeImplicitSubscription, err := subscriptionIsOnlyAttachedToOIR(ctx, r, &id, old.SubscriptionID) + var previousSubscription *scdmodels.Subscription + 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") + } + if previousSubscription == nil { + return stacktrace.NewError("OperationalIntent's Subscription missing from repo") + } + } + + removeImplicitSubscription, err := subscriptionIsOnlyAttachedToOIR(ctx, r, &id, previousSubscription) if err != nil { return stacktrace.Propagate(err, "Could not determine if Subscription can be removed") } @@ -440,9 +444,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) } @@ -451,7 +455,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) @@ -476,6 +480,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } var previousSubscriptionID *dssmodels.ID + var previousSubscription *scdmodels.Subscription if old != nil { if old.Manager != manager { return stacktrace.NewErrorWithCode(dsserr.PermissionDenied, @@ -488,6 +493,10 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize version = int32(old.Version) previousSubscriptionID = old.SubscriptionID + previousSubscription, err = r.GetSubscription(ctx, *previousSubscriptionID) + 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) @@ -498,7 +507,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize var sub *scdmodels.Subscription removePreviousImplicitSubscription := false - if subscriptionID.Empty() { + if specifiedSubscriptionID.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. @@ -511,7 +520,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } - removePreviousImplicitSubscription, err = subscriptionIsOnlyAttachedToOIR(ctx, r, &id, previousSubscriptionID) + removePreviousImplicitSubscription, err = subscriptionIsOnlyAttachedToOIR(ctx, r, &id, previousSubscription) if err != nil { return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed") } @@ -542,18 +551,28 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } 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) + // Use the specified subscription. + // First, check if we have replaced an existing implicit subscription + if specifiedSubscriptionID == *previousSubscriptionID { + sub = previousSubscription + } else { + removePreviousImplicitSubscription, err = subscriptionIsOnlyAttachedToOIR(ctx, r, &id, previousSubscription) + if err != nil { + return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed") + + } + sub, err = r.GetSubscription(ctx, specifiedSubscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to get requested Subscription from store") + } + if sub == nil { + return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", specifiedSubscriptionID) + } } if sub.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, sub.Manager, manager) } updateSub := false if sub.StartTime != nil && sub.StartTime.After(*uExtent.StartTime) {