Automatic merge from release/2.1.0 -> master

* commit '815ec7f94f1359dd5a176efe5cbfcff1a6af8216':
  更改sync算法
  同步阿里云安全组规则到本地

cmd/climc/shell/secgrouprules.go

@@ -59,7 +59,6 @@ func init() {
 	})
 
 	type SecGroupRulesCreateOptions struct {
-		NAME      string `help:"Name of security group rule to create"`
 		SECGROUP  string `help:"Secgroup ID or Name" metavar:"Secgroup"`
 		Direction string `help:"Direction of rule" choices:"in|out"`
 		Action    string `help:"Action of rule" choices:"allow|deny"`
@@ -72,7 +71,6 @@ func init() {
 
 	R(&SecGroupRulesCreateOptions{}, "secgroup-rule-create", "Create all security group rule", func(s *mcclient.ClientSession, args *SecGroupRulesCreateOptions) error {
 		params := jsonutils.NewDict()
-		params.Add(jsonutils.NewString(args.NAME), "name")
 		if len(args.Desc) > 0 {
 			params.Add(jsonutils.NewString(args.Desc), "description")
 		}

pkg/cloudprovider/resources.go

@@ -5,6 +5,7 @@ import (
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/onecloud/pkg/mcclient"
+	"yunion.io/x/pkg/util/secrules"
 )
 
 type ICloudResource interface {
@@ -143,6 +144,7 @@ type ICloudVM interface {
 	GetBios() string
 	GetMachine() string
 
+	SyncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) error
 	GetHypervisor() string
 
 	// GetSecurityGroup() ICloudSecurityGroup
@@ -169,6 +171,8 @@ type ICloudEIP interface {
 
 type ICloudSecurityGroup interface {
 	ICloudResource
+	GetDescription() string
+	GetRules() ([]secrules.SecurityRule, error)
 }
 
 type ICloudDisk interface {
@@ -200,6 +204,7 @@ type ICloudVpc interface {
 	GetCidrBlock() string
 	// GetStatus() string
 	GetIWires() ([]ICloudWire, error)
+	GetISecurityGroups() ([]ICloudSecurityGroup, error)
 
 	GetManagerId() string
 

pkg/compute/guestdrivers/aliyun.go

@@ -170,6 +170,13 @@ func (self *SAliyunGuestDriver) RequestDeployGuestOnHost(ctx context.Context, gu
 			return nil, err
 		}
 
+		if len(guest.SecgrpId) > 0 {
+			if err := iVM.SyncSecurityGroup(guest.SecgrpId, guest.GetSecgroupName(), guest.GetSecRules()); err != nil {
+				log.Errorf("SyncSecurityGroup error: %v", err)
+				return nil, err
+			}
+		}
+
 		if onfinish == "none" {
 			err = iVM.StartVM()
 			if err != nil {
@@ -256,5 +263,17 @@ func (self *SAliyunGuestDriver) OnGuestDeployTaskDataReceived(ctx context.Contex
 }
 
 func (self *SAliyunGuestDriver) RequestSyncConfigOnHost(ctx context.Context, guest *models.SGuest, host *models.SHost, task taskman.ITask) error {
+	taskman.LocalTaskRun(task, func() (jsonutils.JSONObject, error) {
+		if fw_only, _ := task.GetParams().Bool("fw_only"); fw_only {
+			if ihost, err := host.GetIHost(); err != nil {
+				return nil, err
+			} else if iVM, err := ihost.GetIVMById(guest.ExternalId); err != nil {
+				return nil, err
+			} else if err := iVM.SyncSecurityGroup(guest.SecgrpId, guest.GetSecgroupName(), guest.GetSecRules()); err != nil {
+				return nil, err
+			}
+		}
+		return nil, nil
+	})
 	return nil
 }

pkg/compute/models/guests.go

@@ -18,6 +18,7 @@ import (
 	"yunion.io/x/pkg/util/netutils"
 	"yunion.io/x/pkg/util/osprofile"
 	"yunion.io/x/pkg/util/regutils"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/util/sysutils"
 	"yunion.io/x/pkg/util/timeutils"
 	"yunion.io/x/pkg/utils"
@@ -910,7 +911,7 @@ func (self *SGuest) GetCustomizeColumns(ctx context.Context, userCred mcclient.T
 		extra.Add(jsonutils.NewString(zone.Id), "zone_id")
 		extra.Add(jsonutils.NewString(zone.Name), "zone")
 	}
-	extra.Add(jsonutils.NewString(self.getSecgroupName()), "secgroup")
+	extra.Add(jsonutils.NewString(self.GetSecgroupName()), "secgroup")
 
 	if self.PendingDeleted {
 		pendingDeletedAt := self.PendingDeletedAt.Add(time.Second * time.Duration(options.Options.PendingDeleteExpireSeconds))
@@ -932,7 +933,7 @@ func (self *SGuest) GetExtraDetails(ctx context.Context, userCred mcclient.Token
 	}
 	// extra.Add(jsonutils.NewString(self.getFlavorName()), "flavor")
 	extra.Add(jsonutils.NewString(self.getKeypairName()), "keypair")
-	extra.Add(jsonutils.NewString(self.getSecgroupName()), "secgroup")
+	extra.Add(jsonutils.NewString(self.GetSecgroupName()), "secgroup")
 	extra.Add(jsonutils.NewString(strings.Join(self.getIPs(), ",")), "ips")
 	extra.Add(jsonutils.NewString(self.getSecurityRules()), "security_rules")
 	extra.Add(jsonutils.NewString(self.getIsolatedDeviceDetails()), "isolated_devices")
@@ -1129,7 +1130,7 @@ func (self *SGuest) getAdminSecgroup() *SSecurityGroup {
 	return SecurityGroupManager.FetchSecgroupById(self.AdminSecgrpId)
 }
 
-func (self *SGuest) getSecgroupName() string {
+func (self *SGuest) GetSecgroupName() string {
 	secgrp := self.getSecgroup()
 	if secgrp != nil {
 		return secgrp.GetName()
@@ -1145,6 +1146,22 @@ func (self *SGuest) getAdminSecgroupName() string {
 	return ""
 }
 
+func (self *SGuest) GetSecRules() []secrules.SecurityRule {
+	return self.getSecRules()
+}
+
+func (self *SGuest) getSecRules() []secrules.SecurityRule {
+	if secgrp := self.getSecgroup(); secgrp != nil {
+		return secgrp.getSecRules()
+	}
+	if rule, err := secrules.ParseSecurityRule(options.Options.DefaultSecurityRules); err == nil {
+		return []secrules.SecurityRule{*rule}
+	} else {
+		log.Errorf("Default SecurityRules error: %v", err)
+	}
+	return []secrules.SecurityRule{}
+}
+
 func (self *SGuest) getSecurityRules() string {
 	secgrp := self.getSecgroup()
 	if secgrp != nil {
@@ -2128,6 +2145,54 @@ func (self *SGuest) StartDeleteGuestTask(ctx context.Context, userCred mcclient.
 	return self.GetDriver().StartDeleteGuestTask(ctx, userCred, self, params, parentTaskId)
 }
 
+func (self *SGuest) AllowPerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
+	return self.IsOwner(userCred)
+}
+
+func (self *SGuest) AllowPerformRevokeSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
+	return self.IsOwner(userCred)
+}
+
+func (self *SGuest) PerformRevokeSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+	if !utils.IsInStringArray(self.Status, []string{VM_READY, VM_RUNNING, VM_SUSPEND}) {
+		return nil, httperrors.NewInputParameterError("Cannot revoke security rules in status %s", self.Status)
+	} else {
+		if _, err := self.GetModelManager().TableSpec().Update(self, func() error {
+			self.SecgrpId = ""
+			return nil
+		}); err != nil {
+			return nil, err
+		}
+		if err := self.StartSyncTask(ctx, userCred, true, ""); err != nil {
+			return nil, err
+		}
+	}
+	return nil, nil
+}
+
+func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+	if !utils.IsInStringArray(self.Status, []string{VM_READY, VM_RUNNING, VM_SUSPEND}) {
+		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
+	} else {
+		if secgrp, err := data.GetString("secgrp"); err != nil {
+			return nil, err
+		} else if sg, err := SecurityGroupManager.FetchByIdOrName(userCred.GetProjectId(), secgrp); err != nil {
+			return nil, httperrors.NewNotFoundError("SecurityGroup %s not found", secgrp)
+		} else {
+			if _, err := self.GetModelManager().TableSpec().Update(self, func() error {
+				self.SecgrpId = sg.GetId()
+				return nil
+			}); err != nil {
+				return nil, err
+			}
+			if err := self.StartSyncTask(ctx, userCred, true, ""); err != nil {
+				return nil, err
+			}
+		}
+	}
+	return nil, nil
+}
+
 func (self *SGuest) AllowPerformPurge(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
 	return self.IsAdmin(userCred)
 }
@@ -3187,8 +3252,8 @@ func (manager *SGuestManager) getIpsByExit(ips []string, isExitOnly bool) []stri
 	return extRet
 }
 
-func (manager *SGuestManager) getExpiredPendingDeleteGuests() ([]SGuest) {
-	deadline := time.Now().Add(time.Duration(options.Options.PendingDeleteExpireSeconds)*time.Second)
+func (manager *SGuestManager) getExpiredPendingDeleteGuests() []SGuest {
+	deadline := time.Now().Add(time.Duration(options.Options.PendingDeleteExpireSeconds) * time.Second)
 
 	q := manager.Query()
 	q = q.IsTrue("pending_deleted").LT("pending_deleted_at", deadline).In("hypervisor", []string{"aliyun"}).Limit(options.Options.PendingDeleteMaxCleanBatchSize)

pkg/compute/models/secgrouprules.go

@@ -3,6 +3,7 @@ package models
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -11,6 +12,7 @@ import (
 	"yunion.io/x/onecloud/pkg/cloudcommon/db"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
+	"yunion.io/x/pkg/util/compare"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/util/stringutils"
 	"yunion.io/x/sqlchemy"
@@ -39,6 +41,25 @@ type SSecurityGroupRule struct {
 	SecgroupID  string `width:"128" charset:"ascii" create:"required"`
 }
 
+type SecurityGroupRuleSet []SSecurityGroupRule
+
+func (v SecurityGroupRuleSet) Len() int {
+	return len(v)
+}
+
+func (v SecurityGroupRuleSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v SecurityGroupRuleSet) Less(i, j int) bool {
+	if v[i].Priority < v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
 func (manager *SSecurityGroupRuleManager) AllowCreateItem(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
 	return true
 }
@@ -131,6 +152,8 @@ func (manager *SSecurityGroupRuleManager) ValidateCreateData(
 				key += ":"
 			}
 			fields = append(fields, key)
+		} else if field == "cidr" {
+			data.Add(jsonutils.NewString("0.0.0.0/0"), "cidr")
 		}
 	}
 	if _, err := secrules.ParseSecurityRule(strings.Join(fields, " ")); err != nil {
@@ -167,7 +190,11 @@ func (self *SSecurityGroupRule) ValidateUpdateData(ctx context.Context, userCred
 					fields = append(fields, self.CIDR)
 				}
 			case "protocol":
-				fields = append(fields, self.Protocol)
+				protocol := self.Protocol
+				if protocol == "" {
+					protocol = secrules.PROTO_ANY
+				}
+				fields = append(fields, protocol)
 			case "ports":
 				if len(self.Ports) > 0 {
 					fields = append(fields, self.Ports)
@@ -181,7 +208,7 @@ func (self *SSecurityGroupRule) ValidateUpdateData(ctx context.Context, userCred
 	return self.SResourceBase.ValidateUpdateData(ctx, userCred, query, data)
 }
 
-func (self *SSecurityGroupRule) GetRule() string {
+func (self *SSecurityGroupRule) String() string {
 	var fields []string
 	for _, field := range []string{"direction", "action", "cidr", "protocol", "ports"} {
 		switch field {
@@ -193,11 +220,15 @@ func (self *SSecurityGroupRule) GetRule() string {
 		case "action":
 			fields = append(fields, self.Action)
 		case "cidr":
-			if len(self.CIDR) > 0 {
+			if len(self.CIDR) > 0 && self.CIDR != "0.0.0.0/0" {
 				fields = append(fields, self.CIDR)
 			}
 		case "protocol":
-			fields = append(fields, self.Protocol)
+			protocol := self.Protocol
+			if protocol == "" {
+				protocol = secrules.PROTO_ANY
+			}
+			fields = append(fields, protocol)
 		case "ports":
 			if len(self.Ports) > 0 {
 				fields = append(fields, self.Ports)
@@ -207,12 +238,49 @@ func (self *SSecurityGroupRule) GetRule() string {
 	return fields[0] + strings.Join(fields[1:], " ")
 }
 
+func (self *SSecurityGroupRule) SingleRules() ([]secrules.SecurityRule, error) {
+	rules := make([]secrules.SecurityRule, 0)
+	ruleStr := self.String()
+	if rule, err := secrules.ParseSecurityRule(ruleStr); err != nil {
+		return nil, err
+	} else if len(rule.Ports) > 0 {
+		for _, port := range rule.Ports {
+			_rule := secrules.SecurityRule{
+				Priority:    int(self.Priority),
+				Action:      rule.Action,
+				IPNet:       rule.IPNet,
+				Protocol:    rule.Protocol,
+				Direction:   rule.Direction,
+				PortStart:   -1,
+				PortEnd:     -1,
+				Ports:       []int{port},
+				Description: self.Description,
+			}
+			rules = append(rules, _rule)
+		}
+	} else {
+		_rule := secrules.SecurityRule{
+			Priority:    int(self.Priority),
+			Action:      rule.Action,
+			IPNet:       rule.IPNet,
+			Protocol:    rule.Protocol,
+			Direction:   rule.Direction,
+			PortStart:   rule.PortStart,
+			PortEnd:     rule.PortEnd,
+			Ports:       []int{},
+			Description: self.Description,
+		}
+		rules = append(rules, _rule)
+	}
+	return rules, nil
+}
+
 func (self *SSecurityGroupRule) PostCreate(ctx context.Context, userCred mcclient.TokenCredential, ownerProjId string, query jsonutils.JSONObject, data jsonutils.JSONObject) {
 	self.SResourceBase.PostCreate(ctx, userCred, ownerProjId, query, data)
 
 	log.Debugf("POST Create %s", data)
 	if secgroup := self.GetSecGroup(); secgroup != nil {
-		secgroup.DoSync()
+		secgroup.DoSync(ctx, userCred)
 	}
 }
 
@@ -220,7 +288,7 @@ func (self *SSecurityGroupRule) PreDelete(ctx context.Context, userCred mcclient
 	self.SResourceBase.PreDelete(ctx, userCred)
 
 	if secgroup := self.GetSecGroup(); secgroup != nil {
-		secgroup.DoSync()
+		secgroup.DoSync(ctx, userCred)
 	}
 }
 
@@ -229,6 +297,111 @@ func (self *SSecurityGroupRule) PostUpdate(ctx context.Context, userCred mcclien
 
 	log.Debugf("POST Update %s", data)
 	if secgroup := self.GetSecGroup(); secgroup != nil {
-		secgroup.DoSync()
+		secgroup.DoSync(ctx, userCred)
 	}
 }
+
+func (manager *SSecurityGroupRuleManager) getRulesBySecurityGroup(secgroup *SSecurityGroup) ([]SSecurityGroupRule, error) {
+	rules := make([]SSecurityGroupRule, 0)
+	q := manager.Query().Equals("secgroup_id", secgroup.Id)
+	if err := db.FetchModelObjects(manager, q, &rules); err != nil {
+		return nil, err
+	}
+	return rules, nil
+}
+
+func (manager *SSecurityGroupRuleManager) SyncRules(ctx context.Context, userCred mcclient.TokenCredential, secgroup *SSecurityGroup, rules []secrules.SecurityRule) ([]SSecurityGroupRule, []SSecurityGroupRule, compare.SyncResult) {
+	syncResult := compare.SyncResult{}
+
+	if dbRules, err := manager.getRulesBySecurityGroup(secgroup); err != nil {
+		return nil, nil, syncResult
+	} else {
+
+		sort.Sort(SecurityGroupRuleSet(dbRules))
+		sort.Sort(secrules.SecurityRuleSet(rules))
+
+		i, j := 0, 0
+		for i < len(rules) || j < len(dbRules) {
+			if i < len(rules) && j < len(dbRules) {
+				dbStr := dbRules[j].String()
+				ruleStr := rules[i].String()
+				cmp := strings.Compare(dbStr, ruleStr)
+				if cmp == 0 {
+					if dbRules[j].Description != rules[i].Description {
+						if _, err := manager.TableSpec().Update(dbRules[j], func() error {
+							dbRules[j].Description = rules[i].Description
+							return nil
+						}); err != nil {
+							log.Errorf("Update SecurityGroupRule failed: %v", err)
+						}
+					}
+					i += 1
+					j += 1
+				} else if cmp > 0 {
+					if err := dbRules[j].Delete(ctx, userCred); err != nil {
+						syncResult.AddError(err)
+					} else {
+						syncResult.Delete()
+					}
+					j += 1
+				} else {
+					if _, err := manager.newFromCloudSecurityGroup(rules[i], secgroup); err != nil {
+						syncResult.AddError(err)
+					} else {
+						syncResult.Add()
+					}
+					i += 1
+				}
+			} else if i >= len(rules) {
+				if err := dbRules[j].Delete(ctx, userCred); err != nil {
+					syncResult.AddError(err)
+				} else {
+					syncResult.Delete()
+				}
+				j += 1
+			} else if j >= len(dbRules) {
+				if _, err := manager.newFromCloudSecurityGroup(rules[i], secgroup); err != nil {
+					syncResult.AddError(err)
+				} else {
+					syncResult.Add()
+				}
+				i += 1
+			}
+		}
+	}
+	return nil, nil, syncResult
+}
+
+func (manager *SSecurityGroupRuleManager) newFromCloudSecurityGroup(rule secrules.SecurityRule, secgroup *SSecurityGroup) (*SSecurityGroupRule, error) {
+	protocol := rule.Protocol
+	if rule.Protocol == "any" {
+		protocol = ""
+	}
+	ports, _ports := "", make([]string, len(rule.Ports))
+	if len(rule.Ports) > 0 {
+		for _, port := range rule.Ports {
+			_ports = append(_ports, fmt.Sprintf("%d", port))
+		}
+		ports = strings.Join(_ports, ",")
+	} else if rule.PortStart != 0 || rule.PortEnd != 0 {
+		if rule.PortStart == rule.PortEnd {
+			ports = fmt.Sprintf("%d", rule.PortStart)
+		} else {
+			ports = fmt.Sprintf("%d-%d", rule.PortStart, rule.PortEnd)
+		}
+	}
+	secrule := &SSecurityGroupRule{
+		Priority:    int64(rule.Priority),
+		Protocol:    protocol,
+		Ports:       ports,
+		Direction:   string(rule.Direction),
+		CIDR:        rule.IPNet.String(),
+		Action:      string(rule.Action),
+		Description: rule.Description,
+		SecgroupID:  secgroup.Id,
+	}
+	if err := manager.TableSpec().Insert(secrule); err != nil {
+		return nil, err
+	}
+	return secrule, nil
+}

pkg/compute/models/secgroups.go

@@ -3,12 +3,17 @@ package models
 import (
 	"context"
 	"strings"
+	"time"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/onecloud/pkg/cloudcommon/db"
+	"yunion.io/x/onecloud/pkg/cloudcommon/db/lockman"
+	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/onecloud/pkg/mcclient"
+	"yunion.io/x/pkg/util/compare"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/sqlchemy"
 )
 
@@ -94,16 +99,28 @@ func (self *SSecurityGroup) getSecurityRules() (rules []SSecurityGroupRule) {
 	sql := secgrouprules.Query().Filter(sqlchemy.Equals(secgrouprules.Field("secgroup_id"), self.Id))
 	if err := db.FetchModelObjects(SecurityGroupRuleManager, sql, &rules); err != nil {
 		log.Errorf("GetGuests fail %s", err)
-		return nil
+		return
 	}
 	return
 }
 
+func (self *SSecurityGroup) getSecRules() []secrules.SecurityRule {
+	rules := make([]secrules.SecurityRule, 0)
+	for _, _rule := range self.getSecurityRules() {
+		singleRules, err := _rule.SingleRules()
+		if err != nil {
+			log.Errorf(err.Error())
+		}
+		rules = append(rules, singleRules...)
+	}
+	return rules
+}
+
 func (self *SSecurityGroup) getSecurityRuleString() string {
 	secgrouprules := self.getSecurityRules()
 	var rules []string
 	for _, rule := range secgrouprules {
-		rules = append(rules, rule.GetRule())
+		rules = append(rules, rule.String())
 	}
 	return strings.Join(rules, SECURITY_GROUP_SEPARATOR)
 }
@@ -158,11 +175,124 @@ func (self *SSecurityGroup) PerformClone(ctx context.Context, userCred mcclient.
 	return nil, nil
 }
 
-func (self *SSecurityGroup) DoSync() {
+func (manager *SSecurityGroupManager) getSecurityGroups() ([]SSecurityGroup, error) {
+	secgroups := make([]SSecurityGroup, 0)
+	q := manager.Query()
+	if err := db.FetchModelObjects(manager, q, &secgroups); err != nil {
+		return nil, err
+	} else {
+		return secgroups, nil
+	}
+}
+
+func (manager *SSecurityGroupManager) SyncSecgroups(ctx context.Context, userCred mcclient.TokenCredential, secgroups []cloudprovider.ICloudSecurityGroup) ([]SSecurityGroup, []cloudprovider.ICloudSecurityGroup, compare.SyncResult) {
+	localSecgroups := make([]SSecurityGroup, 0)
+	remoteSecgroups := make([]cloudprovider.ICloudSecurityGroup, 0)
+	syncResult := compare.SyncResult{}
+
+	if dbSecgroups, err := manager.getSecurityGroups(); err != nil {
+		syncResult.Error(err)
+		return nil, nil, syncResult
+	} else {
+		removed := make([]SSecurityGroup, 0)
+		commondb := make([]SSecurityGroup, 0)
+		commonext := make([]cloudprovider.ICloudSecurityGroup, 0)
+		added := make([]cloudprovider.ICloudSecurityGroup, 0)
+		if err := compare.CompareSets(dbSecgroups, secgroups, &removed, &commondb, &commonext, &added); err != nil {
+			syncResult.Error(err)
+			return nil, nil, syncResult
+		}
+
+		for i := 0; i < len(commondb); i += 1 {
+			if rules, err := commonext[i].GetRules(); err != nil {
+				syncResult.Error(err)
+			} else if len(rules) > 0 {
+				if err = commondb[i].SyncWithCloudSecurityGroup(userCred, commonext[i]); err != nil {
+					syncResult.UpdateError(err)
+				} else {
+					localSecgroups = append(localSecgroups, commondb[i])
+					remoteSecgroups = append(remoteSecgroups, commonext[i])
+					SecurityGroupRuleManager.SyncRules(ctx, userCred, &commondb[i], rules)
+					syncResult.Update()
+				}
+			}
+		}
+
+		for i := 0; i < len(added); i += 1 {
+			if rules, err := added[i].GetRules(); err != nil {
+				syncResult.AddError(err)
+			} else if len(rules) > 0 {
+				if new, err := manager.newFromCloudVpc(added[i]); err != nil {
+					syncResult.AddError(err)
+				} else {
+					localSecgroups = append(localSecgroups, *new)
+					remoteSecgroups = append(remoteSecgroups, added[i])
+					SecurityGroupRuleManager.SyncRules(ctx, userCred, new, rules)
+					syncResult.Add()
+				}
+			}
+		}
+	}
+	return localSecgroups, remoteSecgroups, syncResult
+}
+
+func (self *SSecurityGroup) SyncWithCloudSecurityGroup(userCred mcclient.TokenCredential, extSec cloudprovider.ICloudSecurityGroup) error {
+	if _, err := self.GetModelManager().TableSpec().Update(self, func() error {
+		self.Name = extSec.GetName()
+		self.Description = extSec.GetDescription()
+		return nil
+	}); err != nil {
+		log.Errorf("syncWithCloudSecurityGroup error %s", err)
+		return err
+	}
+	return nil
+}
+
+func (manager *SSecurityGroupManager) newFromCloudVpc(extSec cloudprovider.ICloudSecurityGroup) (*SSecurityGroup, error) {
+	secgroup := SSecurityGroup{}
+	secgroup.SetModelManager(manager)
+	secgroup.Name = extSec.GetName()
+	secgroup.ExternalId = extSec.GetGlobalId()
+	secgroup.Description = extSec.GetDescription()
+
+	if err := manager.TableSpec().Insert(&secgroup); err != nil {
+		return nil, err
+	}
+	return &secgroup, nil
+}
+
+func (manager *SSecurityGroupManager) DelaySync(ctx context.Context, userCred mcclient.TokenCredential, idStr string) {
+	if secgrp := manager.FetchSecgroupById(idStr); secgrp == nil {
+		log.Errorf("DelaySync secgroup failed")
+	} else {
+		needSync := false
+		lockman.LockObject(ctx, secgrp)
+		defer lockman.ReleaseObject(ctx, secgrp)
+		if secgrp.IsDirty {
+			if _, err := secgrp.GetModelManager().TableSpec().Update(secgrp, func() error {
+				secgrp.IsDirty = false
+				return nil
+			}); err != nil {
+				log.Errorf("Update Security Group error: %s", err.Error())
+			}
+			needSync = true
+		}
+		if needSync {
+			for _, guest := range secgrp.GetGuests() {
+				guest.StartSyncTask(ctx, userCred, true, "")
+			}
+		}
+	}
+}
+
+func (self *SSecurityGroup) DoSync(ctx context.Context, userCred mcclient.TokenCredential) {
 	if _, err := self.GetModelManager().TableSpec().Update(self, func() error {
 		self.IsDirty = true
 		return nil
 	}); err != nil {
 		log.Errorf("Update Security Group error: %s", err.Error())
 	}
+	time.AfterFunc(10*time.Second, func() {
+		SecurityGroupManager.DelaySync(ctx, userCred, self.Id)
+	})
 }

pkg/compute/tasks/cloud_provider_sync_info_task.go

@@ -141,13 +141,31 @@ func syncRegionVPCs(ctx context.Context, provider *models.SCloudprovider, task *
 
 	for j := 0; j < len(localVpcs); j += 1 {
 		syncVpcWires(ctx, provider, task, &localVpcs[j], remoteVpcs[j])
+		syncVpcSecGroup(ctx, provider, task, &localVpcs[j], remoteVpcs[j])
+	}
+}
+
+func syncVpcSecGroup(ctx context.Context, provider *models.SCloudprovider, task *CloudProviderSyncInfoTask, localVpc *models.SVpc, remoteVpc cloudprovider.ICloudVpc) {
+	if secgroups, err := remoteVpc.GetISecurityGroups(); err != nil {
+		msg := fmt.Sprintf("GetISecurityGroups for vpc %s failed %s", remoteVpc.GetId(), err)
+		log.Errorf(msg)
+		logSyncFailed(provider, task, msg)
+		return
+	} else {
+		_, _, result := models.SecurityGroupManager.SyncSecgroups(ctx, task.UserCred, secgroups)
+		msg := result.Result()
+		log.Infof("SyncSecurityGroup for VPC %s result: %s", localVpc.Name, msg)
+		if result.IsError() {
+			logSyncFailed(provider, task, msg)
+			return
+		}
 	}
 }
 
 func syncVpcWires(ctx context.Context, provider *models.SCloudprovider, task taskman.ITask, localVpc *models.SVpc, remoteVpc cloudprovider.ICloudVpc) {
 	wires, err := remoteVpc.GetIWires()
 	if err != nil {
-		msg := fmt.Sprintf("GetIWires for vps %s failed %s", remoteVpc.GetId(), err)
+		msg := fmt.Sprintf("GetIWires for vpc %s failed %s", remoteVpc.GetId(), err)
 		log.Errorf(msg)
 		logSyncFailed(provider, task, msg)
 		return

pkg/compute/tasks/guest_sync_task.go

@@ -38,7 +38,7 @@ func (self *GuestSyncConfTask) OnSyncComplete(ctx context.Context, obj db.IStand
 	guest := obj.(*models.SGuest)
 	if fw_only, _ := self.GetParams().Bool("fw_only"); fw_only {
 		db.OpsLog.LogEvent(guest, db.ACT_SYNC_CONF, nil, self.UserCred)
-		self.OnSyncComplete(ctx, obj, guest.GetShortDesc())
+		self.SetStageComplete(ctx, guest.GetShortDesc())
 	} else if data.Contains("task") {
 		self.SetStage("on_disk_sync_complete", nil)
 	} else {

pkg/util/aliyun/host.go

@@ -222,7 +222,7 @@ func (self *SHost) _createVM(name string, imgId string, sysDiskSize int, cpu int
 		return "", fmt.Errorf("vsiwtch's wire's vpc is empty")
 	}
 
-	secgroups, err := net.wire.vpc.GetSecurityGroups()
+	secgroups, err := net.wire.vpc.GetISecurityGroups()
 	if err != nil {
 		return "", fmt.Errorf("get security group error %s", err)
 	}
@@ -236,7 +236,7 @@ func (self *SHost) _createVM(name string, imgId string, sysDiskSize int, cpu int
 			secgroupId = secId
 		}
 	} else {
-		secgroupId = secgroups[0].SecurityGroupId
+		secgroupId = secgroups[0].GetId()
 	}
 
 	keypair := ""

pkg/util/aliyun/instance.go

@@ -8,6 +8,7 @@ import (
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/util/osprofile"
 	"yunion.io/x/pkg/util/seclib"
+	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
@@ -185,6 +186,10 @@ func (self *SInstance) IsEmulated() bool {
 	return false
 }
 
+func (self *SInstance) getVpc() (*SVpc, error) {
+	return self.host.zone.region.getVpc(self.VpcAttributes.VpcId)
+}
+
 func (self *SInstance) fetchDisks() error {
 	disks, total, err := self.host.zone.region.GetDisks(self.InstanceId, "", "", nil, 0, 50)
 	if err != nil {
@@ -508,3 +513,29 @@ func (self *SInstance) GetVNCInfo() (jsonutils.JSONObject, error) {
 	ret.Add(jsonutils.NewString(self.InstanceId), "instance_id")
 	return ret, nil
 }
+
+func (self *SInstance) SyncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) error {
+	if vpc, err := self.getVpc(); err != nil {
+		return err
+	} else if len(secgroupId) == 0 {
+		for index, secgrpId := range self.SecurityGroupIds.SecurityGroupId {
+			if err := vpc.revokeSecurityGroup(secgrpId, self.InstanceId, index == 0); err != nil {
+				return err
+			}
+		}
+	} else if secgrpId, err := vpc.syncSecurityGroup(secgroupId, name, rules); err != nil {
+		return err
+	} else if err := vpc.assignSecurityGroup(secgrpId, self.InstanceId); err != nil {
+		return err
+	} else {
+		for _, secgroupId := range self.SecurityGroupIds.SecurityGroupId {
+			if secgroupId != secgrpId {
+				if err := vpc.revokeSecurityGroup(secgroupId, self.InstanceId, false); err != nil {
+					return err
+				}
+			}
+		}
+		self.SecurityGroupIds.SecurityGroupId = []string{secgrpId}
+	}
+	return nil
+}

pkg/util/aliyun/region.go

@@ -291,6 +291,7 @@ func (self *SRegion) getVpc(vpcId string) (*SVpc, error) {
 	if total != 1 {
 		return nil, cloudprovider.ErrNotFound
 	}
+	vpcs[0].region = self
 	return &vpcs[0], nil
 }
 

pkg/util/aliyun/securitygroup.go

@@ -2,9 +2,13 @@ package aliyun
 
 import (
 	"fmt"
+	"sort"
+	"strings"
 	"time"
 
+	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/onecloud/pkg/httperrors"
 	"yunion.io/x/pkg/util/secrules"
 	"yunion.io/x/pkg/utils"
 )
@@ -43,6 +47,7 @@ type SPermissions struct {
 }
 
 type SSecurityGroup struct {
+	vpc               *SVpc
 	CreationTime      time.Time
 	Description       string
 	SecurityGroupId   string
@@ -53,6 +58,82 @@ type SSecurityGroup struct {
 	RegionId          string
 }
 
+type PermissionSet []SPermission
+
+func (v PermissionSet) Len() int {
+	return len(v)
+}
+
+func (v PermissionSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v PermissionSet) Less(i, j int) bool {
+	if v[i].Priority < v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
+func (self *SSecurityGroup) GetId() string {
+	return self.SecurityGroupId
+}
+
+func (self *SSecurityGroup) GetGlobalId() string {
+	return self.SecurityGroupId
+}
+
+func (self *SSecurityGroup) GetDescription() string {
+	return self.Description
+}
+
+func (self *SSecurityGroup) GetRules() ([]secrules.SecurityRule, error) {
+	rules := make([]secrules.SecurityRule, 0)
+	if secgrp, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
+		return rules, err
+	} else {
+		for _, permission := range secgrp.Permissions.Permission {
+			if rule, err := secrules.ParseSecurityRule(permission.String()); err != nil {
+				return rules, err
+			} else {
+				priority := permission.Priority
+				if priority > 100 {
+					priority = 100
+				}
+				rule.Priority = 101 - priority
+				rule.Description = permission.Description
+				rules = append(rules, *rule)
+			}
+		}
+	}
+	return rules, nil
+}
+
+func (self *SSecurityGroup) GetName() string {
+	if len(self.SecurityGroupName) > 0 {
+		return self.SecurityGroupName
+	}
+	return self.SecurityGroupId
+}
+
+func (self *SSecurityGroup) GetStatus() string {
+	return ""
+}
+
+func (self *SSecurityGroup) IsEmulated() bool {
+	return false
+}
+
+func (self *SSecurityGroup) Refresh() error {
+	if new, err := self.vpc.region.GetSecurityGroupDetails(self.SecurityGroupId); err != nil {
+		return err
+	} else {
+		return jsonutils.Update(self, new)
+	}
+}
+
 func (self *SRegion) GetSecurityGroups(vpcId string, offset int, limit int) ([]SSecurityGroup, int, error) {
 	if limit > 50 || limit <= 0 {
 		limit = 50
@@ -122,15 +203,25 @@ func (self *SRegion) createSecurityGroup(vpcId string, name string, desc string)
 	return body.GetString("SecurityGroupId")
 }
 
-func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+func (self *SRegion) modifySecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
 	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
 	params["SecurityGroupId"] = secGrpId
+	params["NicType"] = string(IntranetNicType)
+	params["Description"] = rule.Description
+	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
 	protocol := rule.Protocol
-	if len(rule.Protocol) == 0 {
+	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
 		protocol = "all"
 	}
 	params["IpProtocol"] = protocol
-	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
 	if rule.Action == secrules.SecurityRuleAllow {
 		params["Policy"] = "accept"
 	} else {
@@ -143,7 +234,78 @@ func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.Securi
 		} else {
 			params["SourceCidrIp"] = "0.0.0.0/0"
 		}
-		params["DestCidrIp"] = "0.0.0.0/0"
+		_, err := self.ecsRequest("ModifySecurityGroupRule", params)
+		return err
+	} else { // rule.Direction == secrules.SecurityRuleEgress {
+		//阿里云不支持出方向API接口调用
+		return nil
+		// if rule.IPNet != nil {
+		// 	params["DestCidrIp"] = rule.IPNet.String()
+		// } else {
+		// 	params["DestCidrIp"] = "0.0.0.0/0"
+		// }
+		// _, err := self.ecsRequest("ModifySecurityGroupRule", params)
+		// return err
+	}
+}
+
+func (self *SRegion) modifySecurityGroup(secGrpId string, name string, desc string) error {
+	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
+	params["SecurityGroupId"] = secGrpId
+	params["SecurityGroupName"] = name
+	if len(desc) > 0 {
+		params["Description"] = desc
+	}
+	_, err := self.ecsRequest("ModifySecurityGroupAttribute", params)
+	return err
+}
+
+func (self *SRegion) addSecurityGroupRules(secGrpId string, rule *secrules.SecurityRule) error {
+	if len(rule.Ports) != 0 {
+		for _, port := range rule.Ports {
+			rule.PortStart, rule.PortEnd = port, port
+			if err := self.addSecurityGroupRule(secGrpId, rule); err != nil {
+				return err
+			}
+		}
+	} else {
+		return self.addSecurityGroupRule(secGrpId, rule)
+	}
+	return nil
+}
+
+func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
+	params["SecurityGroupId"] = secGrpId
+	params["NicType"] = string(IntranetNicType)
+	params["Description"] = rule.Description
+	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
+	protocol := rule.Protocol
+	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
+		protocol = "all"
+	}
+	params["IpProtocol"] = protocol
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
+	if rule.Action == secrules.SecurityRuleAllow {
+		params["Policy"] = "accept"
+	} else {
+		params["Policy"] = "drop"
+	}
+	params["Priority"] = fmt.Sprintf("%d", 101-rule.Priority)
+	if rule.Direction == secrules.SecurityRuleIngress {
+		if rule.IPNet != nil {
+			params["SourceCidrIp"] = rule.IPNet.String()
+		} else {
+			params["SourceCidrIp"] = "0.0.0.0/0"
+		}
 		_, err := self.ecsRequest("AuthorizeSecurityGroup", params)
 		return err
 	} else { // rule.Direction == secrules.SecurityRuleEgress {
@@ -152,12 +314,54 @@ func (self *SRegion) addSecurityGroupRule(secGrpId string, rule *secrules.Securi
 		} else {
 			params["DestCidrIp"] = "0.0.0.0/0"
 		}
-		params["SourceCidrIp"] = "0.0.0.0/0"
 		_, err := self.ecsRequest("AuthorizeSecurityGroupEgress", params)
 		return err
 	}
 }
 
+func (self *SRegion) delSecurityGroupRule(secGrpId string, rule *secrules.SecurityRule) error {
+	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
+	params["SecurityGroupId"] = secGrpId
+	params["NicType"] = string(IntranetNicType)
+	params["PortRange"] = fmt.Sprintf("%d/%d", rule.PortStart, rule.PortEnd)
+	protocol := rule.Protocol
+	if len(rule.Protocol) == 0 || rule.Protocol == secrules.PROTO_ANY {
+		protocol = "all"
+	}
+	params["IpProtocol"] = protocol
+	if rule.PortStart == 0 && rule.PortEnd == 0 {
+		if protocol == "udp" || protocol == "tcp" {
+			params["PortRange"] = "1/65535"
+		} else {
+			params["PortRange"] = "-1/-1"
+		}
+	}
+	if rule.Action == secrules.SecurityRuleAllow {
+		params["Policy"] = "accept"
+	} else {
+		params["Policy"] = "drop"
+	}
+	params["Priority"] = fmt.Sprintf("%d", rule.Priority)
+	if rule.Direction == secrules.SecurityRuleIngress {
+		if rule.IPNet != nil {
+			params["SourceCidrIp"] = rule.IPNet.String()
+		} else {
+			params["SourceCidrIp"] = "0.0.0.0/0"
+		}
+		_, err := self.ecsRequest("RevokeSecurityGroup", params)
+		return err
+	} else { // rule.Direction == secrules.SecurityRuleEgress {
+		if rule.IPNet != nil {
+			params["DestCidrIp"] = rule.IPNet.String()
+		} else {
+			params["DestCidrIp"] = "0.0.0.0/0"
+		}
+		_, err := self.ecsRequest("RevokeSecurityGroupEgress", params)
+		return err
+	}
+}
+
 func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 	secId, err := self.createSecurityGroup(vpcId, "", "")
 	if err != nil {
@@ -171,7 +375,7 @@ func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 		PortStart: -1,
 		PortEnd:   -1,
 	}
-	err = self.addSecurityGroupRule(secId, &inRule)
+	err = self.addSecurityGroupRules(secId, &inRule)
 	if err != nil {
 		return "", err
 	}
@@ -183,9 +387,190 @@ func (self *SRegion) createDefaultSecurityGroup(vpcId string) (string, error) {
 		PortStart: -1,
 		PortEnd:   -1,
 	}
-	err = self.addSecurityGroupRule(secId, &outRule)
+	err = self.addSecurityGroupRules(secId, &outRule)
 	if err != nil {
 		return "", err
 	}
 	return secId, nil
 }
+
+func (self *SRegion) getSecurityGroupByTag(vpcId, secgroupId string) (*SSecurityGroup, error) {
+	params := make(map[string]string)
+	params["RegionId"] = self.RegionId
+	if len(vpcId) > 0 {
+		params["VpcId"] = vpcId
+	}
+	params["Tag.1.Key"] = "id"
+	params["Tag.1.Value"] = secgroupId
+
+	secgrps := make([]SSecurityGroup, 0)
+	if body, err := self.ecsRequest("DescribeSecurityGroups", params); err != nil {
+		return nil, err
+	} else if err := body.Unmarshal(&secgrps, "SecurityGroups", "SecurityGroup"); err != nil {
+		return nil, err
+	} else if len(secgrps) != 1 {
+		return nil, httperrors.NewNotFoundError("failed to find SecurityGroup %s", secgroupId)
+	}
+	return &secgrps[0], nil
+}
+
+func (self *SPermission) String() string {
+	action := secrules.SecurityRuleDeny
+	if strings.ToLower(self.Policy) == "accept" {
+		action = secrules.SecurityRuleAllow
+	}
+	direction := "in"
+	if self.Direction == "egress" {
+		direction = "out"
+	}
+	cidr := self.SourceCidrIp
+	if direction == "out" {
+		cidr = self.DestCidrIp
+	}
+	if cidr == "0.0.0.0/0" {
+		cidr = ""
+	}
+	protocol := strings.ToLower(self.IpProtocol)
+	if protocol == "all" {
+		protocol = "any"
+	}
+	port, ports := "", strings.Split(self.PortRange, "/")
+	if ports[0] == ports[1] {
+		if ports[0] != "-1" {
+			port = ports[0]
+		}
+	} else if ports[0] != "1" && ports[1] != "65535" {
+		port = fmt.Sprintf("%s-%s", ports[0], ports[1])
+	}
+	result := fmt.Sprintf("%s:%s", direction, string(action))
+	if len(cidr) > 0 {
+		result += fmt.Sprintf(" %s", cidr)
+	}
+	result += fmt.Sprintf(" %s", protocol)
+	if len(port) > 0 {
+		result += fmt.Sprintf(" %s", port)
+	}
+	return result
+}
+
+func (self *SRegion) addTagToSecurityGroup(secgroupId, key, value string, index int) error {
+	if index > 5 || index < 1 {
+		index = 1
+	}
+	params := map[string]string{"ResourceType": "securitygroup", "ResourceId": secgroupId}
+	params[fmt.Sprintf("Tag.%d.Key", index)] = key
+	params[fmt.Sprintf("Tag.%d.Value", index)] = value
+	_, err := self.ecsRequest("AddTags", params)
+	return err
+}
+
+func (self *SRegion) revokeSecurityGroup(secgroupId, instanceId string, keep bool) error {
+	if !keep {
+		return self.leaveSecurityGroup(secgroupId, instanceId)
+	}
+	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
+		return err
+	} else {
+		for _, permission := range secgroup.Permissions.Permission {
+			if rule, err := secrules.ParseSecurityRule(permission.String()); err != nil {
+				return err
+			} else {
+				rule.Priority = permission.Priority
+				if err := self.delSecurityGroupRule(secgroup.SecurityGroupId, rule); err != nil {
+					return err
+				}
+			}
+		}
+		if rule, err := secrules.ParseSecurityRule("in:allow any"); err != nil {
+			rule.Priority = 100
+			if err := self.addSecurityGroupRules(secgroup.SecurityGroupId, rule); err != nil {
+				return err
+			}
+		}
+		if rule, err := secrules.ParseSecurityRule("out:allow any"); err != nil {
+			rule.Priority = 100
+			if err := self.addSecurityGroupRules(secgroup.SecurityGroupId, rule); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func (self *SRegion) syncSecgroupRules(secgroupId string, rules []secrules.SecurityRule) error {
+	if secgroup, err := self.GetSecurityGroupDetails(secgroupId); err != nil {
+		return err
+	} else {
+
+		sort.Sort(secrules.SecurityRuleSet(rules))
+		sort.Sort(PermissionSet(secgroup.Permissions.Permission))
+
+		i, j := 0, 0
+		for i < len(rules) || j < len(secgroup.Permissions.Permission) {
+			if i < len(rules) && j < len(secgroup.Permissions.Permission) {
+				permissionStr := secgroup.Permissions.Permission[j].String()
+				ruleStr := rules[i].String()
+				cmp := strings.Compare(permissionStr, ruleStr)
+				if cmp == 0 {
+					if secgroup.Permissions.Permission[j].Description != rules[i].Description {
+						rules[i].Priority = secgroup.Permissions.Permission[j].Priority
+						if err := self.modifySecurityGroupRule(secgroupId, &rules[i]); err != nil {
+							log.Errorf("modifySecurityGroupRule error %v", rules[i])
+							return err
+						}
+					}
+					i += 1
+					j += 1
+				} else if cmp > 0 {
+					if rule, err := secrules.ParseSecurityRule(permissionStr); err != nil {
+						return err
+					} else {
+						rule.Priority = secgroup.Permissions.Permission[j].Priority
+						if err := self.delSecurityGroupRule(secgroupId, rule); err != nil {
+							log.Errorf("delSecurityGroupRule error %v", rule)
+							return err
+						}
+					}
+					j += 1
+				} else {
+					if err := self.addSecurityGroupRules(secgroupId, &rules[i]); err != nil {
+						log.Errorf("addSecurityGroupRule error %v", rules[i])
+						return err
+					}
+					i += 1
+				}
+			} else if i >= len(rules) {
+				permissionStr := secgroup.Permissions.Permission[j].String()
+				if rule, err := secrules.ParseSecurityRule(permissionStr); err != nil {
+					return err
+				} else {
+					rule.Priority = secgroup.Permissions.Permission[j].Priority
+					if err := self.delSecurityGroupRule(secgroupId, rule); err != nil {
+						log.Errorf("delSecurityGroupRule error %v", rule)
+						return err
+					}
+				}
+				j += 1
+			} else if j >= len(secgroup.Permissions.Permission) {
+				if err := self.addSecurityGroupRules(secgroupId, &rules[i]); err != nil {
+					log.Errorf("addSecurityGroupRule error %v", rules[i])
+					return err
+				}
+				i += 1
+			}
+		}
+	}
+	return nil
+}
+
+func (self *SRegion) assignSecurityGroup(secgroupId, instanceId string) error {
+	params := map[string]string{"InstanceId": instanceId, "SecurityGroupId": secgroupId}
+	_, err := self.ecsRequest("JoinSecurityGroup", params)
+	return err
+}
+
+func (self *SRegion) leaveSecurityGroup(secgroupId, instanceId string) error {
+	params := map[string]string{"InstanceId": instanceId, "SecurityGroupId": secgroupId}
+	_, err := self.ecsRequest("LeaveSecurityGroup", params)
+	return err
+}

pkg/util/aliyun/vpc.go

@@ -5,7 +5,9 @@ import (
 	"time"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/log"
 	"yunion.io/x/onecloud/pkg/cloudprovider"
+	"yunion.io/x/pkg/util/secrules"
 )
 
 const (
@@ -28,7 +30,7 @@ type SVpc struct {
 
 	iwires []cloudprovider.ICloudWire
 
-	secgroups []SSecurityGroup
+	secgroups []cloudprovider.ICloudSecurityGroup
 
 	CidrBlock    string
 	CreationTime time.Time
@@ -159,11 +161,15 @@ func (self *SVpc) fetchSecurityGroups() error {
 			break
 		}
 	}
-	self.secgroups = secgroups
+	self.secgroups = make([]cloudprovider.ICloudSecurityGroup, len(secgroups))
+	for index, secgroup := range secgroups {
+		secgroup.vpc = self
+		self.secgroups[index] = &secgroup
+	}
 	return nil
 }
 
-func (self *SVpc) GetSecurityGroups() ([]SSecurityGroup, error) {
+func (self *SVpc) GetISecurityGroups() ([]cloudprovider.ICloudSecurityGroup, error) {
 	if self.secgroups == nil {
 		err := self.fetchSecurityGroups()
 		if err != nil {
@@ -180,3 +186,39 @@ func (self *SVpc) GetManagerId() string {
 func (self *SVpc) Delete() error {
 	return self.region.DeleteVpc(self.VpcId)
 }
+
+func (self *SVpc) syncSecurityGroup(secgroupId string, name string, rules []secrules.SecurityRule) (string, error) {
+	secgrpId := ""
+	if secgroup, err := self.region.getSecurityGroupByTag(self.VpcId, secgroupId); err != nil {
+		if secgrpId, err = self.region.createSecurityGroup(self.VpcId, name, ""); err != nil {
+			return "", err
+		} else if err := self.region.addTagToSecurityGroup(secgrpId, "id", secgroupId, 1); err != nil {
+			return "", err
+		}
+		//addRules
+		log.Debugf("Add Rules for %s", secgrpId)
+		for _, rule := range rules {
+			if err := self.region.addSecurityGroupRule(secgrpId, &rule); err != nil {
+				return "", err
+			}
+		}
+	} else {
+		//syncRules
+		secgrpId = secgroup.SecurityGroupId
+		log.Debugf("Sync Rules for %s", secgroup.GetName())
+		if secgroup.GetName() != name {
+			if err := self.region.modifySecurityGroup(secgrpId, name, ""); err != nil {
+				log.Errorf("Change SecurityGroup name to %s failed: %v", name, err)
+			}
+		}
+		self.region.syncSecgroupRules(secgrpId, rules)
+	}
+	return secgrpId, nil
+}
+func (self *SVpc) assignSecurityGroup(secgroupId string, instanceId string) error {
+	return self.region.assignSecurityGroup(secgroupId, instanceId)
+}
+
+func (self *SVpc) revokeSecurityGroup(secgroupId string, instanceId string, keep bool) error {
+	return self.region.revokeSecurityGroup(secgroupId, instanceId, keep)
+}

pkg/util/esxi/virtualmachine.go

@@ -1,18 +1,18 @@
 package esxi
 
 import (
+	"fmt"
 	"time"
 
+	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/vim25/mo"
 	"github.com/vmware/govmomi/vim25/types"
 
 	"yunion.io/x/jsonutils"
+	"yunion.io/x/pkg/util/secrules"
 
 	"yunion.io/x/onecloud/pkg/cloudprovider"
 	"yunion.io/x/onecloud/pkg/compute/models"
-
-	"fmt"
-	"github.com/vmware/govmomi/object"
 )
 
 var VIRTUAL_MACHINE_PROPS = []string{"name", "parent", "runtime", "summary"}
@@ -35,6 +35,10 @@ func (self *SVirtualMachine) GetGlobalId() string {
 	return self.getUuid()
 }
 
+func (self *SVirtualMachine) SyncSecurityGroup(secgroupId, name string, rules []secrules.SecurityRule) error {
+	return nil
+}
+
 func (self *SVirtualMachine) GetStatus() string {
 	vm := object.NewVirtualMachine(self.manager.client.Client, self.getVirtualMachine().Self)
 	state, err := vm.PowerState(self.manager.context)

vendor/yunion.io/x/pkg/util/secrules/secrules.go

@@ -25,14 +25,15 @@ const (
 )
 
 type SecurityRule struct {
-	Priority  int // [1, 100]
-	Action    TSecurityRuleAction
-	IPNet     *net.IPNet
-	Protocol  string
-	Direction TSecurityRuleDirection
-	PortStart int
-	PortEnd   int
-	Ports     []int
+	Priority    int // [1, 100]
+	Action      TSecurityRuleAction
+	IPNet       *net.IPNet
+	Protocol    string
+	Direction   TSecurityRuleDirection
+	PortStart   int
+	PortEnd     int
+	Ports       []int
+	Description string
 }
 
 const (
@@ -62,6 +63,25 @@ var (
 	ErrInvalidPort      = errors.New("invalid port")
 )
 
+type SecurityRuleSet []SecurityRule
+
+func (v SecurityRuleSet) Len() int {
+	return len(v)
+}
+
+func (v SecurityRuleSet) Swap(i, j int) {
+	v[i], v[j] = v[j], v[i]
+}
+
+func (v SecurityRuleSet) Less(i, j int) bool {
+	if v[i].Priority > v[j].Priority {
+		return true
+	} else if v[i].Priority == v[j].Priority {
+		return strings.Compare(v[i].String(), v[j].String()) <= 0
+	}
+	return false
+}
+
 func parsePortString(ps string) (int, error) {
 	p, err := strconv.ParseUint(ps, 10, 16)
 	if err != nil || p == 0 {