Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[scd] factor out parameter validation for oir upsert method #1089

Merged
merged 3 commits into from
Aug 30, 2024
Merged

[scd] factor out parameter validation for oir upsert method #1089

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8524493
[scd] factor out parameter validation for oir upsert method
Shastick Aug 30, 2024
4fba42a
comments
Shastick Aug 30, 2024
f041be8
rename var name and struct
Shastick Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 116 additions & 79 deletions pkg/scd/operational_intents_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,95 +363,132 @@ func (a *Server) UpdateOperationalIntentReference(ctx context.Context, req *rest
return restapi.UpdateOperationalIntentReferenceResponseSet{Response200: respOK}
}

// 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,
) (*restapi.ChangeOperationalIntentReferenceResponse, *restapi.AirspaceConflictResponse, error) {
type validOIRUpsertParams struct {
manager dssmodels.Manager
id dssmodels.ID
ovn restapi.EntityOVN
state scdmodels.OperationalIntentState
extents []*dssmodels.Volume4D
uExtent *dssmodels.Volume4D
cells s2.CellUnion
subscriptionID dssmodels.ID
}

func (a *Server) validateAndReturnUpsertParams(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass AllowHTTPBaseUrls as a param to avoid attaching this to Server?

authorizedManager *api.AuthorizationResult,
entityid restapi.EntityID,
ovn restapi.EntityOVN,
params *restapi.PutOperationalIntentReferenceParameters,
) (*validOIRUpsertParams, error) {

valid := &validOIRUpsertParams{}
var err error

if authorizedManager.ClientID == nil {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Missing manager")
return nil, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Missing manager")
}
manager := dssmodels.Manager(*authorizedManager.ClientID)
valid.manager = dssmodels.Manager(*authorizedManager.ClientID)

id, err := dssmodels.IDFromString(string(entityid))
valid.id, err = dssmodels.IDFromString(string(entityid))
if err != nil {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format: `%s`", entityid)
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format: `%s`", entityid)
}

var (
extents = make([]*dssmodels.Volume4D, len(params.Extents))
)

if len(params.UssBaseUrl) == 0 {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing required UssBaseUrl")
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing required UssBaseUrl")
}

if !a.AllowHTTPBaseUrls {
err = scdmodels.ValidateUSSBaseURL(string(params.UssBaseUrl))
if err != nil {
return nil, nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to validate base URL")
return nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to validate base URL")
}
}

state := scdmodels.OperationalIntentState(params.State)
if !state.IsValidInDSS() {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid OperationalIntent state: %s", params.State)
valid.state = scdmodels.OperationalIntentState(params.State)
if !valid.state.IsValidInDSS() {
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid OperationalIntent state: %s", params.State)
}

hasCMSARole := auth.HasScope(authorizedManager.Scopes, restapi.UtmConformanceMonitoringSaScope)
if state.RequiresCMSA() && !hasCMSARole {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Missing `%s` Conformance Monitoring for Situational Awareness scope to transition to CMSA state: %s (see SCD0100)", restapi.UtmConformanceMonitoringSaScope, params.State)
if valid.state.RequiresCMSA() && !hasCMSARole {
return nil, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Missing `%s` Conformance Monitoring for Situational Awareness scope to transition to CMSA state: %s (see SCD0100)", restapi.UtmConformanceMonitoringSaScope, params.State)
}

valid.extents = make([]*dssmodels.Volume4D, len(params.Extents))

for idx, extent := range params.Extents {
cExtent, err := dssmodels.Volume4DFromSCDRest(&extent)
if err != nil {
return nil, nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to parse extent %d", idx)
return nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to parse extent %d", idx)
}
extents[idx] = cExtent
valid.extents[idx] = cExtent
}
uExtent, err := dssmodels.UnionVolumes4D(extents...)

valid.uExtent, err = dssmodels.UnionVolumes4D(valid.extents...)
if err != nil {
return nil, nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to union extents")
return nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Failed to union extents")
}

if uExtent.StartTime == nil {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing time_start from extents")
if valid.uExtent.StartTime == nil {
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing time_start from extents")
}
if uExtent.EndTime == nil {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing time_end from extents")
if valid.uExtent.EndTime == nil {
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Missing time_end from extents")
}

if time.Now().After(*uExtent.EndTime) {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "OperationalIntents may not end in the past")
if time.Now().After(*valid.uExtent.EndTime) {
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "OperationalIntents may not end in the past")
}

cells, err := uExtent.CalculateSpatialCovering()
valid.cells, err = valid.uExtent.CalculateSpatialCovering()
if err != nil {
return nil, nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Invalid area")
return nil, stacktrace.PropagateWithCode(err, dsserr.BadRequest, "Invalid area")
}

if uExtent.EndTime.Before(*uExtent.StartTime) {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "End time is past the start time")
if valid.uExtent.EndTime.Before(*valid.uExtent.StartTime) {
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "End time is past the start time")
}

if ovn == "" && params.State != restapi.OperationalIntentState_Accepted {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid state for initial version: `%s`", params.State)
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid state for initial version: `%s`", params.State)
}
valid.ovn = ovn

subscriptionID := dssmodels.ID("")
if params.SubscriptionId != nil {
subscriptionID, err = dssmodels.IDFromOptionalString(string(*params.SubscriptionId))
valid.subscriptionID, 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)
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format for Subscription ID: `%s`", *params.SubscriptionId)
}
}

// Check if a subscription is required for this request:
// OIRs in an accepted state do not need a subscription.
if state.RequiresSubscription() &&
subscriptionID.Empty() &&
if valid.state.RequiresSubscription() &&
valid.subscriptionID.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)
return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Provided Operational Intent Reference state `%s` requires either a subscription ID or information to create an implicit subscription", valid.state)
}

return valid, 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,
) (*restapi.ChangeOperationalIntentReferenceResponse, *restapi.AirspaceConflictResponse, error) {
// Note: validateAndReturnUpsertParams could be moved out of this method and only the valid params passed,
// but this requires some changes in the caller that go beyond the immediate scope of #1088 and can be done later.
upsertParams, err := a.validateAndReturnUpsertParams(authorizedManager, entityid, ovn, params)
if err != nil {
// Important note:
// - stacktrace.Propagate sets NoCode as the code for the error
// - subsequently, stacktrace.GetCode(err) only looks into the first error in the stack and returns NoCode if the error has no code
// - therefore, we need to explicitly use stacktrace.PropagateWithCode if we want the calling logic
// to be able to retrieve the code of the error
// TODO we can consider fixing this in the stacktrace library
return nil, nil, stacktrace.PropagateWithCode(err, stacktrace.GetCode(err), "Failed to validate Operational Intent Reference upsert parameters")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I suggest making validateAndReturnUpsertParams purely a validation function by:

  • move out of validateAndReturnUpsertParams the two cases of errors dsserr.PermissionDenied
  • never returning any code within validateAndReturnUpsertParams
  • if err != nil set the code dsserr.BadRequest

That makes it clear this function validates the input to create the op intent and any error implies bad input. That should also enable passing the manager instead of the whole authorizedManager.

}

var responseOK *restapi.ChangeOperationalIntentReferenceResponse
Expand All @@ -461,20 +498,20 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

// Lock subscriptions based on the cell to reduce the number of retries under concurrent load.
// See issue #1002 for details.
err = r.LockSubscriptionsOnCells(ctx, cells)
err = r.LockSubscriptionsOnCells(ctx, upsertParams.cells)
if err != nil {
return stacktrace.Propagate(err, "Unable to acquire lock")
}

// Get existing OperationalIntent, if any, and validate request
old, err := r.GetOperationalIntent(ctx, id)
old, err := r.GetOperationalIntent(ctx, upsertParams.id)
if err != nil {
return stacktrace.Propagate(err, "Could not get OperationalIntent from repo")
}
if old != nil {
if old.Manager != manager {
if old.Manager != upsertParams.manager {
return stacktrace.NewErrorWithCode(dsserr.PermissionDenied,
"OperationalIntent owned by %s, but %s attempted to modify", old.Manager, manager)
"OperationalIntent owned by %s, but %s attempted to modify", old.Manager, upsertParams.manager)
}
if old.OVN != scdmodels.OVN(ovn) {
return stacktrace.NewErrorWithCode(dsserr.VersionMismatch,
Expand All @@ -491,7 +528,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
}

var sub *scdmodels.Subscription
if subscriptionID.Empty() {
if upsertParams.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.
Expand All @@ -506,12 +543,12 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

subToUpsert := scdmodels.Subscription{
ID: dssmodels.ID(uuid.New().String()),
Manager: manager,
StartTime: uExtent.StartTime,
EndTime: uExtent.EndTime,
AltitudeLo: uExtent.SpatialVolume.AltitudeLo,
AltitudeHi: uExtent.SpatialVolume.AltitudeHi,
Cells: cells,
Manager: upsertParams.manager,
StartTime: upsertParams.uExtent.StartTime,
EndTime: upsertParams.uExtent.EndTime,
AltitudeLo: upsertParams.uExtent.SpatialVolume.AltitudeLo,
AltitudeHi: upsertParams.uExtent.SpatialVolume.AltitudeHi,
Cells: upsertParams.cells,
USSBaseURL: string(params.NewSubscription.UssBaseUrl),
NotifyForOperationalIntents: true,
ImplicitSubscription: true,
Expand All @@ -528,38 +565,38 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

} else {
// Use existing Subscription
sub, err = r.GetSubscription(ctx, subscriptionID)
sub, err = r.GetSubscription(ctx, upsertParams.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)
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", upsertParams.subscriptionID)
}
if sub.Manager != dssmodels.Manager(manager) {
if sub.Manager != dssmodels.Manager(upsertParams.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", upsertParams.subscriptionID, sub.Manager, upsertParams.manager)
}
updateSub := false
if sub.StartTime != nil && sub.StartTime.After(*uExtent.StartTime) {
if sub.StartTime != nil && sub.StartTime.After(*upsertParams.uExtent.StartTime) {
if sub.ImplicitSubscription {
sub.StartTime = uExtent.StartTime
sub.StartTime = upsertParams.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.EndTime != nil && sub.EndTime.Before(*upsertParams.uExtent.EndTime) {
if sub.ImplicitSubscription {
sub.EndTime = uExtent.EndTime
sub.EndTime = upsertParams.uExtent.EndTime
updateSub = true
} else {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription ends before the OperationalIntent ends")
}
}
if !sub.Cells.Contains(cells) {
if !sub.Cells.Contains(upsertParams.cells) {
if sub.ImplicitSubscription {
sub.Cells = s2.CellUnionFromUnion(sub.Cells, cells)
sub.Cells = s2.CellUnionFromUnion(sub.Cells, upsertParams.cells)
updateSub = true
} else {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not cover entire spatial area of the OperationalIntent")
Expand All @@ -573,7 +610,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
}
}

if state.RequiresKey() {
if upsertParams.state.RequiresKey() {
// Construct a hash set of OVNs as the key
key := map[scdmodels.OVN]bool{}
if params.Key != nil {
Expand All @@ -584,15 +621,15 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

// Identify OperationalIntents missing from the key
var missingOps []*scdmodels.OperationalIntent
relevantOps, err := r.SearchOperationalIntents(ctx, uExtent)
relevantOps, err := r.SearchOperationalIntents(ctx, upsertParams.uExtent)
if err != nil {
return stacktrace.Propagate(err, "Unable to SearchOperations")
}
for _, relevantOp := range relevantOps {
_, ok := key[relevantOp.OVN]
// Note: The OIR being mutated does not need to be specified in the key:
if !ok && relevantOp.RequiresKey() && relevantOp.ID != id {
if relevantOp.Manager != dssmodels.Manager(manager) {
if !ok && relevantOp.RequiresKey() && relevantOp.ID != upsertParams.id {
if relevantOp.Manager != dssmodels.Manager(upsertParams.manager) {
relevantOp.OVN = scdmodels.NoOvnPhrase
}
missingOps = append(missingOps, relevantOp)
Expand All @@ -602,13 +639,13 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
// Identify Constraints missing from the key
var missingConstraints []*scdmodels.Constraint
if sub != nil && sub.NotifyForConstraints {
constraints, err := r.SearchConstraints(ctx, uExtent)
constraints, err := r.SearchConstraints(ctx, upsertParams.uExtent)
if err != nil {
return stacktrace.Propagate(err, "Unable to SearchConstraints")
}
for _, relevantConstraint := range constraints {
if _, ok := key[relevantConstraint.OVN]; !ok {
if relevantConstraint.Manager != dssmodels.Manager(manager) {
if relevantConstraint.Manager != dssmodels.Manager(upsertParams.manager) {
relevantConstraint.OVN = scdmodels.NoOvnPhrase
}
missingConstraints = append(missingConstraints, relevantConstraint)
Expand All @@ -626,7 +663,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
responseConflict.MissingOperationalIntents = new([]restapi.OperationalIntentReference)
for _, missingOp := range missingOps {
p := missingOp.ToRest()
if missingOp.Manager != manager {
if missingOp.Manager != upsertParams.manager {
noOvnPhrase := restapi.EntityOVN(scdmodels.NoOvnPhrase)
p.Ovn = &noOvnPhrase
}
Expand All @@ -638,7 +675,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
responseConflict.MissingConstraints = new([]restapi.ConstraintReference)
for _, missingConstraint := range missingConstraints {
c := missingConstraint.ToRest()
if missingConstraint.Manager != manager {
if missingConstraint.Manager != upsertParams.manager {
noOvnPhrase := restapi.EntityOVN(scdmodels.NoOvnPhrase)
c.Ovn = &noOvnPhrase
}
Expand All @@ -660,19 +697,19 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

// Construct the new OperationalIntent
op := &scdmodels.OperationalIntent{
ID: id,
Manager: dssmodels.Manager(manager),
ID: upsertParams.id,
Manager: dssmodels.Manager(upsertParams.manager),
Version: scdmodels.VersionNumber(version + 1),

StartTime: uExtent.StartTime,
EndTime: uExtent.EndTime,
AltitudeLower: uExtent.SpatialVolume.AltitudeLo,
AltitudeUpper: uExtent.SpatialVolume.AltitudeHi,
Cells: cells,
StartTime: upsertParams.uExtent.StartTime,
EndTime: upsertParams.uExtent.EndTime,
AltitudeLower: upsertParams.uExtent.SpatialVolume.AltitudeLo,
AltitudeUpper: upsertParams.uExtent.SpatialVolume.AltitudeHi,
Cells: upsertParams.cells,

USSBaseURL: string(params.UssBaseUrl),
SubscriptionID: subID,
State: state,
State: upsertParams.state,
}
err = op.ValidateTimeRange()
if err != nil {
Expand All @@ -682,7 +719,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
// Compute total affected Volume4D for notification purposes
var notifyVol4 *dssmodels.Volume4D
if old == nil {
notifyVol4 = uExtent
notifyVol4 = upsertParams.uExtent
} else {
oldVol4 := &dssmodels.Volume4D{
StartTime: old.StartTime,
Expand All @@ -694,7 +731,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
return old.Cells, nil
}),
}}
notifyVol4, err = dssmodels.UnionVolumes4D(uExtent, oldVol4)
notifyVol4, err = dssmodels.UnionVolumes4D(upsertParams.uExtent, oldVol4)
if err != nil {
return stacktrace.Propagate(err, "Error constructing 4D volumes union")
}
Expand Down
Loading