diff mbox series

[ovs-dev] lex: Indicate that the template wasn't found during parsing.

Message ID 20241023114542.529787-1-amusil@redhat.com
State New
Headers show
Series [ovs-dev] lex: Indicate that the template wasn't found during parsing. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil Oct. 23, 2024, 11:45 a.m. UTC
Indicate that the template variable wasn't found and hte expansion
wasn't succesful. This way we can ignore lflows and LBs that have
template variables, but the expansion for that variable wasn't
provided. This prevents noisy errors especially for LBs:

socket_util|ERR|NODEIP_IPv4_1:32358: bad IP address "NODEIP_IPv4_1"

Reported-at: https://issues.redhat.com/browse/FDP-607
Signed-off-by: Ales Musil <amusil@redhat.com>
---
 controller/lb.c       | 26 +++++++++++++-------------
 controller/lflow.c    | 24 ++++++++++++++++++------
 controller/ofctrl.c   |  8 +++++---
 include/ovn/lex.h     |  6 +++---
 lib/lex.c             | 37 +++++++++++++++++++++++++++++--------
 tests/ovn.at          |  5 ++---
 tests/test-ovn.c      | 10 ++++++++--
 utilities/ovn-trace.c | 31 ++++++++++++++++++++++---------
 8 files changed, 100 insertions(+), 47 deletions(-)
diff mbox series

Patch

diff --git a/controller/lb.c b/controller/lb.c
index 8f9f20ed5..f3e77872a 100644
--- a/controller/lb.c
+++ b/controller/lb.c
@@ -51,7 +51,6 @@  ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
                          struct sset *template_vars_ref)
 {
     struct ovn_controller_lb *lb = xzalloc(sizeof *lb);
-    bool template = smap_get_bool(&sbrec_lb->options, "template", false);
 
     lb->slb = sbrec_lb;
     lb->n_vips = smap_count(&sbrec_lb->vips);
@@ -63,18 +62,19 @@  ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb,
     SMAP_FOR_EACH (node, &sbrec_lb->vips) {
         struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
 
-        struct lex_str key_s = template
-                               ? lexer_parse_template_string(node->key,
-                                                             template_vars,
-                                                             template_vars_ref)
-                               : lex_str_use(node->key);
-        struct lex_str value_s = template
-                               ? lexer_parse_template_string(node->value,
-                                                             template_vars,
-                                                             template_vars_ref)
-                               : lex_str_use(node->value);
-        char *error = ovn_lb_vip_init_explicit(lb_vip,
-                                               lex_str_get(&key_s),
+        struct lex_str key_s;
+        if (!lexer_parse_template_string(&key_s, node->key, template_vars,
+                                        template_vars_ref)) {
+            continue;
+        }
+
+        struct lex_str value_s;
+        if (!lexer_parse_template_string(&value_s, node->value, template_vars,
+                                         template_vars_ref)) {
+            continue;
+        }
+
+        char *error = ovn_lb_vip_init_explicit(lb_vip, lex_str_get(&key_s),
                                                lex_str_get(&value_s));
         if (error) {
             free(error);
diff --git a/controller/lflow.c b/controller/lflow.c
index 13c3a0d73..522c04a89 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -336,9 +336,16 @@  lflow_parse_actions(const struct sbrec_logical_flow *lflow,
         .cur_ltable = lflow->table_id,
     };
 
-    struct lex_str actions_s =
-        lexer_parse_template_string(lflow->actions, l_ctx_in->template_vars,
-                                    template_vars_ref);
+    struct lex_str actions_s;
+    if (!lexer_parse_template_string(&actions_s, lflow->actions,
+                                     l_ctx_in->template_vars,
+                                     template_vars_ref)) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "error parsing actions \"%s\"",
+                     lflow->actions);
+        return false;
+    }
+
     char *error = ovnacts_parse_string(lex_str_get(&actions_s), &pp,
                                        ovnacts_out, prereqs_out);
     lex_str_free(&actions_s);
@@ -974,9 +981,14 @@  convert_match_to_expr(const struct sbrec_logical_flow *lflow,
     struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
     char *error = NULL;
 
-    struct lex_str match_s = lexer_parse_template_string(lflow->match,
-                                                         template_vars,
-                                                         template_vars_ref);
+    struct lex_str match_s;
+    if (!lexer_parse_template_string(&match_s, lflow->match, template_vars,
+                                     template_vars_ref)) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "error parsing match \"%s\"",
+                     lflow->match);
+        return NULL;
+    }
     struct expr *e = expr_parse_string(lex_str_get(&match_s), &symtab,
                                        addr_sets, port_groups, &addr_sets_ref,
                                        &port_groups_ref,
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index f9387d375..b681bc368 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -3027,11 +3027,13 @@  ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s,
     if (version < 0) {
         return xstrdup("OpenFlow channel not ready.");
     }
+    struct lex_str flow_exp_s;
+    if (!lexer_parse_template_string(&flow_exp_s, flow_s,
+                                     template_vars, NULL)) {
+        return xasprintf("Could not parse flow \"%s\"", flow_s);
+    }
 
     struct flow uflow;
-    struct lex_str flow_exp_s = lexer_parse_template_string(flow_s,
-                                                            template_vars,
-                                                            NULL);
     char *error = expr_parse_microflow(lex_str_get(&flow_exp_s), &symtab,
                                        addr_sets, port_groups,
                                        ofctrl_lookup_port, br_int, &uflow);
diff --git a/include/ovn/lex.h b/include/ovn/lex.h
index 64d33361f..513dd5da9 100644
--- a/include/ovn/lex.h
+++ b/include/ovn/lex.h
@@ -196,7 +196,7 @@  lex_str_free(struct lex_str *ls)
     }
 }
 
-struct lex_str lexer_parse_template_string(const char *s,
-                                           const struct smap *template_vars,
-                                           struct sset *template_vars_ref);
+bool lexer_parse_template_string(struct lex_str *ls, const char *s,
+                                 const struct smap *template_vars,
+                                 struct sset *template_vars_ref);
 #endif /* ovn/lex.h */
diff --git a/lib/lex.c b/lib/lex.c
index 06c40f7ae..0ed6f6311 100644
--- a/lib/lex.c
+++ b/lib/lex.c
@@ -1052,35 +1052,56 @@  lexer_steal_error(struct lexer *lexer)
 }
 
 /* Takes ownership of 's' and expands all templates that are encountered
- * in the contents of 's', if possible.  Adds the encountered template names
- * to 'template_vars_ref'.
+ * in the contents of 's'.  Adds the encountered template names
+ * to 'template_vars_ref'. If the expansion is not possible e.g.
+ * missing template var it, the function will return 'false'.
+ * The caller has to free the returned 'struct lex_str'.
  */
-struct lex_str
-lexer_parse_template_string(const char *s, const struct smap *template_vars,
+bool
+lexer_parse_template_string(struct lex_str *ls, const char *s,
+                            const struct smap *template_vars,
                             struct sset *template_vars_ref)
 {
     /* No '^' means no templates. */
     if (!strchr(s, LEX_TEMPLATE_PREFIX)) {
-        return lex_str_use(s);
+        *ls = lex_str_use(s);
+        return true;
     }
 
+    bool ok = true;
     struct ds expanded = DS_EMPTY_INITIALIZER;
 
     struct lexer lexer;
     lexer_init(&lexer, s);
 
     while (lexer_get(&lexer) != LEX_T_END) {
+        if (!ok) {
+            continue;
+        }
+
         if (lexer.token.type == LEX_T_TEMPLATE) {
-            ds_put_cstr(&expanded, smap_get_def(template_vars, lexer.token.s,
-                                                lexer.token.s));
+            const char *template_var = smap_get(template_vars, lexer.token.s);
+            if (template_var) {
+                ds_put_cstr(&expanded, template_var);
+            }
+
             if (template_vars_ref) {
                 sset_add(template_vars_ref, lexer.token.s);
             }
+
+            ok &= !!template_var;
         } else {
             lex_token_format(&lexer.token, &expanded);
         }
     }
 
     lexer_destroy(&lexer);
-    return lex_str_steal(ds_steal_cstr(&expanded));
+
+    if (ok) {
+        *ls = lex_str_steal(ds_steal_cstr(&expanded));
+    } else {
+        ds_destroy(&expanded);
+        *ls = lex_str_use("");
+    }
+    return ok;
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index c66dee6ac..ae152189f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -35717,6 +35717,7 @@  AT_CHECK([ovs-ofctl dump-flows br-int | grep '42\.42\.42\.42'], [1], [])
 # Need  to investigate whteher those errors are expected
 OVN_CLEANUP([hv1
 /Syntax error at /d
+/error parsing.*\^/d
 ])
 
 AT_CLEANUP
@@ -35889,11 +35890,9 @@  AT_CHECK([ovs-ofctl dump-groups br-int | grep 'nat'], [1], [])
 as hv1
 AT_CHECK([ovs-ofctl dump-flows br-int | grep table=OFTABLE_CHK_LB_HAIRPIN | ofctl_strip_all], [0], [])
 
-# Ignore errors such as "Syntax error at `BACKENDS31' expecting IP address"
-# Need  to investigate whteher those errors are expected
 OVN_CLEANUP([hv1
 /bad port number/d
-/Syntax error at/d
+/error parsing.*\^/d
 ])
 
 AT_CLEANUP
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 2ea68f212..5d2b132ca 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1326,6 +1326,13 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
         puts(ds_cstr(&input));
 
+        struct lex_str exp_input;
+        if (!lexer_parse_template_string(&exp_input, ds_cstr(&input),
+                                         &template_vars, NULL)) {
+            printf("    failed to parse %s\n", ds_cstr(&input));
+            continue;
+        }
+
         ofpbuf_init(&ovnacts, 0);
 
         const struct ovnact_parse_params pp = {
@@ -1337,8 +1344,7 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
             .n_tables = 24,
             .cur_ltable = 10,
         };
-        struct lex_str exp_input =
-            lexer_parse_template_string(ds_cstr(&input), &template_vars, NULL);
+
         error = ovnacts_parse_string(lex_str_get(&exp_input), &pp, &ovnacts,
                                      &prereqs);
         if (!error) {
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index f9cc2463b..656196db1 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -972,10 +972,12 @@  parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf,
         }
 
         char *error;
+        struct lex_str match_s;
+        if (!lexer_parse_template_string(&match_s, sblf->match,
+                                         &template_vars, NULL)) {
+            VLOG_WARN("%s: parsing expression failed", sblf->match);
+        }
         struct expr *match;
-        struct lex_str match_s = lexer_parse_template_string(sblf->match,
-                                                             &template_vars,
-                                                             NULL);
         match = expr_parse_string(lex_str_get(&match_s), &symtab,
                                   &address_sets, &port_groups, NULL, NULL,
                                   dp->tunnel_key, &error);
@@ -1003,8 +1005,13 @@  parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf,
         uint64_t stub[1024 / 8];
         struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(stub);
         struct expr *prereqs;
-        struct lex_str actions_s =
-            lexer_parse_template_string(sblf->actions, &template_vars, NULL);
+        struct lex_str actions_s;
+        if (!lexer_parse_template_string(&actions_s, sblf->actions,
+                                         &template_vars, NULL)) {
+            VLOG_WARN("%s: parsing actions failed", sblf->actions);
+            expr_destroy(match);
+            return;
+        }
         error = ovnacts_parse_string(lex_str_get(&actions_s), &pp, &ovnacts,
                                      &prereqs);
         lex_str_free(&actions_s);
@@ -3577,8 +3584,11 @@  trace_parse(const char *dp_s, const char *flow_s,
          *
          * First make sure that the expression parses. */
         char *error;
-        struct lex_str flow_exp_s =
-            lexer_parse_template_string(flow_s, &template_vars, NULL);
+        struct lex_str flow_exp_s;
+        if (!lexer_parse_template_string(&flow_exp_s, flow_s,
+                                         &template_vars, NULL)) {
+            return xasprintf("failed to parse flow expression \"%s\"", flow_s);
+        }
         struct expr *e = expr_parse_string(lex_str_get(&flow_exp_s), &symtab,
                                            &address_sets, &port_groups, NULL,
                                            NULL, 0, &error);
@@ -3607,8 +3617,11 @@  trace_parse(const char *dp_s, const char *flow_s,
         free(port_name);
     }
 
-    struct lex_str flow_exp_s =
-        lexer_parse_template_string(flow_s, &template_vars, NULL);
+    struct lex_str flow_exp_s;
+    if (!lexer_parse_template_string(&flow_exp_s, flow_s,
+                                     &template_vars, NULL)) {
+        return xasprintf("failed to parse flow expression \"%s\"", flow_s);
+    }
     char *error = expr_parse_microflow(lex_str_get(&flow_exp_s), &symtab,
                                        &address_sets, &port_groups,
                                        ovntrace_lookup_port, *dpp, uflow);