Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove rule index from rule/group ID/name for VPC and unify NSX resource name for VPC and T1 #785

Conversation

timdengyun
Copy link
Contributor

@timdengyun timdengyun commented Sep 29, 2024

This patch is to:

  1. Remove SecurityPolicy rule index from rule ID and for VPC mode,
    and keep T1 mode rule ID unchanged with rule index.
  2. Remove SecurityPolicy rule index from group ID for VPC mode,
    and keep T1 mode group ID unchanged with rule index.
  3. Remove rule index from NSX group/rule name, and unify the NSX resource name for VPC and T1 network,
    including SecurityPolicy, rule, and group.
  4. Reduce length of rule hash string to 8 chars for VPC mode.

Test Done:

  1. Create SecurityPolicy in VPC mode, do CRUD.
    The SecurityPolicy CR name is: sp-app-access-policy-vpc
    The generated NSX SecurityPolicy:
ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49
Name: sp-app-access-policy-vpc

Policy appliedTo group:

ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_scope
Name: sp-app-access-policy-vpc_scope

For the rule with any ports:

    - direction: in
      action: drop

The build NSX Rule:

ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_d79c8ddf_all
Name: all_ingress_isolation

For rule:

    - direction: in
      action: allow
      sources:
        - vmSelector:
            matchLabels:
              app: coco
      ports:
        - protocol: TCP
          port: 8282
        - protocol: TCP
          port: 7000
          endPort: 7020

The build NSX rule:

ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_44b022be_8282_7000.7020
name: TCP.8282_TCP.7000.7020_ingress_allow

The rule appliedTo group:

ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_b7adc115_scope
Name:sp-app-access-policy-vpc_b7adc115_scope

Rule with user defined name:

  - direction: in
      name: rule-with-defined
      action: allow
      sources:
        - namespaceSelector:
            matchLabels:
              role: validate
          vmSelector:
            matchLabels:
              role: check
        - ipBlocks:
            - cidr: 100.64.232.1/32
      ports:
        - protocol: TCP
          port: 8081
ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_52658c89_8081
Name: rule-with-defined_ingress_allow

The source peer group for the this rule:

Path: /orgs/default/projects/dy-project1/infra/domains/default/groups/sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_52658c89_src
ID: sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_52658c89_src
Name: sp-app-access-policy-vpc_52658c89_src

The NSX Share for this source group:

ID: dy-project1_group_sp-app-access-policy-vpc_36fcf0f9-170a-4346-bb14-9730e199bb49_52658c89_src_share
Name: dy-project1_group_sp-app-access-policy-vpc_52658c89_src_share
  1. Create NetworkPolicy in VPC mode, do CRUD.
    The generated allow section SecurityPolicy:
ID: np-app-access_4a2afd8b-a5e8-4867-8515-fa4961b52f53_allow
Name: np-app-access

The generated isolation section SecurityPolicy:

ID: np-app-access_4a2afd8b-a5e8-4867-8515-fa4961b52f53_isolation
Name: np-app-access
  1. Create SecurityPolicy in VPC mode, and do GC

  2. Create NetworkPolicy in VPC mode, and do GC

  3. Create SecurityPolicy with namedport in VPC mode
    The rule with namedport:

      ports:
        - protocol: TCP
          port: db-port
        - protocol: UDP
          port: 5353
        - protocol: UDP
          port: 5354

The db-port will map to two ports number: 80 and 3366, so, the generated IPSet groups:

ID: sp-namedport-web_2cd759be-8d83-41e4-92d9-4eba6beab6cf_a347c628_3366_ipset
Name: TCP.db-port_UDP.5353_UDP.5354_TCP.3366_ingress_allow_ipset

ID: sp-namedport-web_2cd759be-8d83-41e4-92d9-4eba6beab6cf_a347c628_80_ipset
Name: TCP.db-port_UDP.5353_UDP.5354_TCP.80_ingress_allow_ipset
  1. Create NetworkPolicy with namedport in VPC mode
    For the same named port in the aforementioned, the generated IPSet groups:
ID: np-namedport-web_fd555fd3-04a6-4f6a-b035-79e587dd4767_allow_27cfee51_3366_ipset
Name: TCP.db-port_UDP.5353_UDP.5354_TCP.3366_ingress_allow_ipset

ID: np-namedport-web_fd555fd3-04a6-4f6a-b035-79e587dd4767_allow_27cfee51_80_ipset
Name: TCP.db-port_UDP.5353_UDP.5354_TCP.80_ingress_allow_ipset
  1. Create SecurityPolicy in T1 mode, and do CRUD.

  2. Create SecurityPolicy in T1 mode, and do GC.

  3. T1 upgrade case:
    Create a SecurityPolicy in v4.1.2 code, and start NSX operator with this patch to see if SecurityPolicy Name, group and rule name are changed as expected.
    For the rule created in V4.1.2

    - direction: in
      action: allow
      sources:
        - namespaceSelector:
            matchLabels:
              role: test
      ports:
        - protocol: TCP
          port: 8082
          endPort: 8283

Before upgrade in V4.1.2

ID:sp_e52e71f7-600a-4c67-b241-ce87c6dd191e_c2f3ed42f3e59c15731f8426d65e13676663e147_1_0_0
Name:sp-app-access-policy-1-0-0

After upgrade, ID is not changed.

ID:sp_e52e71f7-600a-4c67-b241-ce87c6dd191e_c2f3ed42f3e59c15731f8426d65e13676663e147_1_0_0
Name:TCP.8082.8283_ingress_allow

@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from 7975902 to 129fd18 Compare September 29, 2024 08:02
@timdengyun timdengyun removed the request for review from heypnus September 29, 2024 08:09
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@5c97290). Learn more about missing BASE report.

Files with missing lines Patch % Lines
pkg/nsx/services/securitypolicy/expand.go 50.00% 2 Missing ⚠️
pkg/util/utils.go 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #785   +/-   ##
=======================================
  Coverage        ?   48.61%           
=======================================
  Files           ?       94           
  Lines           ?    12047           
  Branches        ?        0           
=======================================
  Hits            ?     5857           
  Misses          ?     5705           
  Partials        ?      485           
Flag Coverage Δ
unit-tests 48.61% <96.00%> (?)
Files with missing lines Coverage Δ
pkg/nsx/services/securitypolicy/builder.go 82.75% <100.00%> (ø)
pkg/util/utils.go 50.65% <66.66%> (ø)
pkg/nsx/services/securitypolicy/expand.go 34.19% <50.00%> (ø)

@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from 129fd18 to d1eef01 Compare September 29, 2024 08:23
@timdengyun timdengyun changed the title Remove rule index from rule/group name and unify group name for VPC and T1 Remove rule index from rule/group name for VPC and unify group name for VPC and T1 Sep 29, 2024
@timdengyun timdengyun changed the title Remove rule index from rule/group name for VPC and unify group name for VPC and T1 Remove rule index from rule/group name for VPC and unify name for VPC and T1 Oct 1, 2024
pkg/nsx/services/securitypolicy/builder.go Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from d1eef01 to 91fd791 Compare October 8, 2024 11:08
@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from 91fd791 to cd8ac8e Compare October 8, 2024 16:48
@timdengyun timdengyun changed the title Remove rule index from rule/group name for VPC and unify name for VPC and T1 Remove rule index from rule/group ID/name for VPC and unify NSX resource name for VPC and T1 Oct 8, 2024
pkg/nsx/services/securitypolicy/firewall_test.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Show resolved Hide resolved
pkg/nsx/services/securitypolicy/builder.go Outdated Show resolved Hide resolved
@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from cd8ac8e to 5682e9a Compare October 9, 2024 04:37
wenyingd
wenyingd previously approved these changes Oct 9, 2024
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the code looks good to me. The only left comment is the current implementation related with named port is complicated. Because that logic is not introduced by this change, having offline sync with Yun, we would use another change to refine the functions.

@timdengyun
Copy link
Contributor Author

Most of the code looks good to me. The only left comment is the current implementation related with named port is complicated. Because that logic is not introduced by this change, having offline sync with Yun, we would use another change to refine the functions.

The issue is open to track refactor work.
#795

@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch 2 times, most recently from 06dac12 to 9cadcf1 Compare October 9, 2024 09:18
This patch is to:
1. Remove SecurityPolicy rule index from rule ID and for VPC mode,
and keep T1 mode rule ID unchanged with rule index.
2. Remove SecurityPolicy rule index from group ID for VPC mode,
and keep T1 mode group ID unchanged with rule index.
3. Remove rule index from NSX group/rule name, and unify the NSX resource name for VPC and T1 network,
including SecurityPolicy, rule, and group.
4. Reduce length of rule hash string to 8 chars for VPC mode.
@timdengyun timdengyun force-pushed the Remove_ruleindex_unify_name_for_vpc_t1 branch from 9cadcf1 to 06cfe91 Compare October 9, 2024 09:59
Copy link
Contributor

@zhengxiexie zhengxiexie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timdengyun timdengyun merged commit 0445ed3 into vmware-tanzu:main Oct 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants