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

[dhcp_server] Add dhcp server cli #17105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xichen96
Copy link
Contributor

@Xichen96 Xichen96 commented Nov 7, 2023

Why I did it

Add cli for dhcp server feature

Work item tracking
  • Microsoft ADO (number only):

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@Xichen96 Xichen96 requested a review from lguohan as a code owner November 7, 2023 02:52


@dhcp_server.group(cls=clicommon.AliasedGroup)
def ipv6():
Copy link
Contributor

@yaqiangz yaqiangz Nov 7, 2023

Choose a reason for hiding this comment

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

We don't need ipv6 currently, you can remove it. I think if add ipv6 part here, it will auto generate command when press "tab"

"lease_time": lease_time,
"gateway": gateway,
"netmask": netmask,
"customized_options": "",
Copy link
Contributor

@yaqiangz yaqiangz Nov 7, 2023

Choose a reason for hiding this comment

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

In yang model, "customized_options" field is not required. I think maybe we can delete this field if it is empty?

@click.option("--netmask", required=False)
@clicommon.pass_db
def add(db, mode, lease_time, infer_gw_nm, gateway, netmask, dhcp_interface):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for ipv6 too

@clicommon.pass_db
def del(db, dhcp_interface):
dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to check whether this key exist and give related echo msg

@ipv6.command()
@click.argument("dhcp_interface", required=True)
@clicommon.pass_db
def del(db, dhcp_interface):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous

@clicommon.pass_db
def enable(db, dhcp_interface):
dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether key and entry exist

@clicommon.pass_db
def disable(db, dhcp_interface):
dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether key and entry exist

def add(db, range_name, ip_start, ip_end):
if not ip_end:
ip_end = ip_start
ipaddress.ip_address(ip_start)
Copy link
Contributor

@yaqiangz yaqiangz Nov 7, 2023

Choose a reason for hiding this comment

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

Check whether range_name exist (expected: not exist)

@click.argument("ip_end", required=False)
@clicommon.pass_db
def update(db, range_name, ip_start, ip_end):
if not ip_end:
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether range_name exist(expected: exist)

@click.argument("range_name", required=True)
@clicommon.pass_db
def del(db, range_name)
dbconn = db.db
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether range_name exist(expected: exist)


def sanity_check_add(mode, lease_time, infer_gw_nm, gateway, netmask, dhcp_interface):
if mode != "PORT":
raise Exception("Only mode PORT is supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

May be use ctx.fail instead raising exception is better?

ctx.fail("{} is invalid IP address".format(ip))

@click.argument("ip_list", required=False, nargs=-1)
@clicommon.pass_db
def bind(db, vlan_interface, interface, range_, ip_list):
dbconn = db.db
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether key exist.
If exist: incremental binding, merge old and new. Need to clarify that cannot bind both ips and ranges in the same time
If not exist: add new entry

elif ip_list:
dbconn.hmset("CONFIG_DB", key, {"ips": ",".join(ip_list)})
else:
raise Exception("One of range and ip_list should be provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to use ctx.fail too

pass


@ip.command()
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate with line 214?

def add(db, option_name, option_id, type_, value):
assert option_id.isdigit()
assert type_ == "string"
dbconn = db.db
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently only support unassigned options

assert option_id.isdigit()
assert type_ == "string"
dbconn = db.db
key = "DHCP_SERVER_IPV4_CUSTOMIZED_OPTIONS|" + option_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether key exist(expected: not exist)

@clicommon.pass_db
def del(db, option_name):
dbconn = db.db
key = "DHCP_SERVER_IPV4_CUSTOMIZED_OPTIONS|" + option_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether key exist (expected: exist)

@clicommon.pass_db
def add(db, option_name, option_id, type_, value):
assert option_id.isdigit()
assert type_ == "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

Support type ["binary", "boolean", "ipv4-address", "string", "uint8", "uint16", "uint32"]

@option.command()
@click.argument("option_name", required=True)
@click.argument("option_id", required=True)
@click.argument("type", required=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing to no required and default "string" for feature support assigned options which type should not be customized and should follow ietf doc

def bind(db, dhcp_interface, option_list):
dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
if dbconn.exists("CONFIG_DB", key) and option_list:
Copy link
Contributor

@yaqiangz yaqiangz Nov 7, 2023

Choose a reason for hiding this comment

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

Check whether option_list exist (expected: exist). I think fail if any of option_list not exist is better?

@clicommon.pass_db
def unbind(db, dhcp_interface, option_list, all_):
dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether dhcp_interface/option_list exist

dbconn = db.db
key = "DHCP_SERVER_IPV4|" + dhcp_interface
if all_:
dbconn.set("CONFIG_DB", key, "customized_options", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

In yang model, "customized_options" field is not required. I think maybe we can delete this field if it is empty?

def unbind(db, dhcp_interface, option_list, all_):
pass


def register(cli):
# cli.add_command(dhcp_server)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rember to add register

entry = dbconn.get_all("STATE_DB", key)
interface, mac = key.split("|")[1:]
port = dbconn.get("STATE_DB", "FDB_TABLE|"+interface+":"+mac, "port")
table.append([interface, mac, port, entry["ip"], entry["lease_start"], entry["lease_end"]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Format of var interface should like "Vlan1000|Ethernet0". You can query FDB table in state_db to get Ethernet0 by mac address

def count_ipv4(start, end):
ip1 = int(ipaddress.IPv4Address(start))
ip2 = int(ipaddress.IPv4Address(end))
return ip2 - ip1
Copy link
Contributor

Choose a reason for hiding this comment

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

return ip2 - ip1 + 1. Range contains start and end

if not dhcp_interface:
dhcp_interface = ""
headers = ["Interface", "Mode", "Gateway", "Netmask", "Lease Time(s)", "IP Bind"]
if with_customize_option:
Copy link
Contributor

Choose a reason for hiding this comment

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

with_customized_options

def add(db, mode, lease_time, infer_gw_nm, gateway, netmask, dhcp_interface):
sanity_check_add(mode, lease_time, infer_gw_nm, gateway, netmask, dhcp_interface):
if infer_gw_nm:
gateway, netmask = infer_param_for_add(dhcp_interface)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add check to make sure gateway and netmask are given if infer_gw_nm is not given

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.

2 participants