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

[dash] Add Privatelink traffic test #14757

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

theasianpianist
Copy link
Contributor

@theasianpianist theasianpianist commented Sep 26, 2024

Description of PR

Summary:
Fixes #14613

Adds outbound and inbound traffic tests for Privatelink

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Smartswitch testbed only

Documentation

@prsunny
Copy link
Contributor

prsunny commented Sep 26, 2024

@mukeshmv to review


@pytest.fixture(scope="module")
def dpu_ip(duthost, dpu_index):
cmd = f"ip addr show | grep Ethernet-BP{dpu_index} | grep inet | awk '{{print $2}}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an example of Nvidia platform:
sonic-net/sonic-buildimage#20358

Signed-off-by: Lawrence Lee <[email protected]>
Signed-off-by: Lawrence Lee <[email protected]>
Copy link
Contributor

@JibinBao JibinBao left a comment

Choose a reason for hiding this comment

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

Can we have a test plan to explain the test? espeicallly for the privatelink_config,
What do the variables defined in the file of privatelink_config.py mean? If there is a chart to introduce the different ips in the test, it will be better.
Thanks

return str(ip_address(overlay_dip))


def outbound_pl_packets(config, inner_packet_type='udp', vxlan_udp_dport=4789):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have scenarios for inbound_pl_packets?


yield

duthost.shell(f"ip route del {pl.SIP}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Before yield, it uses replace to change the static route, which means that a route related to pl.sip may exist. So, if we use del to remove the pl.sip route, it seems that it cannot restore to the original route status. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We expect no related routes to exist on the switch prior to the test, but use 'replace' here in case there are any routes that weren't cleaned up properly so that they will be overwritten

@@ -65,7 +65,7 @@ def test_outbound_direct(
inner_packet_type,
acl_default_rule,
vxlan_udp_dport):
asic_db_checker(["SAI_OBJECT_TYPE_VNET", "SAI_OBJECT_TYPE_ENI"])
# asic_db_checker(["SAI_OBJECT_TYPE_VNET", "SAI_OBJECT_TYPE_ENI"])
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the line?

PL_ENCODING_IP = "::56b2:0:ff71:0:0"
PL_ENCODING_MASK = "::ffff:ffff:ffff:0:0"
PL_UNDERLAY_SIP1 = "55.1.2.3"
PL_UNDERLAY_SIP2 = "55.2.3.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we don't use it

INBOUND_UNDERLAY_IP = "25.1.1.1"
OUTBOUND_UNDERLAY_IP = "101.1.2.3"
VNET_MAP_IP1 = "10.1.1.5"
VNET_MAP_IP2 = "10.1.2.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we don't use it

ip_dst=pl.SIP,
udp_dport=vxlan_udp_dport,
with_udp_chksum=False,
vxlan_vni=int(pl.VNET1_VNI),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outbound packet. VNI should be set to VM_VNI

inner_packet = generate_inner_packet(inner_packet_type)(
eth_src=pl.ENI_MAC,
ip_src=config[LOCAL_CA_IP],
ip_dst=pl.VNET_MAP_IP1,
Copy link
Contributor

Choose a reason for hiding this comment

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

p1.VNET_MAP_IP1 is PE (private endpoint). Right?

@oleksandrivantsiv
Copy link
Contributor

@theasianpianist the test misses the data plane interface configuration for both the NPU and DPU sides. There is a static route that the test adds on the NPU side to send traffic to the DPU, but no IP address configuration on the interfaces. Therefore, the test won’t work without some manual pre-configuration. My assumption is that the test should be atomic and apply the full configuration needed to pass. The test should be extended to set the IP address on the NPU, and the IP address and VIP address on the DPU.

Copy link
Contributor

@oleksandrivantsiv oleksandrivantsiv left a comment

Choose a reason for hiding this comment

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

As commented


VNET_MAPPING_CONFIG = {
f"DASH_VNET_MAPPING_TABLE:{VNET1}:{VNET_MAP_IP1}": {
"mac_address": REMOTE_MAC_STRING,
Copy link
Contributor

Choose a reason for hiding this comment

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

This attribute is not suppored by the PL features:
sonic-net/sonic-swss#3319
Instead of setting this attribute as a part of the configuration, the test should use REMOTE_MAC as a destination inner MAC in the original VXLAN packet sent by PTF


def outbound_pl_packets(config, inner_packet_type='udp', vxlan_udp_dport=4789):
inner_packet = generate_inner_packet(inner_packet_type)(
eth_src=pl.ENI_MAC,
Copy link
Contributor

Choose a reason for hiding this comment

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

The eth_dst should be set to the actual destination MAC address:

eth_dst=pl.REMOTE_MAC,

ptfhost,
messages,
dpu_index,
set=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

'set' is a Python built-in name, better to use another name.

file.write(message.SerializeToString())
update_list.append(path)
else:
path = f"/APPL_DB/dpu{dpu_index}/{filename}"
Copy link
Contributor

Choose a reason for hiding this comment

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

should be gnmi_key instead of filename

logger.info(messages)
apply_messages(localhost, duthost, ptfhost, messages, dpu_index)

return
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no clean up of the dash configurations.

@StormLiangMS
Copy link
Collaborator

/azp run Azure.sonic-mgmt

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StormLiangMS
Copy link
Collaborator

/azp run Azure.sonic-mgmt

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

gre_key=pl.ENCAP_VNI << 8,
inner_frame=exp_inner_packet,
ip_id=0,
ip_ttl=63,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected TTL should be changed to 254 based on sonic-net/DASH#636 PR

"mac_address": ENI_MAC,
"eni_id": ENI_ID,
"admin_state": State.STATE_ENABLED,
"pl_underlay_sip": PL_UNDERLAY_SIP1,
Copy link
Contributor

Choose a reason for hiding this comment

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

pl_underlay_sip in the ENI table should be equal to the SIP address.

if proto_utils.ENABLE_PROTO:
path = f"/APPL_DB/dpu{dpu_index}/{gnmi_key}:$/root/{filename}"
else:
path = f"/APPL_DB/dpu{dpu_index}/{gnmi_key}:@/root/{filename}"
Copy link

@aronovic aronovic Nov 21, 2024

Choose a reason for hiding this comment

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

For the path assignment, the else seems to be the same as the if.

else:
path = "/APPL_DB/localhost/%s:@/root/%s" % (k, filename)
path = "/APPL_DB/dpu1/%s:@/root/%s" % (k, filename)

Choose a reason for hiding this comment

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

Is this test only for DPU1?

Signed-off-by: Lawrence Lee <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lawrence Lee <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theasianpianist theasianpianist changed the title [dash] Add outbound Privatelink traffic test [dash] Add Privatelink traffic test Dec 17, 2024
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).



"""
Test prerequisites:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have the dpuhosts fixture(#15695), can we also automate the logic for adding the dpu baisc config? So, we can automate the PL case fully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll be adding this soon!

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

[Test Gap][dash] Privatelink packet transformation tests
10 participants