fix: site replication issues and add tests (#13861)

- deleting policies was deleting all LDAP user mapping, this was a regression introduced in #13567 - deleting of policies is properly sent across all sites. - remove unexpected errors instead embed the real errors as part of the 500 error response.
2025-11-07 04:42:56 -05:00 · 2021-12-08 11:50:15 -08:00
parent 92fdcafb66
commit e82a5c5c54
7 changed files with 215 additions and 49 deletions
--- a/cmd/admin-handlers-site-replication.go
+++ b/cmd/admin-handlers-site-replication.go
@@ -113,6 +113,8 @@ func (a adminAPIHandlers) SRInternalBucketOps(w http.ResponseWriter, r *http.Req

 	var err error
 	switch operation {
+	default:
+		err = errInvalidArgument
 	case madmin.MakeWithVersioningBktOp:
 		_, isLockEnabled := r.Form["lockEnabled"]
 		_, isVersioningEnabled := r.Form["versioningEnabled"]
@@ -128,13 +130,10 @@ func (a adminAPIHandlers) SRInternalBucketOps(w http.ResponseWriter, r *http.Req
 		err = globalSiteReplicationSys.PeerBucketDeleteHandler(ctx, bucket, false)
 	case madmin.ForceDeleteBucketBktOp:
 		err = globalSiteReplicationSys.PeerBucketDeleteHandler(ctx, bucket, true)
-	default:
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL)
-		return
 	}
 	if err != nil {
 		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL)
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
 	}

@@ -160,37 +159,40 @@ func (a adminAPIHandlers) SRInternalReplicateIAMItem(w http.ResponseWriter, r *h

 	var err error
 	switch item.Type {
+	default:
+		err = errInvalidArgument
 	case madmin.SRIAMItemPolicy:
-		var policy *iampolicy.Policy
-		if len(item.Policy) > 0 {
-			policy, err = iampolicy.ParseConfig(bytes.NewReader(item.Policy))
-			if err != nil {
-				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		if item.Policy == nil {
+			err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, nil)
+		} else {
+			policy, perr := iampolicy.ParseConfig(bytes.NewReader(item.Policy))
+			if perr != nil {
+				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, perr), r.URL)
 				return
 			}
+			if policy.IsEmpty() {
+				err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, nil)
+			} else {
+				err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, policy)
+			}
 		}
-		err = globalSiteReplicationSys.PeerAddPolicyHandler(ctx, item.Name, policy)
 	case madmin.SRIAMItemSvcAcc:
-		err = globalSiteReplicationSys.PeerSvcAccChangeHandler(ctx, *item.SvcAccChange)
+		err = globalSiteReplicationSys.PeerSvcAccChangeHandler(ctx, item.SvcAccChange)
 	case madmin.SRIAMItemPolicyMapping:
-		err = globalSiteReplicationSys.PeerPolicyMappingHandler(ctx, *item.PolicyMapping)
+		err = globalSiteReplicationSys.PeerPolicyMappingHandler(ctx, item.PolicyMapping)
 	case madmin.SRIAMItemSTSAcc:
-		err = globalSiteReplicationSys.PeerSTSAccHandler(ctx, *item.STSCredential)
-
-	default:
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL)
-		return
+		err = globalSiteReplicationSys.PeerSTSAccHandler(ctx, item.STSCredential)
 	}
 	if err != nil {
 		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL)
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
 	}
 }

 // SRInternalReplicateBucketItem - PUT /minio/admin/v3/site-replication/bucket-meta
 func (a adminAPIHandlers) SRInternalReplicateBucketItem(w http.ResponseWriter, r *http.Request) {
-	ctx := newContext(r, w, "SRInternalReplicateIAMItem")
+	ctx := newContext(r, w, "SRInternalReplicateBucketItem")

 	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))

@@ -208,30 +210,33 @@ func (a adminAPIHandlers) SRInternalReplicateBucketItem(w http.ResponseWriter, r

 	var err error
 	switch item.Type {
+	default:
+		err = errInvalidArgument
 	case madmin.SRBucketMetaTypePolicy:
-		var bktPolicy *policy.Policy
-		if len(item.Policy) > 0 {
-			bktPolicy, err = policy.ParseConfig(bytes.NewReader(item.Policy), item.Bucket)
-			if err != nil {
-				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		if item.Policy == nil {
+			err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, nil)
+		} else {
+			bktPolicy, berr := policy.ParseConfig(bytes.NewReader(item.Policy), item.Bucket)
+			if berr != nil {
+				writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, berr), r.URL)
 				return
 			}
+			if bktPolicy.IsEmpty() {
+				err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, nil)
+			} else {
+				err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, bktPolicy)
+			}
 		}
-		err = globalSiteReplicationSys.PeerBucketPolicyHandler(ctx, item.Bucket, bktPolicy)
 	case madmin.SRBucketMetaTypeTags:
 		err = globalSiteReplicationSys.PeerBucketTaggingHandler(ctx, item.Bucket, item.Tags)
 	case madmin.SRBucketMetaTypeObjectLockConfig:
 		err = globalSiteReplicationSys.PeerBucketObjectLockConfigHandler(ctx, item.Bucket, item.ObjectLockConfig)
 	case madmin.SRBucketMetaTypeSSEConfig:
 		err = globalSiteReplicationSys.PeerBucketSSEConfigHandler(ctx, item.Bucket, item.SSEConfig)
-
-	default:
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminInvalidArgument), r.URL)
-		return
 	}
 	if err != nil {
 		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrInternalError), r.URL)
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
 	}
 }
--- a/cmd/iam-store.go
+++ b/cmd/iam-store.go
@@ -846,12 +846,14 @@ func (store *IAMStoreSys) PolicyNotificationHandler(ctx context.Context, policy
 			if !pset.Contains(policy) {
 				continue
 			}
-			_, ok := cache.iamUsersMap[u]
-			if !ok {
-				// happens when account is deleted or
-				// expired.
-				delete(cache.iamUserPolicyMap, u)
-				continue
+			if store.getUsersSysType() == MinIOUsersSysType {
+				_, ok := cache.iamUsersMap[u]
+				if !ok {
+					// happens when account is deleted or
+					// expired.
+					delete(cache.iamUserPolicyMap, u)
+					continue
+				}
 			}
 			pset.Remove(policy)
 			cache.iamUserPolicyMap[u] = newMappedPolicy(strings.Join(pset.ToSlice(), ","))
@@ -886,11 +888,13 @@ func (store *IAMStoreSys) DeletePolicy(ctx context.Context, policy string) error
 	groups := []string{}
 	for u, mp := range cache.iamUserPolicyMap {
 		pset := mp.policySet()
-		if _, ok := cache.iamUsersMap[u]; !ok {
-			// This case can happen when a temporary account is
-			// deleted or expired - remove it from userPolicyMap.
-			delete(cache.iamUserPolicyMap, u)
-			continue
+		if store.getUsersSysType() == MinIOUsersSysType {
+			if _, ok := cache.iamUsersMap[u]; !ok {
+				// This case can happen when a temporary account is
+				// deleted or expired - remove it from userPolicyMap.
+				delete(cache.iamUserPolicyMap, u)
+				continue
+			}
 		}
 		if pset.Contains(policy) {
 			users = append(users, u)
--- a/cmd/site-replication.go
+++ b/cmd/site-replication.go
@@ -48,8 +48,7 @@ import (

 const (
 	srStatePrefix = minioConfigPrefix + "/site-replication"
-
-	srStateFile = "state.json"
+	srStateFile   = "state.json"
 )

 const (
@@ -1007,7 +1006,10 @@ func (c *SiteReplicationSys) PeerAddPolicyHandler(ctx context.Context, policyNam
 }

 // PeerSvcAccChangeHandler - copies service-account change to local.
-func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change madmin.SRSvcAccChange) error {
+func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change *madmin.SRSvcAccChange) error {
+	if change == nil {
+		return errInvalidArgument
+	}
 	switch {
 	case change.Create != nil:
 		var sp *iampolicy.Policy
@@ -1069,7 +1071,10 @@ func (c *SiteReplicationSys) PeerSvcAccChangeHandler(ctx context.Context, change
 }

 // PeerPolicyMappingHandler - copies policy mapping to local.
-func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mapping madmin.SRPolicyMapping) error {
+func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mapping *madmin.SRPolicyMapping) error {
+	if mapping == nil {
+		return errInvalidArgument
+	}
 	err := globalIAMSys.PolicyDBSet(ctx, mapping.UserOrGroup, mapping.Policy, mapping.IsGroup)
 	if err != nil {
 		return wrapSRErr(err)
@@ -1078,7 +1083,11 @@ func (c *SiteReplicationSys) PeerPolicyMappingHandler(ctx context.Context, mappi
 }

 // PeerSTSAccHandler - replicates STS credential locally.
-func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred madmin.SRSTSCredential) error {
+func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred *madmin.SRSTSCredential) error {
+	if stsCred == nil {
+		return errInvalidArgument
+	}
+
 	// Verify the session token of the stsCred
 	claims, err := auth.ExtractClaims(stsCred.SessionToken, globalActiveCred.SecretKey)
 	if err != nil {
@@ -1089,13 +1098,13 @@ func (c *SiteReplicationSys) PeerSTSAccHandler(ctx context.Context, stsCred madm
 	mapClaims := claims.Map()
 	expiry, err := auth.ExpToInt64(mapClaims["exp"])
 	if err != nil {
-		return fmt.Errorf("Expiry claim was not found")
+		return fmt.Errorf("Expiry claim was not found: %v", mapClaims)
 	}

 	// Extract the username and lookup DN and groups in LDAP.
 	ldapUser, ok := claims.Lookup(ldapUserN)
 	if !ok {
-		return fmt.Errorf("Could not find LDAP username in claims")
+		return fmt.Errorf("Could not find LDAP username in claims: %v", mapClaims)
 	}

 	// Need to lookup the groups from LDAP.