[#4443] Changed to reset N

Author: fxdupont

changelog_unreleased/4443-fqdn-option-flags-lenient-parsing

@@ -1,4 +1,4 @@
 [func]		fdupont
 	Extended lenient parsing of v4 "fqdn" and v6 "client-fqdn"
-	options to skip options with bad flags.
+	options to fix options with bad flags.
 	(Gitlab #4443)

doc/sphinx/arm/dhcp4-srv.rst

@@ -8872,7 +8872,8 @@ or in terms of the log message above, the tuple length ``y`` becomes ``x``.
 
 Starting with Kea version 2.5.8, this parsing is extended to silently ignore
 FQDN (81) options with some invalid domain names, and starting with Kea
-version 3.1.9 with invalid flags (i.e. 'S" and 'N' flags set to 1).
+version 3.1.9 to fix invalid flags, i.e. when 'S' and 'N' flags set to 1
+the 'N' flag is reset to 0 for compatibility with ISC DHCP behavior.
 
 Ignore DHCP Server Identifier
 -----------------------------

doc/sphinx/arm/dhcp6-srv.rst

@@ -8740,7 +8740,8 @@ MiNID.
 
 Starting with Kea version 2.5.8, this parsing is extended to silently ignore
 client-fqdn (39) options with some invalid domain names, and starting with Kea
-version	3.1.9 with invalid flags (i.e. 'S" and 'N' flags set to 1).
+version 3.1.9 to fix invalid flags, i.e. when 'S' and 'N' flags set to 1
+the 'N' flag is reset to 0 for compatibility with ISC DHCP behavior.
 
 .. _dhcp6_allocation_strategies:
 

src/lib/dhcp/option4_client_fqdn.cc

@@ -153,7 +153,7 @@ Option4ClientFqdnImpl::Option4ClientFqdnImpl(OptionBufferConstIter first,
         checkFlags(flags_, false);
     } catch (const InvalidOption4FqdnFlags& ex) {
         if (Option::lenient_parsing_) {
-            isc_throw(SkipThisOptionError, ex.what());
+            flags_ &= ~Option4ClientFqdn::FLAG_N;
         } else {
             throw;
         }
@@ -521,7 +521,7 @@ Option4ClientFqdn::unpack(OptionBufferConstIter first,
         impl_->checkFlags(impl_->flags_, false);
     } catch (const InvalidOption4FqdnFlags& ex) {
         if (Option::lenient_parsing_) {
-            isc_throw(SkipThisOptionError, ex.what());
+            impl_->flags_ &= ~Option4ClientFqdn::FLAG_N;
         } else {
             throw;
         }

src/lib/dhcp/option6_client_fqdn.cc

@@ -126,7 +126,7 @@ Option6ClientFqdnImpl::Option6ClientFqdnImpl(OptionBufferConstIter first,
         checkFlags(flags_, false);
     } catch (const InvalidOption6FqdnFlags& ex) {
         if (Option::lenient_parsing_) {
-            isc_throw(SkipThisOptionError, ex.what());
+            flags_ &= ~Option6ClientFqdn::FLAG_N;
         } else {
             throw;
         }
@@ -441,7 +441,7 @@ Option6ClientFqdn::unpack(OptionBufferConstIter first,
         impl_->checkFlags(impl_->flags_, false);
     } catch (const InvalidOption6FqdnFlags& ex) {
         if (Option::lenient_parsing_) {
-            isc_throw(SkipThisOptionError, ex.what());
+            impl_->flags_ &= ~Option6ClientFqdn::FLAG_N;
         } else {
             throw;
         }

src/lib/dhcp/tests/option4_client_fqdn_unittest.cc

@@ -462,9 +462,13 @@ TEST(Option4ClientFqdnTest, constructFromWireInvalidFlags) {
     LenientOptionParsing lop(false);
     EXPECT_THROW(Option4ClientFqdn(in_buf.begin(), in_buf.end()),
                  InvalidOption4FqdnFlags);
+    // Lenient parsing allows this but reset the N bit.
     Option::lenient_parsing_ = true;
-    EXPECT_THROW(Option4ClientFqdn(in_buf.begin(), in_buf.end()),
-                 SkipThisOptionError);
+    boost::scoped_ptr<Option4ClientFqdn> option;
+    EXPECT_NO_THROW(option.reset(new Option4ClientFqdn(in_buf.begin(), in_buf.end())));
+    ASSERT_TRUE(option);
+    EXPECT_TRUE(option->getFlag(Option4ClientFqdn::FLAG_S));
+    EXPECT_FALSE(option->getFlag(Option4ClientFqdn::FLAG_N));
 }
 
 // This test verifies that if invalid domain name is used the constructor
@@ -844,9 +848,11 @@ TEST(Option4ClientFqdnTest, unpack) {
     LenientOptionParsing lop(false);
     EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
                  InvalidOption4FqdnFlags);
+    // Lenient parsing allows bad flags but reset the N bit.
     Option::lenient_parsing_ = true;
-    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
-                 SkipThisOptionError);
+    ASSERT_NO_THROW(option->unpack(in_buf.begin(), in_buf.end()));
+    EXPECT_TRUE(option->getFlag(Option4ClientFqdn::FLAG_S));
+    EXPECT_FALSE(option->getFlag(Option4ClientFqdn::FLAG_N));
 }
 
 // This test verifies that on-wire option data holding partial domain name

src/lib/dhcp/tests/option6_client_fqdn_unittest.cc

@@ -413,9 +413,13 @@ TEST(Option6ClientFqdnTest, constructFromWireInvalidFlags) {
     LenientOptionParsing lop(false);
     EXPECT_THROW(Option6ClientFqdn(in_buf.begin(), in_buf.end()),
                  InvalidOption6FqdnFlags);
+    // Lenient parsing allows this but reset the N bit.
     Option::lenient_parsing_ = true;
-    EXPECT_THROW(Option6ClientFqdn(in_buf.begin(), in_buf.end()),
-                 SkipThisOptionError);
+    boost::scoped_ptr<Option6ClientFqdn> option;
+    EXPECT_NO_THROW(option.reset(new Option6ClientFqdn(in_buf.begin(), in_buf.end())));
+    ASSERT_TRUE(option);
+    EXPECT_TRUE(option->getFlag(Option6ClientFqdn::FLAG_S));
+    EXPECT_FALSE(option->getFlag(Option6ClientFqdn::FLAG_N));
 }
 
 // This test verifies that if invalid domain name is used the constructor
@@ -712,9 +716,11 @@ TEST(Option6ClientFqdnTest, unpack) {
     LenientOptionParsing lop(false);
     EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
                  InvalidOption6FqdnFlags);
+    // Lenient parsing allows bad flags but reset the N bit.
     Option::lenient_parsing_ = true;
-    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
-                 SkipThisOptionError);
+    ASSERT_NO_THROW(option->unpack(in_buf.begin(), in_buf.end()));
+    EXPECT_TRUE(option->getFlag(Option6ClientFqdn::FLAG_S));
+    EXPECT_FALSE(option->getFlag(Option6ClientFqdn::FLAG_N));
 }
 
 // This test verifies that on-wire option data holding partial domain name