review pass (#1210)

CHANGELOG.md

@@ -2,7 +2,11 @@
 
 ## v2.0.1
 
-FEATURE: Added several new `admin` API endpoints for interfacing with additional management controls: finding limit classes by label, finding applied/applying/removing limit classes from accounts, getting/setting skip interstitial status for an account (https://github.com/openziti/zrok/issues/726)
+FEATURE: Added several new `admin` API endpoints for interfacing with additional management controls: finding limit classes by label, finding applied/applying/removing limit classes from accounts, getting/setting skip interstitial status for an account (https://github.com/openziti/zrok/issues/1210)
+
+CHANGE: Removed the legacy `admin` `/grants` endpoint. Its prior synchronization behavior is now replaced by the new skip interstitial grant management endpoints. (https://github.com/openziti/zrok/issues/726)
+
+CHANGE: Applying limit classes now validates requested assignments to prevent conflicting effective limit class combinations on an account (https://github.com/openziti/zrok/issues/726)
 
 ## v2.0.0
 

controller/applyLimitClasses.go

@@ -6,6 +6,7 @@ import (
 	"github.com/openziti/zrok/v2/controller/store"
 	"github.com/openziti/zrok/v2/rest_model_zrok"
 	"github.com/openziti/zrok/v2/rest_server_zrok/operations/admin"
+	"github.com/openziti/zrok/v2/sdk/golang/sdk"
 )
 
 type applyLimitClassesHandler struct{}
@@ -33,13 +34,49 @@ func (h *applyLimitClassesHandler) Handle(params admin.ApplyLimitClassesParams,
 		return admin.NewApplyLimitClassesNotFound()
 	}
 
+	existingLcs, err := str.FindAppliedLimitClassesForAccount(acct.Id, trx)
+	if err != nil {
+		dl.Errorf("error finding applied limit classes for '%v': %v", params.Body.Email, err)
+		return admin.NewApplyLimitClassesInternalServerError()
+	}
+
+	seenIds := make(map[int]bool)
+	for _, lc := range existingLcs {
+		seenIds[lc.Id] = true
+	}
+
+	occupiedSlots := newLimitClassSlots()
+	for _, lc := range existingLcs {
+		if conflict := occupiedSlots.add(lc); conflict {
+			dl.Errorf("existing applied limit classes for '%v' are already in a conflicting state", params.Body.Email)
+			return admin.NewApplyLimitClassesInternalServerError()
+		}
+	}
+
+	var toApply []*store.AppliedLimitClass
 	for _, lcId := range params.Body.LimitClassIds {
-		if _, err := str.GetLimitClass(int(lcId), trx); err != nil {
+		if seenIds[int(lcId)] {
+			continue
+		}
+
+		lc, err := str.GetLimitClass(int(lcId), trx)
+		if err != nil {
 			dl.Errorf("error finding limit class '%v': %v", lcId, err)
 			return admin.NewApplyLimitClassesNotFound()
 		}
-		if _, err := str.ApplyLimitClass(&store.AppliedLimitClass{AccountId: acct.Id, LimitClassId: int(lcId)}, trx); err != nil {
-			dl.Errorf("error applying limit class '%v' to '%v': %v", lcId, params.Body.Email, err)
+
+		if conflict := occupiedSlots.add(lc); conflict {
+			dl.Errorf("applying limit class '%v' to '%v' would create conflicting effective limits", lcId, params.Body.Email)
+			return admin.NewApplyLimitClassesInternalServerError()
+		}
+
+		seenIds[lc.Id] = true
+		toApply = append(toApply, &store.AppliedLimitClass{AccountId: acct.Id, LimitClassId: lc.Id})
+	}
+
+	for _, applied := range toApply {
+		if _, err := str.ApplyLimitClass(applied, trx); err != nil {
+			dl.Errorf("error applying limit class '%v' to '%v': %v", applied.LimitClassId, params.Body.Email, err)
 			return admin.NewApplyLimitClassesInternalServerError()
 		}
 	}
@@ -51,3 +88,55 @@ func (h *applyLimitClassesHandler) Handle(params admin.ApplyLimitClassesParams,
 
 	return admin.NewApplyLimitClassesOK()
 }
+
+type limitClassSlots struct {
+	resource  *store.LimitClass
+	bwWarning *store.LimitClass
+	bwLimit   *store.LimitClass
+	scopes    map[sdk.BackendMode]*store.LimitClass
+}
+
+func newLimitClassSlots() *limitClassSlots {
+	return &limitClassSlots{
+		scopes: make(map[sdk.BackendMode]*store.LimitClass),
+	}
+}
+
+func (slots *limitClassSlots) add(lc *store.LimitClass) bool {
+	if lc == nil {
+		return false
+	}
+
+	if lc.IsResourceCountClass() {
+		if slots.resource != nil && slots.resource.Id != lc.Id {
+			return true
+		}
+		slots.resource = lc
+		return false
+	}
+
+	if lc.IsUnscopedBandwidthClass() {
+		if lc.LimitAction == store.WarningLimitAction {
+			if slots.bwWarning != nil && slots.bwWarning.Id != lc.Id {
+				return true
+			}
+			slots.bwWarning = lc
+			return false
+		}
+		if slots.bwLimit != nil && slots.bwLimit.Id != lc.Id {
+			return true
+		}
+		slots.bwLimit = lc
+		return false
+	}
+
+	if lc.IsScopedBandwidthClass() {
+		backendMode := *lc.BackendMode
+		if existing, found := slots.scopes[backendMode]; found && existing.Id != lc.Id {
+			return true
+		}
+		slots.scopes[backendMode] = lc
+	}
+
+	return false
+}

controller/limits/userLimits.go

@@ -56,15 +56,15 @@ func (a *Agent) getUserLimits(acctId int, trx *sqlx.Tx) (*userLimits, error) {
 		return nil, errors.Wrapf(err, "error finding applied limit classes for account '%d'", acctId)
 	}
 	for _, alc := range alcs {
-		if a.isResourceCountClass(alc) {
+		if alc.IsResourceCountClass() {
 			resource = alc
-		} else if a.isUnscopedBandwidthClass(alc) {
+		} else if alc.IsUnscopedBandwidthClass() {
 			if alc.LimitAction == store.WarningLimitAction {
 				bwWarning = alc
 			} else {
 				bwLimit = alc
 			}
-		} else if a.isScopedLimitClass(alc) {
+		} else if alc.IsScopedBandwidthClass() {
 			scopes[*alc.BackendMode] = alc
 		} else {
 			dl.Warnf("unknown type of limit class '%v' for account '#%d'", alc, acctId)
@@ -79,45 +79,3 @@ func (a *Agent) getUserLimits(acctId int, trx *sqlx.Tx) (*userLimits, error) {
 
 	return userLimits, nil
 }
-
-func (a *Agent) isResourceCountClass(alc *store.LimitClass) bool {
-	if alc.BackendMode != nil {
-		return false
-	}
-	if alc.Environments == store.Unlimited && alc.Shares == store.Unlimited && alc.ReservedShares == store.Unlimited && alc.UniqueNames == store.Unlimited && alc.ShareFrontends == store.Unlimited {
-		return false
-	}
-	return true
-}
-
-func (a *Agent) isUnscopedBandwidthClass(alc *store.LimitClass) bool {
-	if alc.BackendMode != nil {
-		return false
-	}
-	if alc.Environments > store.Unlimited || alc.Shares > store.Unlimited || alc.ReservedShares > store.Unlimited || alc.UniqueNames > store.Unlimited || alc.ShareFrontends > store.Unlimited {
-		return false
-	}
-	if alc.PeriodMinutes < 1 {
-		return false
-	}
-	if alc.RxBytes == store.Unlimited && alc.TxBytes == store.Unlimited && alc.TotalBytes == store.Unlimited {
-		return false
-	}
-	return true
-}
-
-func (a *Agent) isScopedLimitClass(alc *store.LimitClass) bool {
-	if alc.BackendMode == nil {
-		return false
-	}
-	if alc.Environments > store.Unlimited {
-		return false
-	}
-	if alc.PeriodMinutes < 1 {
-		return false
-	}
-	if alc.RxBytes == store.Unlimited && alc.TxBytes == store.Unlimited && alc.TotalBytes == store.Unlimited {
-		return false
-	}
-	return true
-}

controller/limits/userLimits_simple_test.go

@@ -72,24 +72,20 @@ func TestUserLimits_ToBandwidthArraySimple(t *testing.T) {
 	}
 }
 
-func TestAgent_ClassificationMethods(t *testing.T) {
-	a := &Agent{
-		cfg: DefaultConfig(),
-	}
-
+func TestLimitClass_ClassificationMethods(t *testing.T) {
 	t.Run("isResourceCountClass", func(t *testing.T) {
 		validRC := &store.LimitClass{
 			Model:        store.Model{Id: 1},
 			Environments: 10,
 		}
-		assert.True(t, a.isResourceCountClass(validRC))
+		assert.True(t, validRC.IsResourceCountClass())
 
 		proxyMode := sdk.ProxyBackendMode
 		invalidRC := &store.LimitClass{
 			Model:       store.Model{Id: 2},
 			BackendMode: &proxyMode,
 		}
-		assert.False(t, a.isResourceCountClass(invalidRC))
+		assert.False(t, invalidRC.IsResourceCountClass())
 	})
 
 	t.Run("isUnscopedBandwidthClass", func(t *testing.T) {
@@ -103,7 +99,7 @@ func TestAgent_ClassificationMethods(t *testing.T) {
 			UniqueNames:    store.Unlimited,
 			ShareFrontends: store.Unlimited,
 		}
-		assert.True(t, a.isUnscopedBandwidthClass(validBwc))
+		assert.True(t, validBwc.IsUnscopedBandwidthClass())
 
 		proxyMode := sdk.ProxyBackendMode
 		invalidBwc := &store.LimitClass{
@@ -112,7 +108,7 @@ func TestAgent_ClassificationMethods(t *testing.T) {
 			PeriodMinutes: 60,
 			RxBytes:       1024,
 		}
-		assert.False(t, a.isUnscopedBandwidthClass(invalidBwc))
+		assert.False(t, invalidBwc.IsUnscopedBandwidthClass())
 	})
 
 	t.Run("isScopedLimitClass", func(t *testing.T) {
@@ -128,18 +124,18 @@ func TestAgent_ClassificationMethods(t *testing.T) {
 			UniqueNames:    store.Unlimited,
 			ShareFrontends: store.Unlimited,
 		}
-		assert.True(t, a.isScopedLimitClass(validSLC))
+		assert.True(t, validSLC.IsScopedBandwidthClass())
 
 		invalidSLC := &store.LimitClass{
 			Model:         store.Model{Id: 2},
 			PeriodMinutes: 60,
 			RxBytes:       1024,
 		}
-		assert.False(t, a.isScopedLimitClass(invalidSLC))
+		assert.False(t, invalidSLC.IsScopedBandwidthClass())
 	})
 }
 
 // helper function to get string pointer
 func strPtr(s string) *string {
 	return &s
-}
+}

controller/skipInterstitialGrant.go

@@ -81,13 +81,13 @@ func (h *grantSkipInterstitialHandler) Handle(params admin.GrantSkipInterstitial
 		return admin.NewGrantSkipInterstitialInternalServerError()
 	}
 
-	if err := trx.Commit(); err != nil {
-		dl.Errorf("error committing transaction: %v", err)
+	if err := syncSkipInterstitialForAccount(acct, true); err != nil {
+		dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err)
 		return admin.NewGrantSkipInterstitialInternalServerError()
 	}
 
-	if err := syncSkipInterstitialForAccount(acct, true); err != nil {
-		dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err)
+	if err := trx.Commit(); err != nil {
+		dl.Errorf("error committing transaction: %v", err)
 		return admin.NewGrantSkipInterstitialInternalServerError()
 	}
 
@@ -126,21 +126,23 @@ func (h *revokeSkipInterstitialHandler) Handle(params admin.RevokeSkipInterstiti
 		return admin.NewRevokeSkipInterstitialInternalServerError()
 	}
 
-	if err := trx.Commit(); err != nil {
-		dl.Errorf("error committing transaction: %v", err)
+	if err := syncSkipInterstitialForAccount(acct, false); err != nil {
+		dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err)
 		return admin.NewRevokeSkipInterstitialInternalServerError()
 	}
 
-	if err := syncSkipInterstitialForAccount(acct, false); err != nil {
-		dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err)
+	if err := trx.Commit(); err != nil {
+		dl.Errorf("error committing transaction: %v", err)
 		return admin.NewRevokeSkipInterstitialInternalServerError()
 	}
 
 	return admin.NewRevokeSkipInterstitialOK()
 }
 
-// syncSkipInterstitialForAccount synchronizes the interstitial setting on all
-// public (non-drive) share Ziti configs for the given account.
+// syncSkipInterstitialForAccount best-effort synchronizes the interstitial
+// setting on existing public (non-drive) share Ziti configs for the given
+// account. Failures while processing individual shares are logged and skipped,
+// since affected shares can be recreated to correct transient issues.
 func syncSkipInterstitialForAccount(acct *store.Account, skipInterstitial bool) error {
 	trx, err := str.Begin()
 	if err != nil {

controller/store/limitClass.go

@@ -111,6 +111,48 @@ func (lc LimitClass) GetLimitAction() LimitAction {
 	return lc.LimitAction
 }
 
+func (lc LimitClass) IsResourceCountClass() bool {
+	if lc.BackendMode != nil {
+		return false
+	}
+	if lc.Environments == Unlimited && lc.Shares == Unlimited && lc.ReservedShares == Unlimited && lc.UniqueNames == Unlimited && lc.ShareFrontends == Unlimited {
+		return false
+	}
+	return true
+}
+
+func (lc LimitClass) IsUnscopedBandwidthClass() bool {
+	if lc.BackendMode != nil {
+		return false
+	}
+	if lc.Environments > Unlimited || lc.Shares > Unlimited || lc.ReservedShares > Unlimited || lc.UniqueNames > Unlimited || lc.ShareFrontends > Unlimited {
+		return false
+	}
+	if lc.PeriodMinutes < 1 {
+		return false
+	}
+	if lc.RxBytes == Unlimited && lc.TxBytes == Unlimited && lc.TotalBytes == Unlimited {
+		return false
+	}
+	return true
+}
+
+func (lc LimitClass) IsScopedBandwidthClass() bool {
+	if lc.BackendMode == nil {
+		return false
+	}
+	if lc.Environments > Unlimited {
+		return false
+	}
+	if lc.PeriodMinutes < 1 {
+		return false
+	}
+	if lc.RxBytes == Unlimited && lc.TxBytes == Unlimited && lc.TotalBytes == Unlimited {
+		return false
+	}
+	return true
+}
+
 func (lc LimitClass) String() string {
 	out := "LimitClass<"
 	if lc.Label != nil && *lc.Label != "" {

controller/store/skipInterstitialGrant.go

@@ -18,6 +18,14 @@ func (str *Store) IsAccountGrantedSkipInterstitial(acctId int, trx *sqlx.Tx) (bo
 }
 
 func (str *Store) GrantSkipInterstitial(acctId int, trx *sqlx.Tx) error {
+	granted, err := str.IsAccountGrantedSkipInterstitial(acctId, trx)
+	if err != nil {
+		return err
+	}
+	if granted {
+		return nil
+	}
+
 	stmt, err := trx.Prepare("insert into skip_interstitial_grants (account_id) values ($1)")
 	if err != nil {
 		return errors.Wrap(err, "error preparing skip_interstitial_grants insert statement")