From 498a17b3abec1021248b4c2b084dba9f4ef57b36 Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Fri, 30 Aug 2024 13:47:28 +0200 Subject: [PATCH 1/2] [scd] factor out parameter validation against previous OIR --- pkg/scd/operational_intents_handler.go | 56 +++++++++++++++++--------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index 1e41ca5af..ffdbdfcca 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -478,6 +478,36 @@ func checkUpsertPermissionsAndReturnManager(authorizedManager *api.Authorization return dssmodels.Manager(*authorizedManager.ClientID), nil } +// validateUpsertRequestAgainstPreviousOIRAndReturnVersion checks that the client requesting an OIR upsert has the necessary permissions and that the request is valid. +// On success, the version of the OIR is returned: +// - upon initial creation (if no previous OIR exists), it is 0 +// - otherwise, it is the version of the previous OIR +func validateUpsertRequestAgainstPreviousOIRAndReturnVersion( + requestingManager dssmodels.Manager, + providedOVN restapi.EntityOVN, + previousOIR *scdmodels.OperationalIntent, +) (int32, error) { + + if previousOIR != nil { + if previousOIR.Manager != requestingManager { + return 0, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, + "OperationalIntent owned by %s, but %s attempted to modify", previousOIR.Manager, requestingManager) + } + if previousOIR.OVN != scdmodels.OVN(providedOVN) { + return 0, stacktrace.NewErrorWithCode(dsserr.VersionMismatch, + "Current version is %s but client specified version %s", previousOIR.OVN, providedOVN) + } + + return int32(previousOIR.Version), nil + } + + if providedOVN != "" { + return 0, stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", providedOVN) + } + + return 0, 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, @@ -496,8 +526,6 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize var responseOK *restapi.ChangeOperationalIntentReferenceResponse var responseConflict *restapi.AirspaceConflictResponse action := func(ctx context.Context, r repos.Repository) (err error) { - var version int32 // Version of the Operational Intent (0 means creation requested). - // Lock subscriptions based on the cell to reduce the number of retries under concurrent load. // See issue #1002 for details. err = r.LockSubscriptionsOnCells(ctx, validParams.cells) @@ -505,28 +533,16 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize return stacktrace.Propagate(err, "Unable to acquire lock") } - // Get existing OperationalIntent, if any, and validate request + // Get existing OperationalIntent, if any old, err := r.GetOperationalIntent(ctx, validParams.id) if err != nil { return stacktrace.Propagate(err, "Could not get OperationalIntent from repo") } - if old != nil { - if old.Manager != manager { - return stacktrace.NewErrorWithCode(dsserr.PermissionDenied, - "OperationalIntent owned by %s, but %s attempted to modify", old.Manager, manager) - } - if old.OVN != scdmodels.OVN(ovn) { - return stacktrace.NewErrorWithCode(dsserr.VersionMismatch, - "Current version is %s but client specified version %s", old.OVN, ovn) - } - - version = int32(old.Version) - } else { - if ovn != "" { - return stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", ovn) - } - - version = 0 + // Validate the request against the previous OIR and return the current version + // (upon new OIR creation, version is 0) + version, err := validateUpsertRequestAgainstPreviousOIRAndReturnVersion(manager, validParams.ovn, old) + if err != nil { + return stacktrace.PropagateWithCode(err, stacktrace.GetCode(err), "Request validation failed") } var sub *scdmodels.Subscription From 329993c3ba9b1a9cc7feeefaaa68d011dc787a9c Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Fri, 30 Aug 2024 16:28:16 +0200 Subject: [PATCH 2/2] comment --- pkg/scd/operational_intents_handler.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index ffdbdfcca..8a7039f13 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -478,34 +478,34 @@ func checkUpsertPermissionsAndReturnManager(authorizedManager *api.Authorization return dssmodels.Manager(*authorizedManager.ClientID), nil } -// validateUpsertRequestAgainstPreviousOIRAndReturnVersion checks that the client requesting an OIR upsert has the necessary permissions and that the request is valid. +// validateUpsertRequestAgainstPreviousOIR checks that the client requesting an OIR upsert has the necessary permissions and that the request is valid. // On success, the version of the OIR is returned: // - upon initial creation (if no previous OIR exists), it is 0 // - otherwise, it is the version of the previous OIR -func validateUpsertRequestAgainstPreviousOIRAndReturnVersion( +func validateUpsertRequestAgainstPreviousOIR( requestingManager dssmodels.Manager, providedOVN restapi.EntityOVN, previousOIR *scdmodels.OperationalIntent, -) (int32, error) { +) error { if previousOIR != nil { if previousOIR.Manager != requestingManager { - return 0, stacktrace.NewErrorWithCode(dsserr.PermissionDenied, + return stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "OperationalIntent owned by %s, but %s attempted to modify", previousOIR.Manager, requestingManager) } if previousOIR.OVN != scdmodels.OVN(providedOVN) { - return 0, stacktrace.NewErrorWithCode(dsserr.VersionMismatch, + return stacktrace.NewErrorWithCode(dsserr.VersionMismatch, "Current version is %s but client specified version %s", previousOIR.OVN, providedOVN) } - return int32(previousOIR.Version), nil + return nil } if providedOVN != "" { - return 0, stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", providedOVN) + return stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", providedOVN) } - return 0, nil + return nil } // upsertOperationalIntentReference inserts or updates an Operational Intent. @@ -540,11 +540,16 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } // Validate the request against the previous OIR and return the current version // (upon new OIR creation, version is 0) - version, err := validateUpsertRequestAgainstPreviousOIRAndReturnVersion(manager, validParams.ovn, old) - if err != nil { + if err := validateUpsertRequestAgainstPreviousOIR(manager, validParams.ovn, old); err != nil { return stacktrace.PropagateWithCode(err, stacktrace.GetCode(err), "Request validation failed") } + // For an OIR being created, version starts at 0 + version := int32(0) + if old != nil { + version = int32(old.Version) + } + var sub *scdmodels.Subscription if validParams.subscriptionID.Empty() { // Create an implicit subscription if the implicit subscription params are set: