Skip to content

Commit

Permalink
second round of comments, handle another corner case
Browse files Browse the repository at this point in the history
  • Loading branch information
@Shastick
Shastick committed Aug 23, 2024
1 parent 0cb622a commit cfc4895
Showing 1 changed file with 58 additions and 38 deletions.
96 changes: 58 additions & 38 deletions pkg/scd/operational_intents_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand All @@ -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
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
Expand All @@ -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,
Expand All @@ -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)
Expand All @@ -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.
Expand All @@ -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")
}
Expand Down Expand Up @@ -542,18 +551,29 @@ 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
var currentSubscription *scdmodels.Subscription
if specifiedSubscriptionID == *previousSubscriptionID {
currentSubscription = 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")

}
currentSubscription, err = r.GetSubscription(ctx, specifiedSubscriptionID)
if err != nil {
return stacktrace.Propagate(err, "Unable to get requested Subscription from store")
}
if currentSubscription == 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) {
Expand Down

0 comments on commit cfc4895

Please sign in to comment.