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.

Sign up for GitHub

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

YANG validation for ConfigDB Updates: portchannel add/remove, loopback interface, VLAN YANG validation Using GCU #2190

Merged
merged 1 commit into from
Sep 24, 2022
Merged

YANG validation for ConfigDB Updates: portchannel add/remove, loopback interface, VLAN YANG validation Using GCU #2190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 77 additions & 49 deletions config/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import itertools
import copy

from jsonpatch import JsonPatchConflict
from collections import OrderedDict
from generic_config_updater.generic_updater import GenericUpdater, ConfigFormat
from minigraph import parse_device_desc_xml, minigraph_encoder
Expand All @@ -31,6 +32,7 @@
import utilities_common.cli as clicommon
from utilities_common.helper import get_port_pbh_binding, get_port_acl_binding
from utilities_common.general import load_db_config, load_module_from_source
from .validated_config_db_connector import ValidatedConfigDBConnector
import utilities_common.multi_asic as multi_asic_util

from .utils import log
Expand Down Expand Up @@ -104,6 +106,7 @@
TTL_RANGE = click.IntRange(min=0, max=255)
QUEUE_RANGE = click.IntRange(min=0, max=255)
GRE_TYPE_RANGE = click.IntRange(min=0, max=65535)
ADHOC_VALIDATION = True

# Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension.
sonic_cfggen = load_module_from_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen')
Expand Down Expand Up @@ -2031,51 +2034,64 @@ def portchannel(db, ctx, namespace):
@click.pass_context
def add_portchannel(ctx, portchannel_name, min_links, fallback, fast_rate):
"""Add port channel"""
if is_portchannel_name_valid(portchannel_name) != True:
ctx.fail("{} is invalid!, name should have prefix '{}' and suffix '{}'"
.format(portchannel_name, CFG_PORTCHANNEL_PREFIX, CFG_PORTCHANNEL_NO))

db = ctx.obj['db']

if is_portchannel_present_in_db(db, portchannel_name):
ctx.fail("{} already exists!".format(portchannel_name))


fvs = {
'admin_status': 'up',
'mtu': '9100',
'lacp_key': 'auto',
'fast_rate': fast_rate.lower(),
}

if min_links != 0:
fvs['min_links'] = str(min_links)
if fallback != 'false':
fvs['fallback'] = 'true'
db.set_entry('PORTCHANNEL', portchannel_name, fvs)


isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
if ADHOC_VALIDATION:
db = ctx.obj['db']
if is_portchannel_name_valid(portchannel_name) != True:
ctx.fail("{} is invalid!, name should have prefix '{}' and suffix '{}'"
.format(portchannel_name, CFG_PORTCHANNEL_PREFIX, CFG_PORTCHANNEL_NO))
if is_portchannel_present_in_db(db, portchannel_name):
ctx.fail("{} already exists!".format(portchannel_name)) # TODO: MISSING CONSTRAINT IN YANG MODEL
else:
db = ValidatedConfigDBConnector(ctx.obj['db'])

try:
db.set_entry('PORTCHANNEL', portchannel_name, fvs)
except ValueError:
ctx.fail("{} is invalid!, name should have prefix '{}' and suffix '{}'".format(portchannel_name, CFG_PORTCHANNEL_PREFIX, CFG_PORTCHANNEL_NO))

@portchannel.command('del')
@click.argument('portchannel_name', metavar='<portchannel_name>', required=True)
@click.pass_context
def remove_portchannel(ctx, portchannel_name):
"""Remove port channel"""
if is_portchannel_name_valid(portchannel_name) != True:
ctx.fail("{} is invalid!, name should have prefix '{}' and suffix '{}'"
.format(portchannel_name, CFG_PORTCHANNEL_PREFIX, CFG_PORTCHANNEL_NO))

db = ctx.obj['db']
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved

# Dont proceed if the port channel does not exist
if is_portchannel_present_in_db(db, portchannel_name) is False:
ctx.fail("{} is not present.".format(portchannel_name))

# Dont let to remove port channel if vlan membership exists
for k,v in db.get_table('VLAN_MEMBER'):
if v == portchannel_name:
ctx.fail("{} has vlan {} configured, remove vlan membership to proceed".format(portchannel_name, str(k)))

if len([(k, v) for k, v in db.get_table('PORTCHANNEL_MEMBER') if k == portchannel_name]) != 0:
click.echo("Error: Portchannel {} contains members. Remove members before deleting Portchannel!".format(portchannel_name))

if ADHOC_VALIDATION:
db = ctx.obj['db']
if is_portchannel_name_valid(portchannel_name) != True:
ctx.fail("{} is invalid!, name should have prefix '{}' and suffix '{}'"
.format(portchannel_name, CFG_PORTCHANNEL_PREFIX, CFG_PORTCHANNEL_NO))

# Don't proceed if the port channel does not exist
if is_portchannel_present_in_db(db, portchannel_name) is False:
ctx.fail("{} is not present.".format(portchannel_name))

# Dont let to remove port channel if vlan membership exists
for k,v in db.get_table('VLAN_MEMBER'): # TODO: MISSING CONSTRAINT IN YANG MODEL
if v == portchannel_name:
ctx.fail("{} has vlan {} configured, remove vlan membership to proceed".format(portchannel_name, str(k)))

if len([(k, v) for k, v in db.get_table('PORTCHANNEL_MEMBER') if k == portchannel_name]) != 0: # TODO: MISSING CONSTRAINT IN YANG MODEL
ctx.fail("Error: Portchannel {} contains members. Remove members before deleting Portchannel!".format(portchannel_name))
else:
db = ValidatedConfigDBConnector(ctx.obj['db'])

try:
db.set_entry('PORTCHANNEL', portchannel_name, None)
except JsonPatchConflict:
ctx.fail("{} is not present.".format(portchannel_name))

@portchannel.group(cls=clicommon.AbbreviationGroup, name='member')
@click.pass_context
Expand Down Expand Up @@ -2104,8 +2120,8 @@ def add_portchannel_member(ctx, portchannel_name, port_name):
# Dont proceed if the port channel does not exist
if is_portchannel_present_in_db(db, portchannel_name) is False:
ctx.fail("{} is not present.".format(portchannel_name))

# Dont allow a port to be member of port channel if it is configured with an IP address
# Don't allow a port to be member of port channel if it is configured with an IP address
for key,value in db.get_table('INTERFACE').items():
if type(key) == tuple:
continue
Expand Down Expand Up @@ -6148,36 +6164,48 @@ def loopback(ctx, redis_unix_socket_path):
@click.argument('loopback_name', metavar='<loopback_name>', required=True)
@click.pass_context
def add_loopback(ctx, loopback_name):
config_db = ctx.obj['db']
if is_loopback_name_valid(loopback_name) is False:
ctx.fail("{} is invalid, name should have prefix '{}' and suffix '{}' "
.format(loopback_name, CFG_LOOPBACK_PREFIX, CFG_LOOPBACK_NO))

lo_intfs = [k for k, v in config_db.get_table('LOOPBACK_INTERFACE').items() if type(k) != tuple]
if loopback_name in lo_intfs:
ctx.fail("{} already exists".format(loopback_name))

config_db.set_entry('LOOPBACK_INTERFACE', loopback_name, {"NULL" : "NULL"})
if ADHOC_VALIDATION:
config_db = ctx.obj['db']
if is_loopback_name_valid(loopback_name) is False:
ctx.fail("{} is invalid, name should have prefix '{}' and suffix '{}' "
.format(loopback_name, CFG_LOOPBACK_PREFIX, CFG_LOOPBACK_NO))

lo_intfs = [k for k, v in config_db.get_table('LOOPBACK_INTERFACE').items() if type(k) != tuple]
if loopback_name in lo_intfs:
ctx.fail("{} already exists".format(loopback_name)) # TODO: MISSING CONSTRAINT IN YANG VALIDATION
else:
config_db = ValidatedConfigDBConnector(ctx.obj['db'])

try:
config_db.set_entry('LOOPBACK_INTERFACE', loopback_name, {"NULL" : "NULL"})
except ValueError:
ctx.fail("{} is invalid, name should have prefix '{}' and suffix '{}' ".format(loopback_name, CFG_LOOPBACK_PREFIX, CFG_LOOPBACK_NO))

@loopback.command('del')
@click.argument('loopback_name', metavar='<loopback_name>', required=True)
@click.pass_context
def del_loopback(ctx, loopback_name):
config_db = ctx.obj['db']
if is_loopback_name_valid(loopback_name) is False:
ctx.fail("{} is invalid, name should have prefix '{}' and suffix '{}' "
.format(loopback_name, CFG_LOOPBACK_PREFIX, CFG_LOOPBACK_NO))

lo_config_db = config_db.get_table('LOOPBACK_INTERFACE')
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

lo_config_db = config_db.get_table('LOOPBACK_INTERFACE')

This line is moved. However you can keep it at original place, right? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lo_config_db is used with and without ADHOC_VALIDATION, so I moved the definition to before ADHOC_VALIDATION

lo_intfs = [k for k, v in lo_config_db.items() if type(k) != tuple]
if loopback_name not in lo_intfs:
ctx.fail("{} does not exists".format(loopback_name))

if ADHOC_VALIDATION:
if is_loopback_name_valid(loopback_name) is False:
ctx.fail("{} is invalid, name should have prefix '{}' and suffix '{}' "
.format(loopback_name, CFG_LOOPBACK_PREFIX, CFG_LOOPBACK_NO))
lo_intfs = [k for k, v in lo_config_db.items() if type(k) != tuple]
if loopback_name not in lo_intfs:
ctx.fail("{} does not exist".format(loopback_name))
else:
config_db = ValidatedConfigDBConnector(ctx.obj['db'])

ips = [ k[1] for k in lo_config_db if type(k) == tuple and k[0] == loopback_name ]
for ip in ips:
config_db.set_entry('LOOPBACK_INTERFACE', (loopback_name, ip), None)

config_db.set_entry('LOOPBACK_INTERFACE', loopback_name, None)

try:
config_db.set_entry('LOOPBACK_INTERFACE', loopback_name, None)
except JsonPatchConflict:
ctx.fail("{} does not exist".format(loopback_name))


@config.group(cls=clicommon.AbbreviationGroup)
Expand Down
63 changes: 63 additions & 0 deletions config/validated_config_db_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import jsonpatch
from jsonpointer import JsonPointer

from sonic_py_common import device_info
from generic_config_updater.generic_updater import GenericUpdater, ConfigFormat
from generic_config_updater.gu_common import EmptyTableError, genericUpdaterLogging

def ValidatedConfigDBConnector(config_db_connector):
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As we discussed on design, we need to check global disable/enable option. So suggest add the check in this function. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I did not mean fallback_adhoc_validation. I mean yang_config_validation you added in ConfigDB. You may check @wen587 PR https://github.com/sonic-net/sonic-buildimage/pull/11715/files

Copy link
Contributor

Choose a reason for hiding this comment

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

fallback_adhoc_validation should be independent with ValidatedConfigDBConnector. We have use cases like

  1. True, True
  2. True, False
  3. False, True
  4. False, False
    So just remove this parameter in this class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to make fallback_adhoc_validation independent of ValidatedConfigDBConnector. In ValidatedConfigDBConnector, we use yang_config_enabled

yang_enabled = device_info.is_yang_config_validation_enabled(config_db_connector)
if yang_enabled:
config_db_connector.set_entry = validated_set_entry
config_db_connector.delete_table = validated_delete_table
return config_db_connector

def make_path_value_jsonpatch_compatible(table, key, value):
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
if type(key) == tuple:
path = JsonPointer.from_parts([table, '|'.join(key)]).path
else:
isabelmsft marked this conversation as resolved.
Show resolved Hide resolved
path = JsonPointer.from_parts([table, key]).path
if value == {"NULL" : "NULL"}:
value = {}
return path, value

def create_gcu_patch(op, table, key=None, value=None):
if key:
path, value = make_path_value_jsonpatch_compatible(table, key, value)
else:
path = "/{}".format(table)

gcu_json_input = []
gcu_json = {"op": "{}".format(op),
"path": "{}".format(path)}
if op == "add":
gcu_json["value"] = value

gcu_json_input.append(gcu_json)
gcu_patch = jsonpatch.JsonPatch(gcu_json_input)
return gcu_patch

def validated_delete_table(table):
gcu_patch = create_gcu_patch("remove", table)
format = ConfigFormat.CONFIGDB.name
config_format = ConfigFormat[format.upper()]
try:
GenericUpdater().apply_patch(patch=gcu_patch, config_format=config_format, verbose=False, dry_run=False, ignore_non_yang_tables=False, ignore_paths=None)
except ValueError as e:
logger = genericUpdaterLogging.get_logger(title="Patch Applier", print_all_to_console=True)
logger.log_notice("Unable to remove entry, as doing so will result in invalid config. Error: {}".format(e))

def validated_set_entry(table, key, value):
if value is not None:
op = "add"
else:
op = "remove"

gcu_patch = create_gcu_patch(op, table, key, value)
format = ConfigFormat.CONFIGDB.name
config_format = ConfigFormat[format.upper()]

try:
GenericUpdater().apply_patch(patch=gcu_patch, config_format=config_format, verbose=False, dry_run=False, ignore_non_yang_tables=False, ignore_paths=None)
except EmptyTableError:
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

EmptyTableError

I understand GCU could not delete the last vlan entry in vlan table, because it will lead to an empty vlan table. And GCU is okay to delete the whole vlan table in this case.

Suggest catch the exception, if the original patch is delete one entry, we change the patch to delete the whole table and retry apply_patch. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can decorate delete_table, and call it instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or

gcu_json_input.append(gcu_json)
gcu_json_input.append({
    "op": "remove",
    "path": table
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decorated delete_table and changed exception handling to call validated_delete_table

validated_delete_table(table)
Loading