delete legacy acls when upgrading to v1.13.x (#4742) #4768

hackerain · 2024-11-27T03:17:07Z

the acls in v1.13.x are in tier 2 rather than tier 0 in v1.12.x, the legacy acls
may cause some unexpected behaviors because acls in tier 0 have the higest priority.
we should delete legacy acls and recreate them when upgrading to v1.13.x.

Pull Request

Make sure you have followed Kube-OVN Code Style.

What type of this PR

Examples of user facing changes:

Features
Bug fixes
Docs
Tests

Which issue(s) this PR fixes

Fixes #(issue-number)

bobz965 · 2024-11-27T08:21:59Z

pkg/ovs/ovn-nb-acl.go

@@ -277,6 +277,32 @@ func (c *OVNNbClient) CreateNodeACL(pgName, nodeIPStr, joinIPStr string) error {
 func (c *OVNNbClient) CreateSgDenyAllACL(sgName string) error {
 	pgName := GetSgPortGroupName(sgName)

+	// for upgrading from v1.12.x to v1.13.x


请把这整个部分放到一个函数中吧，比如 cleanOldACL？

只有默认的 denyall 受影响了嘛？还有别的安全组 acl 也受同样的原因影响嘛？

我盘点了下跟acl相关的逻辑，新旧版本的对比如下：

acl(v1.12.x) * networkpolicy(tier 0) * handleUpdateNp // Informer * UpdateIngressACLOps * UpdateEgressACLOps * CreateGatewayACL * node(tier 0) * CheckNodePortGroup //周期性任务 * CreateNodeACL * sg(tier 0) * initDefaultDenyAllSecurityGroup // 启动时执行一次 * CreateSgDenyAllACL * handleAddOrUpdateSg // Informer * UpdateSgACL * CreateSgBaseACL * subnet(tier 0) * handleAddOrUpdateSubnet // Informer * UpdateLogicalSwitchACL * SetLogicalSwitchPrivate acl(v1.13.x) * networkpolicy(tier 2) * handleUpdateNp // Informer * UpdateIngressACLOps * UpdateEgressACLOps * CreateGatewayACL * node(tier 2) * CheckNodePortGroup //周期性任务 * CreateNodeACL * sg(tier 2) * initDefaultDenyAllSecurityGroup // 启动时执行一次 * CreateSgDenyAllACL * handleAddOrUpdateSg // Informer * UpdateSgACL * CreateSgBaseACL * subnet(tier 2) * handleAddOrUpdateSubnet // Informer * UpdateLogicalSwitchACL * SetLogicalSwitchPrivate * admin network policy(tier 1) * handleAddAnp // Informer * UpdateAnpRuleACLOps * baseline admin network policy(tier 3) * handleAddBanp // Informer * UpdateAnpRuleACLOps

应该是我们做admin network policy功能时，引入的tier划分，原来的tier 0现在都切换到了tier 2，我觉得一个比较好的处理这块升级的逻辑的地方就是在控制器启动的时候，在worker启动以及各项初始化工作之前，将所有这些旧的acls规则删除，然后在初始化以及worker启动之后它会自动重建新的acl规则。

类似这种的升级逻辑，以后可能还会遇到，可以考虑将它做成一个固定的模式，在我们的代码中加入升级的原生支持，将特定的升级步骤加在upgradeXXX()中。

@bobz965 @zhangzujian 看这种方式是否可行？目前该PR是按照这种思路提的，如果可行，我再补充一些UT

我感觉可以，统一放到 upgradeXXX XXX可以是版本号，acl 这里如果检查到 tier0 就触发一次清理吧

coveralls · 2024-12-04T08:19:13Z

Pull Request Test Coverage Report for Build 12177594217

Details

64 of 135 (47.41%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 18.746%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller/admin_network_policy.go	0	2	0.0%
pkg/controller/baseline_admin_network_policy.go	0	2	0.0%
pkg/controller/node.go	12	23	52.17%
pkg/controller/subnet.go	10	21	47.62%
pkg/controller/security_group.go	13	25	52.0%
pkg/controller/network_policy.go	15	30	50.0%
pkg/controller/controller.go	0	18	0.0%

Totals
Change from base Build 12164445128:	0.07%
Covered Lines:	8735
Relevant Lines:	46596

💛 - Coveralls

bobz965 · 2024-12-04T09:48:36Z

pkg/controller/network_policy.go

+			npName = "np" + np.Name
+		}
+		pgName := strings.ReplaceAll(fmt.Sprintf("%s.%s", npName, np.Namespace), "-", ".")
+


移除空行

bobz965 · 2024-12-04T09:48:45Z

pkg/controller/network_policy.go

+}
+
+func (c *Controller) upgradeNetworkPolicies() error {
+	if err := c.upgradeNetworkPoliciesForV1_13(); err != nil {


bobz965 · 2024-12-04T09:49:03Z

pkg/controller/node.go

+
+	for _, node := range nodes {
+		pgName := strings.ReplaceAll(node.Annotations[util.PortNameAnnotation], "-", ".")
+


移除空行

bobz965 · 2024-12-04T09:49:09Z

pkg/controller/node.go

+}
+
+func (c *Controller) upgradeNodes() error {
+	if err := c.upgradeNodesForV1_13(); err != nil {


bobz965 · 2024-12-04T09:49:46Z

pkg/controller/security_group.go

+}
+
+func (c *Controller) upgradeSecurityGroups() error {
+	if err := c.upgradeSecurityGroupsForV1_13(); err != nil {


bobz965 · 2024-12-04T09:49:55Z

pkg/controller/subnet.go

+}
+
+func (c *Controller) upgradeSubnets() error {
+	if err := c.upgradeSubnetsForV1_13(); err != nil {


bobz965 · 2024-12-04T09:50:41Z

pkg/ovs/ovn-nb-acl.go

@@ -1118,6 +1122,10 @@ func aclFilter(direction string, externalIDs map[string]string) func(acl *ovnnb.
 			return false
 		}

+		if tier != -1 && acl.Tier != tier {


这个 -1 比较难理解，常量化维护吧

zhangzujian · 2024-12-05T08:23:22Z

If network policy support is disabled by command line parameter --enable-np=false, kube-ovn-controller will panic:

I1205 02:39:41.184134      13 init.go:660] start to sync subnets
W1205 02:39:41.184146      13 init.go:672] subnet join is not ready
W1205 02:39:41.184152      13 init.go:672] subnet ovn-default is not ready
I1205 02:39:41.184157      13 init.go:749] start to sync vlans
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x5611b713c1f3]

hackerain · 2024-12-05T09:29:46Z

@zhangzujian ok, thank you, I will fix it

the acls in v1.13.x are in tier 2 rather than tier 0 in v1.12.x, the legacy acls may cause some unexpected behaviors because acls in tier 0 have the higest priority. we should delete legacy acls and recreate them when upgrading to v1.13.x. Signed-off-by: suo <[email protected]>

zhangzujian · 2024-12-06T09:26:39Z

pkg/ovs/interface.go

+	DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string, tier int) error
+	DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string, tier int) ([]ovsdb.Operation, error)


Suggested change

DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string, tier int) error

DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string, tier int) ([]ovsdb.Operation, error)

DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string, tier *int) error

DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string, tier *int) ([]ovsdb.Operation, error)

这里用指针是不是更好一点？

dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 27, 2024

hackerain force-pushed the bugfix branch 2 times, most recently from 677f4dd to 8aa71e8 Compare November 27, 2024 06:14

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 27, 2024

hackerain force-pushed the bugfix branch from 8aa71e8 to b93defd Compare November 27, 2024 06:21

hackerain mentioned this pull request Nov 27, 2024

[BUG] 创建pod时绑定安全组，进入pod后ping不通网关 #4742

Open

bobz965 reviewed Nov 27, 2024

View reviewed changes

hackerain force-pushed the bugfix branch from feaae30 to 3b23583 Compare December 2, 2024 06:40

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 2, 2024

hackerain changed the title ~~delete legacy denyall acls when upgrading to v1.13.x (#4742)~~ [WIP]delete legacy acls when upgrading to v1.13.x (#4742) Dec 2, 2024

hackerain force-pushed the bugfix branch from 4d4f383 to 5aa10cc Compare December 4, 2024 08:38

hackerain changed the title ~~[WIP]delete legacy acls when upgrading to v1.13.x (#4742)~~ delete legacy acls when upgrading to v1.13.x (#4742) Dec 4, 2024

bobz965 reviewed Dec 4, 2024

View reviewed changes

hackerain force-pushed the bugfix branch 3 times, most recently from 383cd19 to 4a4ba55 Compare December 5, 2024 02:26

hackerain force-pushed the bugfix branch from 4a4ba55 to 282cc9f Compare December 5, 2024 10:11

zhangzujian reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete legacy acls when upgrading to v1.13.x (#4742) #4768

delete legacy acls when upgrading to v1.13.x (#4742) #4768

hackerain commented Nov 27, 2024 •

edited

Loading

bobz965 Nov 27, 2024

hackerain Dec 2, 2024

bobz965 Dec 2, 2024

coveralls commented Dec 4, 2024 •

edited

Loading

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

bobz965 Dec 4, 2024

zhangzujian commented Dec 5, 2024

hackerain commented Dec 5, 2024

zhangzujian Dec 6, 2024


		for _, node := range nodes {
		pgName := strings.ReplaceAll(node.Annotations[util.PortNameAnnotation], "-", ".")

		DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string, tier int) error
		DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string, tier int) ([]ovsdb.Operation, error)

delete legacy acls when upgrading to v1.13.x (#4742) #4768

Are you sure you want to change the base?

delete legacy acls when upgrading to v1.13.x (#4742) #4768

Conversation

hackerain commented Nov 27, 2024 • edited Loading

Pull Request

What type of this PR

Which issue(s) this PR fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 4, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12177594217

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangzujian commented Dec 5, 2024

hackerain commented Dec 5, 2024

Choose a reason for hiding this comment

hackerain commented Nov 27, 2024 •

edited

Loading

coveralls commented Dec 4, 2024 •

edited

Loading