Skip to content

Commit 83c73d1

Browse files
theasianpianisttheasianpianist
authored
[DASH] Validate ca to pa SAI attributes (sonic-net#4031)
What I did Only set certain CA to PA attributes if they are valid for a given mapping type Why I did it Some SAI attributes are only valid for privatelink and some are only valid for non-privatelink. This is enforced by the SAI metadata layer, so add this check in orchagent to prevent crashes.
1 parent 98b9d35 commit 83c73d1

4 files changed

Lines changed: 55 additions & 33 deletions

File tree

‎orchagent/dash/dashvnetorch.cpp‎

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -318,29 +318,30 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
318318
return false;
319319
}
320320

321+
uint32_t routing_type_tunnel_key = 0;
322+
sai_dash_encapsulation_t encap_type = SAI_DASH_ENCAPSULATION_INVALID;
321323
for (auto action: route_type_actions.items())
322324
{
323325
if (action.action_type() == dash::route_type::ACTION_TYPE_STATICENCAP)
324326
{
325-
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_DASH_ENCAPSULATION;
326327
if (action.encap_type() == dash::route_type::ENCAP_TYPE_VXLAN)
327328
{
328-
outbound_ca_to_pa_attr.value.u32 = SAI_DASH_ENCAPSULATION_VXLAN;
329+
encap_type = SAI_DASH_ENCAPSULATION_VXLAN;
329330
}
330331
else if (action.encap_type() == dash::route_type::ENCAP_TYPE_NVGRE)
331332
{
332-
outbound_ca_to_pa_attr.value.u32 = SAI_DASH_ENCAPSULATION_NVGRE;
333+
encap_type = SAI_DASH_ENCAPSULATION_NVGRE;
333334
}
334335
else
335336
{
336337
SWSS_LOG_ERROR("Invalid encap type %d for %s", action.encap_type(), key.c_str());
337338
return true;
338339
}
339-
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
340340

341-
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_TUNNEL_KEY;
342-
outbound_ca_to_pa_attr.value.u32 = action.vni();
343-
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
341+
if (action.has_vni())
342+
{
343+
routing_type_tunnel_key = action.vni();
344+
}
344345

345346
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_UNDERLAY_DIP;
346347
to_sai(ctxt.metadata.underlay_ip(), outbound_ca_to_pa_attr.value.ipaddr);
@@ -349,6 +350,7 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
349350
}
350351
}
351352

353+
// Setting SAI attributes that are valid for all values of SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION
352354
if (ctxt.metadata.has_tunnel())
353355
{
354356
auto tunnel_oid = gDirectory.get<DashTunnelOrch*>()->getTunnelOid(ctxt.metadata.tunnel());
@@ -362,12 +364,25 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
362364
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
363365
}
364366

367+
if (ctxt.metadata.has_metering_class_or())
368+
{
369+
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_METER_CLASS_OR;
370+
outbound_ca_to_pa_attr.value.u32 = ctxt.metadata.metering_class_or();
371+
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
372+
}
373+
365374
if (ctxt.metadata.routing_type() == dash::route_type::ROUTING_TYPE_PRIVATELINK)
366375
{
376+
SWSS_LOG_DEBUG("Creating private link outbound CA to PA entry for %s", key.c_str());
377+
// Setting SAI attributes specific to private link routing type
367378
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_ACTION;
368379
outbound_ca_to_pa_attr.value.u32 = SAI_OUTBOUND_CA_TO_PA_ENTRY_ACTION_SET_PRIVATE_LINK_MAPPING;
369380
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
370381

382+
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_DASH_ENCAPSULATION;
383+
outbound_ca_to_pa_attr.value.u32 = encap_type;
384+
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
385+
371386
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_OVERLAY_DIP;
372387
to_sai(ctxt.metadata.overlay_dip_prefix().ip(), outbound_ca_to_pa_attr.value.ipaddr);
373388
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
@@ -376,7 +391,6 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
376391
to_sai(ctxt.metadata.overlay_dip_prefix().mask(), outbound_ca_to_pa_attr.value.ipaddr);
377392
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
378393

379-
380394
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_OVERLAY_SIP;
381395
to_sai(ctxt.metadata.overlay_sip_prefix().ip(), outbound_ca_to_pa_attr.value.ipaddr);
382396
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
@@ -385,6 +399,13 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
385399
to_sai(ctxt.metadata.overlay_sip_prefix().mask(), outbound_ca_to_pa_attr.value.ipaddr);
386400
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
387401

402+
if (routing_type_tunnel_key != 0)
403+
{
404+
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_TUNNEL_KEY;
405+
outbound_ca_to_pa_attr.value.u32 = routing_type_tunnel_key;
406+
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
407+
}
408+
388409
if (ctxt.metadata.has_port_map())
389410
{
390411
auto port_map_oid =
@@ -400,26 +421,22 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
400421
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
401422
}
402423
}
403-
404-
if (ctxt.metadata.has_metering_class_or())
405-
{
406-
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_METER_CLASS_OR;
407-
outbound_ca_to_pa_attr.value.u32 = ctxt.metadata.metering_class_or();
408-
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
409-
}
410-
411-
if (ctxt.metadata.has_mac_address())
424+
else
412425
{
413-
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_OVERLAY_DMAC;
414-
memcpy(outbound_ca_to_pa_attr.value.mac, ctxt.metadata.mac_address().c_str(), sizeof(sai_mac_t));
415-
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
416-
}
426+
// Setting SAI attributes specific to non-private link routing types
427+
if (ctxt.metadata.has_mac_address())
428+
{
429+
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_OVERLAY_DMAC;
430+
memcpy(outbound_ca_to_pa_attr.value.mac, ctxt.metadata.mac_address().c_str(), sizeof(sai_mac_t));
431+
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
432+
}
417433

418-
if (ctxt.metadata.has_use_dst_vni())
419-
{
420-
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_USE_DST_VNET_VNI;
421-
outbound_ca_to_pa_attr.value.booldata = ctxt.metadata.use_dst_vni();
422-
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
434+
if (ctxt.metadata.has_use_dst_vni())
435+
{
436+
outbound_ca_to_pa_attr.id = SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_USE_DST_VNET_VNI;
437+
outbound_ca_to_pa_attr.value.booldata = ctxt.metadata.use_dst_vni();
438+
outbound_ca_to_pa_attrs.push_back(outbound_ca_to_pa_attr);
439+
}
423440
}
424441

425442
object_statuses.emplace_back();

‎tests/dash/test_dash_vnet.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,6 @@ def test_vnet_map(self, dash_db: DashDB):
182182
attrs = dash_db.get_asic_db_entry(ASIC_OUTBOUND_CA_TO_PA_TABLE, vnet_ca_to_pa_maps[0])
183183
assert_sai_attribute_exists("SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_UNDERLAY_DIP", attrs, self.underlay_ip)
184184
assert_sai_attribute_exists("SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_OVERLAY_DMAC", attrs, self.mac_address)
185-
assert_sai_attribute_exists("SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_DASH_ENCAPSULATION", attrs, "SAI_DASH_ENCAPSULATION_NVGRE")
186185
assert_sai_attribute_exists("SAI_OUTBOUND_CA_TO_PA_ENTRY_ATTR_METER_CLASS_OR", attrs, self.vnet_map_metering_class_or)
187186

188187
vnet_pa_validation_maps = dash_db.wait_for_asic_db_keys(ASIC_PA_VALIDATION_TABLE)

‎tests/mock_tests/dashvnetorch_ut.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,16 @@ namespace dashvnetorch_test
7171
{
7272
InSequence seq;
7373
EXPECT_CALL(*mock_sai_dash_vnet_api, create_vnets).Times(1);
74-
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, create_outbound_ca_to_pa_entries).Times(1);
74+
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, create_outbound_ca_to_pa_entries).WillOnce(DoAll(
75+
Return(SAI_STATUS_SUCCESS)
76+
));
7577
EXPECT_CALL(*mock_sai_dash_pa_validation_api, create_pa_validation_entries).Times(1);
76-
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, create_outbound_ca_to_pa_entries).Times(1);
77-
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, remove_outbound_ca_to_pa_entries).Times(2);
78+
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, create_outbound_ca_to_pa_entries).WillOnce(DoAll(
79+
Return(SAI_STATUS_SUCCESS)
80+
));
81+
EXPECT_CALL(*mock_sai_dash_outbound_ca_to_pa_api, remove_outbound_ca_to_pa_entries).Times(2).WillRepeatedly(DoAll(
82+
Return(SAI_STATUS_SUCCESS)
83+
));
7884
EXPECT_CALL(*mock_sai_dash_pa_validation_api, remove_pa_validation_entries).Times(1);
7985
EXPECT_CALL(*mock_sai_dash_vnet_api, remove_vnets).Times(1);
8086
}

‎tests/mock_tests/mock_dash_orch_test.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ namespace mock_orch_test
1616
auto consumer = make_unique<Consumer>(
1717
new swss::ConsumerStateTable(m_app_db.get(), table_name),
1818
target_orch, table_name);
19-
auto op = set ? SET_COMMAND : DEL_COMMAND;
19+
std::string op = set ? SET_COMMAND : DEL_COMMAND;
2020
consumer->addToSync(
2121
swss::KeyOpFieldsValuesTuple(key, op, { { "pb", message.SerializeAsString() } }));
2222
target_orch->doTask(*consumer.get());
@@ -25,13 +25,13 @@ namespace mock_orch_test
2525
if (expect_empty)
2626
{
2727
EXPECT_EQ(it2, consumer->m_toSync.end())
28-
<< "Expected consumer to be empty after operation on table " << table_name
28+
<< "Expected consumer to be empty after " << op << " operation on table " << table_name
2929
<< " with key " << key;
3030
}
3131
else
3232
{
3333
EXPECT_NE(it2, consumer->m_toSync.end())
34-
<< "Expected consumer to not be empty after operation on table " << table_name
34+
<< "Expected consumer to not be empty after " << op << " operation on table " << table_name
3535
<< " with key " << key;
3636
}
3737
}

0 commit comments

Comments
 (0)