On 6 May 2016 at 07:46, Numan Siddique <nusid...@redhat.com> wrote:
> This patch adds a new OVN action 'dhcp_offer' to support native
> DHCP in OVN.
>
> 'dhcp_offer' takes the DHCP options as input params.
> Eg. dhcp_offer(offerip = 10.0.0.4, router = 10.0.0.1,
> netmask = 255.255.255.0, lease_time = 3600,....)
>
> ovn-controller parses this action and adds a NXT_PACKET_IN2
> OF flow with 'pause' flag set and the DHCP options stored in
> 'userdata' field.
>
> When the DHCP packet is received by ovn-controller, it frames a
> new DHCP reply packet with the DHCP options present in the
> 'userdata' field and resumes the packet.
>
> Eg. dhcp_offer(offerip = 10.0.0.4, router = 10.0.0.1,
> netmask = 255.255.255.0, lease_time = 3600,....)
>
> A new 'DHCP_Options' table is added in SB DB which stores
> the support DHCP options with DHCP code and type. ovn-northd is
> expected to popule this table.
>
> The next patch will add logical flows with this action.
>
> Signed-Off-by: Numan Siddique <nusid...@redhat.com>
Thanks for working on this. I did not try to apply your patch yet so I
might have missed something while reviewing. Also, I was trying to
recollect some DHCP behavioral quirks that I may or may not recollect
correctly after my experience with DHCP. So feel free to ask me to
collaborate on things that I mentioned here but don't make sense to
you.
> ---
> lib/dhcp.h | 13 ++++
> ovn/controller/lflow.c | 11 ++++
> ovn/controller/pinctrl.c | 103 +++++++++++++++++++++++++++++++-
> ovn/lib/actions.c | 151
> ++++++++++++++++++++++++++++++++++++++++++++++-
> ovn/lib/actions.h | 8 +++
> ovn/lib/automake.mk | 3 +-
> ovn/lib/expr.c | 48 ++++-----------
> ovn/lib/expr.h | 37 ++++++++++++
> ovn/lib/ovn-dhcp.h | 121 +++++++++++++++++++++++++++++++++++++
> ovn/ovn-sb.ovsschema | 13 +++-
> ovn/ovn-sb.xml | 44 ++++++++++++++
> tests/automake.mk | 1 +
> tests/ovn.at | 39 ++++++++++++
> tests/test-ovn-dhcp.c | 117 ++++++++++++++++++++++++++++++++++++
> 14 files changed, 667 insertions(+), 42 deletions(-)
> create mode 100644 ovn/lib/ovn-dhcp.h
> create mode 100644 tests/test-ovn-dhcp.c
>
> diff --git a/lib/dhcp.h b/lib/dhcp.h
> index 6f97298..271e0a5 100644
> --- a/lib/dhcp.h
> +++ b/lib/dhcp.h
> @@ -25,6 +25,8 @@
> #define DHCP_SERVER_PORT 67 /* Port used by DHCP server. */
> #define DHCP_CLIENT_PORT 68 /* Port used by DHCP client. */
>
> +#define DHCP_MAGIC_COOKIE 0x63825363
> +
> #define DHCP_HEADER_LEN 236
> struct dhcp_header {
> uint8_t op; /* DHCP_BOOTREQUEST or DHCP_BOOTREPLY. */
> @@ -45,4 +47,15 @@ struct dhcp_header {
> };
> BUILD_ASSERT_DECL(DHCP_HEADER_LEN == sizeof(struct dhcp_header));
>
> +#define DHCP_OP_REQUEST 1
> +#define DHCP_OP_REPLY 2
> +
> +#define DHCP_MSG_DISCOVER 1
> +#define DHCP_MSG_OFFER 2
> +#define DHCP_MSG_REQUEST 3
> +#define DHCP_MSG_ACK 5
I don't see code where you would send DHCP NACK message. The typical
use case for DHCPNACK is if DHCP server configuration changed at the
serverside and DHCP client state machine should immediately move to
INIT state see
[http://www.tcpipguide.com/free/t_DHCPGeneralOperationandClientFiniteStateMachine.htm]
> +
> +#define DHCP_OPT_MSG_TYPE 53
> +#define DHCP_OPT_END 255
> +
> #endif /* dhcp.h */
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 96b7c66..0299d49 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -24,6 +24,7 @@
> #include "ovn-controller.h"
> #include "ovn/lib/actions.h"
> #include "ovn/lib/expr.h"
> +#include "ovn/lib/ovn-dhcp.h"
> #include "ovn/lib/ovn-sb-idl.h"
> #include "packets.h"
> #include "simap.h"
> @@ -203,6 +204,13 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> {
> uint32_t conj_id_ofs = 1;
>
> + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> + const struct sbrec_dhcp_options *dhcp_opt_row;
> + SBREC_DHCP_OPTIONS_FOR_EACH(dhcp_opt_row, ctx->ovnsb_idl) {
> + dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> + dhcp_opt_row->type);
> + }
> +
> const struct sbrec_logical_flow *lflow;
> SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
> /* Determine translation of logical table IDs to physical table IDs.
> */
> @@ -277,6 +285,7 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> };
> struct action_params ap = {
> .symtab = &symtab,
> + .dhcp_opts = &dhcp_opts,
> .lookup_port = lookup_port_cb,
> .aux = &aux,
> .ct_zones = ct_zones,
> @@ -360,6 +369,8 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> ofpbuf_uninit(&ofpacts);
> conj_id_ofs += n_conjs;
> }
> +
> + dhcp_opts_destroy(&dhcp_opts);
> }
>
> static void
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 289a995..489ca21 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -17,7 +17,9 @@
>
> #include "pinctrl.h"
>
> +
> #include "coverage.h"
> +#include "csum.h"
> #include "dirs.h"
> #include "dp-packet.h"
> #include "flow.h"
> @@ -27,6 +29,8 @@
> #include "openvswitch/ofp-print.h"
> #include "openvswitch/ofp-util.h"
> #include "openvswitch/vlog.h"
> +
> +#include "lib/dhcp.h"
> #include "ovn-controller.h"
> #include "ovn/lib/actions.h"
> #include "ovn/lib/logical-fields.h"
> @@ -192,13 +196,106 @@ exit:
> }
>
> static void
> +pinctl_handle_dhcp_offer(struct dp_packet *pkt, struct ofputil_packet_in
> *pin,
> + struct ofpbuf *userdata, struct ofpbuf
> *continuation)
> +{
> + enum ofp_version version = rconn_get_version(swconn);
> + enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
> +
> + ovs_be32 *offer_ip = ofpbuf_pull(userdata, sizeof *offer_ip);
> + const uint8_t *payload = dp_packet_get_udp_payload(pkt);
> + payload = (payload + sizeof (struct dhcp_header) + sizeof(uint32_t));
> +
> + if (payload[0] != DHCP_OPT_MSG_TYPE && (payload[2] != DHCP_MSG_DISCOVER
> ||
> + payload[2] != DHCP_MSG_REQUEST))
> {
> + /* If its not a valid dhcp packet resume the packet without any
> + * changes */
> + goto exit;
> + }
> +
> + uint8_t msg_type;
> + if (payload[2] == DHCP_MSG_DISCOVER) {
> + msg_type = DHCP_MSG_OFFER;
> + } else {
> + msg_type = DHCP_MSG_ACK;
> + }
> +
> + /* Total dhcp options length will be options stored in the userdata +
> + * 16 bytes.
> + *
> + * --------------------------------------------------------------
> + *| 4 Bytes (dhcp cookie) | 3 Bytes (option type) | dhcp options |
> + * --------------------------------------------------------------
> + *| 4 Bytes padding | 1 Byte (option end 0xFF ) | 4 Bytes padding|
> + * --------------------------------------------------------------
> + */
> + uint16_t new_udp_len = UDP_HEADER_LEN + DHCP_HEADER_LEN + \
> + userdata->size + 16;
> + size_t new_packet_len = ETH_HEADER_LEN + IP_HEADER_LEN + new_udp_len;
> +
> + struct dp_packet packet;
> + dp_packet_init(&packet, new_packet_len);
> + dp_packet_clear(&packet);
> + dp_packet_prealloc_tailroom(&packet, new_packet_len);
> +
> + dp_packet_put(&packet, dp_packet_pull(pkt, ETH_HEADER_LEN),
> + ETH_HEADER_LEN);
> + struct ip_header *ip = dp_packet_put(
> + &packet, dp_packet_pull(pkt, IP_HEADER_LEN), IP_HEADER_LEN);
> +
> + struct udp_header *udp = dp_packet_put(
> + &packet, dp_packet_pull(pkt, UDP_HEADER_LEN), UDP_HEADER_LEN);
> +
> + struct dhcp_header *dhcp_data = dp_packet_put(
> + &packet, dp_packet_pull(pkt, DHCP_HEADER_LEN), DHCP_HEADER_LEN);
> + dhcp_data->op = DHCP_OP_REPLY;
> + dhcp_data->yiaddr = *offer_ip;
> +
> + ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> + dp_packet_put(&packet, &magic_cookie, sizeof(ovs_be32));
> +
> + uint8_t *dhcp_options = dp_packet_put_zeros(&packet, userdata->size +
> 12);
> +
> + /* Dhcp option - type */
> + dhcp_options[0] = (uint8_t) DHCP_OPT_MSG_TYPE;
> + dhcp_options[1] = (uint8_t) 1;
> + dhcp_options[2] = msg_type;
> + dhcp_options += 3;
> +
> + memcpy(dhcp_options, userdata->data, userdata->size);
> + dhcp_options += userdata->size;
> +
> + /* Padding */
> + dhcp_options += 4;
> +
> + /* End */
> + dhcp_options[0] = DHCP_OPT_END;
> +
> + udp->udp_len = htons(new_udp_len);
> + ip->ip_tos = 0;
> + ip->ip_tot_len = htons(IP_HEADER_LEN + new_udp_len);
> + udp->udp_csum = 0;
> + ip->ip_csum = 0;
> + ip->ip_csum = csum(ip, sizeof *ip);
> +
> + pin->packet = dp_packet_data(&packet);
> + pin->packet_len = dp_packet_size(&packet);
> +
> +exit:
> + queue_msg(ofputil_encode_resume(pin, continuation, proto));
> + dp_packet_uninit(&packet);
> +}
> +
> +static void
> process_packet_in(const struct ofp_header *msg)
> {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> struct ofputil_packet_in pin;
> + struct ofpbuf continuation;
> enum ofperr error = ofputil_decode_packet_in(msg, true, &pin,
> - NULL, NULL, NULL);
> + NULL, NULL, &continuation);
> +
> if (error) {
> VLOG_WARN_RL(&rl, "error decoding packet-in: %s",
> ofperr_to_string(error));
> @@ -230,6 +327,9 @@ process_packet_in(const struct ofp_header *msg)
> pinctrl_handle_put_arp(&pin.flow_metadata.flow, &headers);
> break;
>
> + case ACTION_OPCODE_DHCP_OFFER:
> + pinctl_handle_dhcp_offer(&packet, &pin, &userdata, &continuation);
> + break;
> default:
> VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
> ntohl(ah->opcode));
> @@ -240,6 +340,7 @@ process_packet_in(const struct ofp_header *msg)
> static void
> pinctrl_recv(const struct ofp_header *oh, enum ofptype type)
> {
> +
> if (type == OFPTYPE_ECHO_REQUEST) {
> queue_msg(make_echo_reply(oh));
> } else if (type == OFPTYPE_GET_CONFIG_REPLY) {
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 5f0bf19..6081cae 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -20,14 +20,18 @@
> #include "actions.h"
> #include "byte-order.h"
> #include "compiler.h"
> +#include "ovn-dhcp.h"
> #include "expr.h"
> #include "lex.h"
> #include "logical-fields.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/ofp-actions.h"
> #include "openvswitch/ofpbuf.h"
> +#include "packets.h"
> +#include "shash.h"
> #include "simap.h"
>
> +
> /* Context maintained during actions_parse(). */
> struct action_context {
> const struct action_params *ap; /* Parameters. */
> @@ -186,13 +190,15 @@ add_prerequisite(struct action_context *ctx, const char
> *prerequisite)
> }
>
> static size_t
> -start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode)
> +start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode,
> + bool pause)
> {
> size_t ofs = ofpacts->size;
>
> struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts);
> oc->max_len = UINT16_MAX;
> oc->reason = OFPR_ACTION;
> + oc->pause = pause;
>
> struct action_header ah = { .opcode = htonl(opcode) };
> ofpbuf_put(ofpacts, &ah, sizeof ah);
> @@ -212,7 +218,7 @@ finish_controller_op(struct ofpbuf *ofpacts, size_t ofs)
> static void
> put_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode)
> {
> - size_t ofs = start_controller_op(ofpacts, opcode);
> + size_t ofs = start_controller_op(ofpacts, opcode, false);
> finish_controller_op(ofpacts, ofs);
> }
>
> @@ -246,7 +252,9 @@ parse_arp_action(struct action_context *ctx)
> * converted to OpenFlow, as its userdata. ovn-controller will convert
> the
> * packet to an ARP and then send the packet and actions back to the
> switch
> * inside an OFPT_PACKET_OUT message. */
> - size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_ARP);
> + /* controller. */
> + size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_ARP,
> + false);
> ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
> ctx->ofpacts, OFP13_VERSION);
> finish_controller_op(ctx->ofpacts, oc_offset);
> @@ -261,6 +269,141 @@ parse_arp_action(struct action_context *ctx)
> }
>
> static bool
> +parse_dhcp_option(struct action_context *ctx, struct ofpbuf *dhcp_opts)
> +{
> + if (ctx->lexer->token.type != LEX_T_ID) {
> + action_syntax_error(ctx, NULL);
> + return false;
> + }
> +
> + enum lex_type lookahead = lexer_lookahead(ctx->lexer);
> + if (lookahead != LEX_T_EQUALS) {
> + action_syntax_error(ctx, NULL);
> + return false;
> + }
> +
> + const struct dhcp_opts_map *dhcp_opt = dhcp_opts_find(
> + ctx->ap->dhcp_opts, ctx->lexer->token.s);
> +
> + if (!dhcp_opt) {
> + action_syntax_error(ctx, "expecting valid dhcp option.");
> + return false;
> + }
> +
> + lexer_get(ctx->lexer);
> + lexer_get(ctx->lexer);
> +
> + struct expr_constant_set cs;
> + memset(&cs, 0, sizeof(struct expr_constant_set));
> + if (!expr_parse_constant_set(ctx->lexer, NULL, &cs)) {
> + action_syntax_error(ctx, "invalid dhcp option values");
> + return false;
> + }
> +
> + if (!lexer_match(ctx->lexer, LEX_T_COMMA) && (
> + ctx->lexer->token.type != LEX_T_RPAREN)) {
> + action_syntax_error(ctx, NULL);
> + return false;
> + }
> +
> +
> + if (dhcp_opt->code == 0) {
> + /* offer-ip */
> + ofpbuf_push(dhcp_opts, &cs.values[0].value.ipv4, sizeof(ovs_be32));
> + goto exit;
> + }
> +
> + uint8_t *opt_header = ofpbuf_put_uninit(dhcp_opts, 2);
> + opt_header[0] = dhcp_opt->code;
> +
> +
> + switch(dhcp_opt->type) {
> + case DHCP_OPT_TYPE_BOOL:
> + case DHCP_OPT_TYPE_UINT8:
> + opt_header[1] = 1;
> + uint8_t val = cs.values[0].value.u8[127];
> + ofpbuf_put(dhcp_opts, &val, 1);
> + break;
> +
> + case DHCP_OPT_TYPE_UINT16:
> + opt_header[1] = 2;
> + ofpbuf_put(dhcp_opts, &cs.values[0].value.be16[63], 2);
> + break;
> +
> + case DHCP_OPT_TYPE_UINT32:
> + opt_header[1] = 4;
> + ofpbuf_put(dhcp_opts, &cs.values[0].value.be32[31], 4);
> + break;
> +
> + case DHCP_OPT_TYPE_IP4:
> + opt_header[1] = 0;
> + for (size_t i = 0; i < cs.n_values; i++) {
> + ofpbuf_put(dhcp_opts, &cs.values[i].value.ipv4,
> sizeof(ovs_be32));
> + opt_header[1] += sizeof(ovs_be32);
> + }
> + break;
> +
> + case DHCP_OPT_TYPE_STATIC_ROUTES:
> + {
> + size_t no_of_routes = cs.n_values;
> + if (no_of_routes % 2) {
> + no_of_routes -= 1;
> + }
> + opt_header[1] = 0;
> + for (size_t i = 0; i < no_of_routes; i+= 2) {
> + uint8_t plen = 32;
> + if (cs.values[i].masked) {
> + plen = (uint8_t) ip_count_cidr_bits(cs.values[i].mask.ipv4);
> + }
> + ofpbuf_put(dhcp_opts, &plen, 1);
> + ofpbuf_put(dhcp_opts, &cs.values[i].value.ipv4, plen / 8);
> + ofpbuf_put(dhcp_opts, &cs.values[i + 1].value.ipv4,
> + sizeof(ovs_be32));
> + opt_header[1] += (1 + (plen / 8) + sizeof(ovs_be32)) ;
> + }
> + break;
> + }
> +
> + case DHCP_OPT_TYPE_STR:
> + opt_header[1] = strlen(cs.values[0].string);
> + ofpbuf_put(dhcp_opts, cs.values[0].string, opt_header[1]);
> + break;
> + }
> +
> +exit:
> + expr_constant_set_destroy(&cs);
> + return true;
> +}
> +
> +static void
> +parse_dhcp_offer_action(struct action_context *ctx)
> +{
> + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> + action_syntax_error(ctx, "expecting `('");
> + return;
> + }
> +
> + uint64_t dhcp_opts_stub[1024 / 8];
> + struct ofpbuf dhcp_opts = OFPBUF_STUB_INITIALIZER(dhcp_opts_stub);
> +
> + /* Parse inner actions. */
> + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> + if (!parse_dhcp_option(ctx, &dhcp_opts)) {
> + return;
> + }
> + }
> +
> + /* controller. */
> + size_t oc_offset = start_controller_op(ctx->ofpacts,
> + ACTION_OPCODE_DHCP_OFFER, true);
> + ofpbuf_put(ctx->ofpacts, dhcp_opts.data, dhcp_opts.size);
> + finish_controller_op(ctx->ofpacts, oc_offset);
> +
> + /* Free memory. */
> + ofpbuf_uninit(&dhcp_opts);
> +}
> +
> +static bool
> action_force_match(struct action_context *ctx, enum lex_type t)
> {
> if (lexer_match(ctx->lexer, t)) {
> @@ -475,6 +618,8 @@ parse_action(struct action_context *ctx)
> parse_get_arp_action(ctx);
> } else if (lexer_match_id(ctx->lexer, "put_arp")) {
> parse_put_arp_action(ctx);
> + } else if (lexer_match_id(ctx->lexer, "dhcp_offer")) {
> + parse_dhcp_offer_action(ctx);
> } else {
> action_syntax_error(ctx, "expecting action");
> }
> diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
> index 29af06f..6c74878 100644
> --- a/ovn/lib/actions.h
> +++ b/ovn/lib/actions.h
> @@ -44,6 +44,11 @@ enum action_opcode {
> * MFF_ETH_SRC = mac
> */
> ACTION_OPCODE_PUT_ARP,
> +
> + /* "dhcp_offer(...dhcp actions ...}".
> + *
> + */
> + ACTION_OPCODE_DHCP_OFFER,
> };
>
> /* Header. */
> @@ -58,6 +63,9 @@ struct action_params {
> * expr_parse()). */
> const struct shash *symtab;
>
> + /* hmap of 'struct dhcp_opts_map' to support 'dhcp_offer' */
> + const struct hmap *dhcp_opts;
> +
> /* Looks up logical port 'port_name'. If found, stores its port number
> in
> * '*portp' and returns true; otherwise, returns false. */
> bool (*lookup_port)(const void *aux, const char *port_name,
> diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
> index 9e09352..7294742 100644
> --- a/ovn/lib/automake.mk
> +++ b/ovn/lib/automake.mk
> @@ -10,7 +10,8 @@ ovn_lib_libovn_la_SOURCES = \
> ovn/lib/expr.h \
> ovn/lib/lex.c \
> ovn/lib/lex.h \
> - ovn/lib/logical-fields.h
> + ovn/lib/logical-fields.h \
> + ovn/lib/ovn-dhcp.h
> nodist_ovn_lib_libovn_la_SOURCES = \
> ovn/lib/ovn-nb-idl.c \
> ovn/lib/ovn-nb-idl.h \
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index f274ab4..05a5c8e 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -405,39 +405,6 @@ expr_print(const struct expr *e)
>
> /* Parsing. */
>
> -/* Type of a "union expr_constant" or "struct expr_constant_set". */
> -enum expr_constant_type {
> - EXPR_C_INTEGER,
> - EXPR_C_STRING
> -};
> -
> -/* A string or integer constant (one must know which from context). */
> -union expr_constant {
> - /* Integer constant.
> - *
> - * The width of a constant isn't always clear, e.g. if you write "1",
> - * there's no way to tell whether you mean for that to be a 1-bit
> constant
> - * or a 128-bit constant or somewhere in between. */
> - struct {
> - union mf_subvalue value;
> - union mf_subvalue mask; /* Only initialized if 'masked'. */
> - bool masked;
> -
> - enum lex_format format; /* From the constant's lex_token. */
> - };
> -
> - /* Null-terminated string constant. */
> - char *string;
> -};
> -
> -/* A collection of "union expr_constant"s of the same type. */
> -struct expr_constant_set {
> - union expr_constant *values; /* Constants. */
> - size_t n_values; /* Number of constants. */
> - enum expr_constant_type type; /* Type of the constants. */
> - bool in_curlies; /* Whether the constants were in {}. */
> -};
> -
> /* A reference to a symbol or a subfield of a symbol.
> *
> * For string fields, ofs and n_bits are 0. */
> @@ -457,7 +424,6 @@ struct expr_context {
>
> struct expr *expr_parse__(struct expr_context *);
> static void expr_not(struct expr *);
> -static void expr_constant_set_destroy(struct expr_constant_set *);
> static bool parse_field(struct expr_context *, struct expr_field *);
>
> static bool
> @@ -838,7 +804,7 @@ parse_constant_set(struct expr_context *ctx, struct
> expr_constant_set *cs)
> return ok;
> }
>
> -static void
> +void
> expr_constant_set_destroy(struct expr_constant_set *cs)
> {
> if (cs) {
> @@ -2928,3 +2894,15 @@ exit:
> }
> return ctx.error;
> }
> +
> +bool
> +expr_parse_constant_set(struct lexer *lexer, const struct shash *symtab,
> + struct expr_constant_set *cs)
> +{
> + struct expr_context ctx = {
> + .lexer = lexer,
> + .symtab = symtab,
> + };
> +
> + return parse_constant_set(&ctx, cs);
> +}
> diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
> index 1327789..99e7aaf 100644
> --- a/ovn/lib/expr.h
> +++ b/ovn/lib/expr.h
> @@ -391,4 +391,41 @@ char *expr_parse_field(struct lexer *, int n_bits, bool
> rw,
> const struct shash *symtab, struct mf_subfield *,
> struct expr **prereqsp);
>
> +/* Type of a "union expr_constant" or "struct expr_constant_set". */
> +enum expr_constant_type {
> + EXPR_C_INTEGER,
> + EXPR_C_STRING
> +};
> +
> +/* A string or integer constant (one must know which from context). */
> +union expr_constant {
> + /* Integer constant.
> + *
> + * The width of a constant isn't always clear, e.g. if you write "1",
> + * there's no way to tell whether you mean for that to be a 1-bit
> constant
> + * or a 128-bit constant or somewhere in between. */
> + struct {
> + union mf_subvalue value;
> + union mf_subvalue mask; /* Only initialized if 'masked'. */
> + bool masked;
> +
> + enum lex_format format; /* From the constant's lex_token. */
> + };
> +
> + /* Null-terminated string constant. */
> + char *string;
> +};
> +
> +/* A collection of "union expr_constant"s of the same type. */
> +struct expr_constant_set {
> + union expr_constant *values; /* Constants. */
> + size_t n_values; /* Number of constants. */
> + enum expr_constant_type type; /* Type of the constants. */
> + bool in_curlies; /* Whether the constants were in {}. */
> +};
> +
> +bool expr_parse_constant_set(struct lexer *, const struct shash *symtab,
> + struct expr_constant_set *cs);
> +void expr_constant_set_destroy(struct expr_constant_set *cs);
> +
> #endif /* ovn/expr.h */
> diff --git a/ovn/lib/ovn-dhcp.h b/ovn/lib/ovn-dhcp.h
> new file mode 100644
> index 0000000..fae944f
> --- /dev/null
> +++ b/ovn/lib/ovn-dhcp.h
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (c) 2016 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_DHCP_H
> +#define OVN_DHCP_H 1
> +
> +#include "hmap.h"
> +#include "hash.h"
> +
> +enum dhcp_opt_type {
> + DHCP_OPT_TYPE_BOOL,
> + DHCP_OPT_TYPE_UINT8,
> + DHCP_OPT_TYPE_UINT16,
> + DHCP_OPT_TYPE_UINT32,
> + DHCP_OPT_TYPE_IP4,
> + DHCP_OPT_TYPE_STATIC_ROUTES,
> + DHCP_OPT_TYPE_STR
> +};
> +
> +struct dhcp_opts_map {
> + struct hmap_node hmap_node;
> + char *name;
> + size_t code;
> + size_t type;
> +};
> +
> +#define DHCP_OPTION(NAME, CODE, TYPE) \
> + {.name = NAME, .code = CODE, .type = TYPE}
> +
> +#define OFFERIP DHCP_OPTION("offerip", 0, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_NETMASK DHCP_OPTION("netmask", 1, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_ROUTER DHCP_OPTION("router", 3, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_DNS_SERVER DHCP_OPTION("dns_server", 6, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_LOG_SERVER DHCP_OPTION("log_server", 7, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_LPR_SERVER DHCP_OPTION("lpr_server", 9, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_SWAP_SERVER DHCP_OPTION("swap_server", 16,
> DHCP_OPT_TYPE_IP4)
> +
> +#define DHCP_OPT_POLICY_FILTER \
> + DHCP_OPTION("policy_filter", 21, DHCP_OPT_TYPE_IP4)
> +
> +#define DHCP_OPT_ROUTER_SOLICITATION \
> + DHCP_OPTION("router_solicitation", 32, DHCP_OPT_TYPE_IP4)
> +
> +#define DHCP_OPT_NIS_SERVER DHCP_OPTION("nis_server", 41, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_NTP_SERVER DHCP_OPTION("ntp_server", 42, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_SERVER_ID DHCP_OPTION("server_id", 54, DHCP_OPT_TYPE_IP4)
> +#define DHCP_OPT_TFTP_SERVER DHCP_OPTION("tftp_server", 66,
> DHCP_OPT_TYPE_IP4)
AFAIK DHCP option tftp_server (66) is tricky. Allegedly there are
clients who use it. However, the KVM PXE implementation asks for this
option via DHCP Parameters list option, but still unconditionally ends
up using SIADDR field to locate tftp server with initial PXE boot
image. If tftp_server happens to be an IP address, then I think you
may want to populate it in SIADDR field as well.
> +
> +#define DHCP_OPT_CLASSLESS_STATIC_ROUTE \
> + DHCP_OPTION("classless_static_route", 121, DHCP_OPT_TYPE_STATIC_ROUTES)
I think Linux dhclient is using option 121, but microsoft dhcp client
is using option 249 for classless static routes See
https://ercpe.de/blog/pushing-static-routes-with-isc-dhcp-server for
more details.
I think ideally DHCP server should honor option 54 (the DHCP options
client understands) and then server should return either 121 or 249.
> +
> +#define DHCP_OPT_IP_FORWARD_ENABLE \
> + DHCP_OPTION("ip_forward_enable", 19, DHCP_OPT_TYPE_BOOL)
> +#define DHCP_OPT_ROUTER_DISCOVERY \
> + DHCP_OPTION("router_discovery", 31, DHCP_OPT_TYPE_BOOL)
> +#define DHCP_OPT_ETHERNET_ENCAP \
> + DHCP_OPTION("ethernet_encap", 36, DHCP_OPT_TYPE_BOOL)
> +
> +#define DHCP_OPT_DEFAULT_TTL \
> + DHCP_OPTION("default_ttl", 23, DHCP_OPT_TYPE_UINT8)
> +
> +#define DHCP_OPT_TCP_TTL DHCP_OPTION("tcp_ttl", 37, DHCP_OPT_TYPE_UINT8)
> +#define DHCP_OPT_MTU DHCP_OPTION("mtu", 26, DHCP_OPT_TYPE_UINT16)
> +#define DHCP_OPT_LEASE_TIME \
> + DHCP_OPTION("lease_time", 51, DHCP_OPT_TYPE_UINT32)
> +
> +static inline uint32_t
> +dhcp_opt_hash(char *opt_name)
> +{
> + return hash_string(opt_name, 0);
> +}
> +
> +static inline struct dhcp_opts_map *
> +dhcp_opts_find(const struct hmap *dhcp_opts, char *opt_name)
> +{
> + struct dhcp_opts_map *dhcp_opt;
> + HMAP_FOR_EACH_WITH_HASH (dhcp_opt, hmap_node, dhcp_opt_hash(opt_name),
> + dhcp_opts) {
> + if (!strcmp(dhcp_opt->name, opt_name)) {
> + return dhcp_opt;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static inline void
> +dhcp_opt_add(struct hmap *dhcp_opts, char *opt_name, size_t code, size_t
> type)
> +{
> + struct dhcp_opts_map *dhcp_opt = xzalloc(sizeof *dhcp_opt);
> + dhcp_opt->name = xstrdup(opt_name);
> + dhcp_opt->code = code;
> + dhcp_opt->type = type;
> + hmap_insert(dhcp_opts, &dhcp_opt->hmap_node, dhcp_opt_hash(opt_name));
> +}
> +
> +static inline void
> +dhcp_opts_destroy(struct hmap *dhcp_opts)
> +{
> + struct dhcp_opts_map *dhcp_opt;
> + HMAP_FOR_EACH_POP(dhcp_opt, hmap_node, dhcp_opts) {
> + free(dhcp_opt->name);
> + free(dhcp_opt);
> + }
> + hmap_destroy(dhcp_opts);
> +}
> +
> +#endif /* OVN_DHCP_H */
> diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
> index 06e8a07..1b20d45 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "1.3.0",
> - "cksum": "654726257 5528",
> + "version": "1.4.0",
> + "cksum": "1002497001 5942",
> "tables": {
> "Chassis": {
> "columns": {
> @@ -110,4 +110,13 @@
> "ip": {"type": "string"},
> "mac": {"type": "string"}},
> "indexes": [["logical_port", "ip"]],
> + "isRoot": true},
> + "DHCP_Options": {
> + "columns": {
> + "name": {"type": "string"},
> + "code": {
> + "type": {"key": {"type": "integer",
> + "minInteger": 0, "maxInteger": 254}}},
> + "type": {"type": {"key": {"type": "integer",
> + "minInteger": 0, "maxInteger": 6}}}},
> "isRoot": true}}}
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index efd2f9a..d699c2e 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1022,6 +1022,35 @@
>
> <p><b>Example:</b> <code>put_arp(inport, arp.spa,
> arp.sha);</code></p>
> </dd>
> +
> + <dt>
> + <code>dhcp_offer(<var>D1</var> = <var>V1</var>, <var>D2</var> =
> <var>V2</var>, ...,<var>Dn</var> = <var>Vn</var>);</code>
> + </dt>
> +
> + <dd>
> + <p>
> + <b>Parameters</b>: DHCP option name <var>Dn</var>, DHCP option
> + value <var>Vn</var>.
> + </p>
> +
> + <p>
> + Valid only in the ingress pipeline.
> + </p>
> +
> + <p>
> + DHCP options and values defined in the parameters are added in
> + the 'DHCP options' field of the incoming DHCP DISCOVER/REQUEST
> + packet and resumed in the pipeline.
> + </p>
> +
> + <p>
> + <b>Example:</b>
> + <code>
> + dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1,
> + netmask = 255.255.255.0, dns_server =
> {8.8.8.8,7.7.7.7});
> + </code>
> + </p>
> + </dd>
> </dl>
>
> <p>
> @@ -1511,4 +1540,19 @@ tcp.flags = RST;
> The Ethernet address to which the IP is bound.
> </column>
> </table>
> +
> + <table name="DHCP_Options" title="DHCP Options supported by native OVN
> DHCP">
> +
> + <column name="name">
> + Name of the DHCP option
> + </column>
> +
> + <column name="code">
> + DHCP option code
> + </column>
> +
> + <column name="type">
> + Data type of the DHCP option code
> + </column>
> + </table>
> </database>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index a5c6074..15fcb50 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -324,6 +324,7 @@ tests_ovstest_SOURCES = \
> tests/test-odp.c \
> tests/test-ofpbuf.c \
> tests/test-ovn.c \
> + tests/test-ovn-dhcp.c \
> tests/test-packets.c \
> tests/test-random.c \
> tests/test-rcu.c \
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b9d7ada..c439947 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -2545,3 +2545,42 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>
> AT_CLEANUP
>
> +AT_SETUP([ovn -- dhcp_offer action])
> +dhcp_options="offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400"
> +echo "dhcp_options = $dhcp_options"
> +
> +expected_dhcp_opts="0a00000403040a0000010104fffffe001a020578"
> +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> + [0], [ignore])
> +
> +dhcp_options="offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,\
> +ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},\
> +classless_static_route={30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,\
> +0.0.0.0/0,10.0.0.1},ethernet_encap=1"
> +
> +# offerip=10.0.0.4 --> 0a000004
> +# router=10.0.0.1 --> 03040a000001
> +# netmask=255.255.255.0 --> 0104ffffff00
> +# mtu=1400 --> 1a020578
> +# ip_forward_enable-1 --> 130101
> +# default_ttl=121 --> 170179
> +# dns_server={8.8.8.8,7.7.7.7} --> 06080808080807070707
> +# classless_static_route= --> 7914
> +# {30.0.0.0/24,10.0.0.4 --> 181e00000a000004
> +# 40.0.0.0/16,10.0.0.6 --> 1028000a000006
> +# 0.0.0.0/0,10.0.0.1} --> 000a000001
> +# ethernet_encap=1 --> 240101
> +# router_discovery=0 --> 1f0100
> +
> +expected_dhcp_opts="0a00000403040a0000010104ffffff001a020578130101170179\
> +060808080808070707077914181e00000a0000041028000a000006000a000001240101"
> +
> +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> + [0], [ignore])
> +
> +dhcp_options="offerip=10.0.0.4,invalid_options=inv_value"
> +expected_dhcp_opts="00"
> +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> + [1], [ignore])
> +
> +AT_CLEANUP
> diff --git a/tests/test-ovn-dhcp.c b/tests/test-ovn-dhcp.c
> new file mode 100644
> index 0000000..60726d9
> --- /dev/null
> +++ b/tests/test-ovn-dhcp.c
> @@ -0,0 +1,117 @@
> +/* Copyright (c) 2016 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <assert.h>
> +#include <config.h>
> +#include "command-line.h"
> +#include "openvswitch/ofp-actions.h"
> +#include "ovstest.h"
> +#include "ovn/lib/actions.h"
> +#include "ovn/lib/ovn-dhcp.h"
> +
> +
> +static void
> +test_dhcp_offer_action(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> +{
> + if (argc != 3) {
> + printf("Usage: %s dhcp-options expected-dhcp-opt-codes", argv[0]);
> + exit(1);
> + }
> +
> + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> +
> + dhcp_opt_add(&dhcp_opts, "offerip", 0, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "netmask", 1, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "router", 3, DHCP_OPT_TYPE_IP4);
DHCP option 3 (Routers) is a list of IP address a (See setion 3.5 of
RFC2132), but DHCP options 1 (subnet mask) is a single IP address (see
3.3 of RFC2132). However, here you are using the same type for all of
these options - DHCP_OPT_TYPE_IP4. Shouldn't you break down
DHCP_OPT_TYPE_IP4 option into two types - one that represents a single
IP address and another that represents a list of IP addresses?
> + dhcp_opt_add(&dhcp_opts, "dns_server", 6, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "log_server", 7, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "lpr_server", 9, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "swap_server", 16, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "policy_filter", 21, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "router_solicitation", 32, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "nis_server", 41, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "ntp_server", 42, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "server_id", 54, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "tftp_server", 66, DHCP_OPT_TYPE_IP4);
> + dhcp_opt_add(&dhcp_opts, "classless_static_route", 121,
> + DHCP_OPT_TYPE_STATIC_ROUTES);
> + dhcp_opt_add(&dhcp_opts, "ip_forward_enable", 19, DHCP_OPT_TYPE_BOOL);
> + dhcp_opt_add(&dhcp_opts, "router_discovery", 31, DHCP_OPT_TYPE_BOOL);
> + dhcp_opt_add(&dhcp_opts, "ethernet_encap", 36, DHCP_OPT_TYPE_BOOL);
> + dhcp_opt_add(&dhcp_opts, "default_ttl", 23, DHCP_OPT_TYPE_UINT8);
> + dhcp_opt_add(&dhcp_opts, "tcp_ttl", 37, DHCP_OPT_TYPE_UINT8);
> + dhcp_opt_add(&dhcp_opts, "mtu", 26, DHCP_OPT_TYPE_UINT16);
> + dhcp_opt_add(&dhcp_opts, "lease_time", 51, DHCP_OPT_TYPE_UINT32);
Should option 58 (renewal time) and 59 (rebinding time) be in this list as well?
If they are absent, then it seems that dhclient3 implementation
derives renewal time and rebinding time from lease_time option by
setting both of them to a "randomized" value that is lower so that all
DHCP clients would not try to renew their leases at the same time. I
don't know what other DHCP client implementations do in such scenarios
where 58 and 59 are absent. But the worst case scenario is that out
there could be some implementations that set option 58, 59 to the
same value as 51 and then there is a chance that client may unassign
IP address from interface and proceed with renewal.
IIRC then ISC dhcpd server always returns all three options (e.g. 58,
59, 51) to client. However, server is the one generating 58 and 59 to
randomized lower value.
> +
> + struct action_params ap = {
> + .dhcp_opts = &dhcp_opts,
> + };
> +
> +
> + char *actions = xasprintf("dhcp_offer(%s);", argv[1]);
> + uint64_t ofpacts_stub[128 / 8];
> + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(
> + ofpacts_stub);
> + struct expr *prereqs;
> + char *error;
> +
> + error = actions_parse_string(actions, &ap, &ofpacts, &prereqs);
> + dhcp_opts_destroy(&dhcp_opts);
> + free(actions);
> + if (error) {
> + printf("actions_parse_string failed with error - %s\n", error);
> + free(error);
> + exit(1);
> + }
> +
> + if (ofpacts.size < (sizeof(struct ofpact_controller) +
> + sizeof(struct action_header))) {
> + ovs_fatal(1, "Error. dhcp_offer parse action failed : "
> + " ofpact_controller not configured");
> + }
> +
> + struct ofpact_controller *oc = ofpbuf_pull(&ofpacts, sizeof *oc);
> + if (!oc->pause) {
> + ovs_fatal(1, "Error. dhcp_offer parse action failed : pause flag "
> + " not set in ofpact_controller");
> + }
> + struct action_header *ah = ofpbuf_pull(&ofpacts, sizeof *ah);
> + if (ah->opcode != htonl(ACTION_OPCODE_DHCP_OFFER)) {
> + ovs_fatal(1, "Error. dhcp_offer parse action failed : dhcp_offer "
> + "action header flag not set");
> + }
> +
> + uint64_t expected_dhcp_opts_stub[128 / 8];
> + struct ofpbuf expected_dhcp_opts = OFPBUF_STUB_INITIALIZER(
> + expected_dhcp_opts_stub);
> + if (ofpbuf_put_hex(&expected_dhcp_opts, argv[2], NULL)[0] != '\0') {
> + ovs_fatal(1, "Error. Invalid expected dhcp opts");
> + }
> +
> + if (oc->userdata_len != (expected_dhcp_opts.size + sizeof *ah)) {
> + ovs_fatal(1, "Error. dhcp_offer parse action failed : userdata
> length"
> + " mismatch. Expected - %lu : Actual - %u",
> + expected_dhcp_opts.size + sizeof *ah, oc->userdata_len);
> + }
> +
> + if (memcmp(ofpacts.data, expected_dhcp_opts.data,
> expected_dhcp_opts.size)) {
> + ovs_fatal(1, "Error. dhcp_offer parse action failed : dhcp opts are"
> + " not as expected");
> + }
> +
> + exit(0);
> +}
> +
> +OVSTEST_REGISTER("test-ovn-dhcp-offer-action", test_dhcp_offer_action);
> --
> 2.5.5
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
