Merge pull request #47497 from robmry/resolvconf_fixes

Fix 'resolv.conf' parsing issues

libnetwork/internal/resolvconf/resolvconf.go

@@ -291,21 +291,17 @@ func (rc *ResolvConf) TransformForIntNS(
 		rc.md.UsedDefaultNS = true
 	}
 
-	// Validate the ndots option from host config or overrides, if present.
+	// For each option required by the nameserver, add it if not already present. If
-	// TODO(robmry) - pre-existing behaviour, but ...
+	// the option is already present, don't override it. Apart from ndots - if the
-	//   Validating ndots from an override is good, but not-liking something in the
+	// ndots value is invalid and an ndots option is required, replace the existing
-	//   host's resolv.conf isn't a reason to fail - just remove? (And it'll be
+	// value.
-	//   replaced by the value in reqdOptions, if given.)
-	if ndots, exists := rc.Option("ndots"); exists {
-		if n, err := strconv.Atoi(ndots); err != nil || n < 0 {
-			return nil, errdefs.InvalidParameter(
-				fmt.Errorf("invalid number for ndots option: %v", ndots))
-		}
-	}
-	// For each option required by the nameserver, add it if not already
-	// present (if the option already has a value don't change it).
 	for _, opt := range reqdOptions {
 		optName, _, _ := strings.Cut(opt, ":")
+		if optName == "ndots" {
+			rc.options = removeInvalidNDots(rc.options)
+			// No need to update rc.md.NDotsFrom, if there is no ndots option remaining,
+			// it'll be set to "internal" when the required value is added.
+		}
 		if _, exists := rc.Option(optName); !exists {
 			rc.AddOption(opt)
 		}
@@ -450,18 +446,8 @@ func UserModified(rcPath, rcHashPath string) (bool, error) {
 func (rc *ResolvConf) processLine(line string) {
 	fields := strings.Fields(line)
 
-	// Strip comments.
+	// Strip blank lines and comments.
-	// TODO(robmry) - ignore comment chars except in column 0.
+	if len(fields) == 0 || fields[0][0] == '#' || fields[0][0] == ';' {
-	//   This preserves old behaviour, but it's wrong. For example, resolvers
-	//   will honour the option in line "options # ndots:0" (and ignore the
-	//   "#" as an unknown option).
-	for i, s := range fields {
-		if s[0] == '#' || s[0] == ';' {
-			fields = fields[:i]
-			break
-		}
-	}
-	if len(fields) == 0 {
 		return
 	}
 
@@ -488,10 +474,8 @@ func (rc *ResolvConf) processLine(line string) {
 		if len(fields) < 2 {
 			return
 		}
-		// Replace options from earlier directives.
+		// Accumulate options.
-		// TODO(robmry) - preserving incorrect behaviour, options should accumulate.
+		rc.options = append(rc.options, fields[1:]...)
-		//     rc.options = append(rc.options, fields[1:]...)
-		rc.options = fields[1:]
 	default:
 		// Copy anything that's not a recognised directive.
 		rc.other = append(rc.other, line)
@@ -506,3 +490,22 @@ func defaultNSAddrs(ipv6 bool) []netip.Addr {
 	}
 	return addrs
 }
+
+// removeInvalidNDots filters ill-formed "ndots" settings from options.
+// The backing array of the options slice is reused.
+func removeInvalidNDots(options []string) []string {
+	n := 0
+	for _, opt := range options {
+		k, v, _ := strings.Cut(opt, ":")
+		if k == "ndots" {
+			ndots, err := strconv.Atoi(v)
+			if err != nil || ndots < 0 {
+				continue
+			}
+		}
+		options[n] = opt
+		n++
+	}
+	clear(options[n:]) // Zero out the obsolete elements, for GC.
+	return options[:n]
+}

libnetwork/internal/resolvconf/resolvconf_test.go

@@ -500,6 +500,60 @@ func TestRCTransformForIntNS(t *testing.T) {
 	}
 }
 
+// Check that invalid ndots options in the host's file are ignored, unless
+// starting the internal resolver (which requires an ndots option), in which
+// case invalid ndots should be replaced.
+func TestRCTransformForIntNSInvalidNdots(t *testing.T) {
+	testcases := []struct {
+		name         string
+		options      string
+		reqdOptions  []string
+		expVal       string
+		expOptions   []string
+		expNDotsFrom string
+	}{
+		{
+			name:         "Negative value",
+			options:      "options ndots:-1",
+			expOptions:   []string{"ndots:-1"},
+			expVal:       "-1",
+			expNDotsFrom: "host",
+		},
+		{
+			name:         "Invalid values with reqd ndots",
+			options:      "options ndots:-1 foo:bar ndots ndots:",
+			reqdOptions:  []string{"ndots:2"},
+			expVal:       "2",
+			expNDotsFrom: "internal",
+			expOptions:   []string{"foo:bar", "ndots:2"},
+		},
+		{
+			name:         "Valid value with reqd ndots",
+			options:      "options ndots:1 foo:bar ndots ndots:",
+			reqdOptions:  []string{"ndots:2"},
+			expVal:       "1",
+			expNDotsFrom: "host",
+			expOptions:   []string{"ndots:1", "foo:bar"},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			content := "nameserver 8.8.8.8\n" + tc.options
+			rc, err := Parse(bytes.NewBuffer([]byte(content)), "/etc/resolv.conf")
+			assert.NilError(t, err)
+			_, err = rc.TransformForIntNS(false, netip.MustParseAddr("127.0.0.11"), tc.reqdOptions)
+			assert.NilError(t, err)
+
+			val, found := rc.Option("ndots")
+			assert.Check(t, is.Equal(found, true))
+			assert.Check(t, is.Equal(val, tc.expVal))
+			assert.Check(t, is.Equal(rc.md.NDotsFrom, tc.expNDotsFrom))
+			assert.Check(t, is.DeepEqual(rc.options, tc.expOptions))
+		})
+	}
+}
+
 func TestRCRead(t *testing.T) {
 	d := t.TempDir()
 	path := filepath.Join(d, "resolv.conf")

libnetwork/resolvconf/resolvconf_unix_test.go

@@ -149,16 +149,16 @@ func TestGetSearchDomains(t *testing.T) {
 			result: []string{"example.com"},
 		},
 		{
-			input:  `search example.com # ignored`,
+			input:  `search example.com # notignored`,
-			result: []string{"example.com"},
+			result: []string{"example.com", "#", "notignored"},
 		},
 		{
 			input:  `	  search	 example.com	  `,
 			result: []string{"example.com"},
 		},
 		{
-			input:  `	  search	 example.com	  # ignored`,
+			input:  `	  search	 example.com	  # notignored`,
-			result: []string{"example.com"},
+			result: []string{"example.com", "#", "notignored"},
 		},
 		{
 			input:  `search foo.example.com example.com`,
@@ -169,8 +169,8 @@ func TestGetSearchDomains(t *testing.T) {
 			result: []string{"foo.example.com", "example.com"},
 		},
 		{
-			input:  `	   search	   foo.example.com	 example.com	# ignored`,
+			input:  `	   search	   foo.example.com	 example.com	# notignored`,
-			result: []string{"foo.example.com", "example.com"},
+			result: []string{"foo.example.com", "example.com", "#", "notignored"},
 		},
 		{
 			input: `nameserver 1.2.3.4
@@ -212,6 +212,9 @@ func TestGetOptions(t *testing.T) {
 		{
 			input: `# ignored`,
 		},
+		{
+			input: `; ignored`,
+		},
 		{
 			input: `nameserver 1.2.3.4`,
 		},
@@ -220,32 +223,36 @@ func TestGetOptions(t *testing.T) {
 			result: []string{"opt1"},
 		},
 		{
-			input:  `options opt1 # ignored`,
+			input:  `options opt1 # notignored`,
-			result: []string{"opt1"},
+			result: []string{"opt1", "#", "notignored"},
+		},
+		{
+			input:  `options opt1 ; notignored`,
+			result: []string{"opt1", ";", "notignored"},
 		},
 		{
 			input:  `	  options	 opt1	  `,
 			result: []string{"opt1"},
 		},
 		{
-			input:  `	  options	 opt1	  # ignored`,
+			input:  `	  options	 opt1	  # notignored`,
-			result: []string{"opt1"},
+			result: []string{"opt1", "#", "notignored"},
 		},
 		{
 			input:  `options opt1 opt2 opt3`,
 			result: []string{"opt1", "opt2", "opt3"},
 		},
 		{
-			input:  `options opt1 opt2 opt3 # ignored`,
+			input:  `options opt1 opt2 opt3 # notignored`,
-			result: []string{"opt1", "opt2", "opt3"},
+			result: []string{"opt1", "opt2", "opt3", "#", "notignored"},
 		},
 		{
 			input:  `	   options	 opt1	 opt2	 opt3	`,
 			result: []string{"opt1", "opt2", "opt3"},
 		},
 		{
-			input:  `	   options	 opt1	 opt2	 opt3	# ignored`,
+			input:  `	   options	 opt1	 opt2	 opt3	# notignored`,
-			result: []string{"opt1", "opt2", "opt3"},
+			result: []string{"opt1", "opt2", "opt3", "#", "notignored"},
 		},
 		{
 			input: `nameserver 1.2.3.4
@@ -256,7 +263,7 @@ options opt1 opt2 opt3`,
 			input: `nameserver 1.2.3.4
 options opt1 opt2
 options opt3 opt4`,
-			result: []string{"opt3", "opt4"},
+			result: []string{"opt1", "opt2", "opt3", "opt4"},
 		},
 	} {
 		test := GetOptions([]byte(tc.input))

libnetwork/sandbox_dns_unix_test.go

@@ -77,11 +77,19 @@ func TestDNSOptions(t *testing.T) {
 	err = sb2.setupDNS()
 	assert.NilError(t, err)
 	err = sb2.rebuildDNS()
-	assert.Error(t, err, "invalid number for ndots option: foobar")
+	assert.NilError(t, err)
+	currRC, err = resolvconf.GetSpecific(sb2.config.resolvConfPath)
+	assert.NilError(t, err)
+	dnsOptionsList = resolvconf.GetOptions(currRC.Content)
+	assert.Check(t, is.DeepEqual([]string{"ndots:0"}, dnsOptionsList))
 
 	sb2.config.dnsOptionsList = []string{"ndots:-1"}
 	err = sb2.setupDNS()
 	assert.NilError(t, err)
 	err = sb2.rebuildDNS()
-	assert.Error(t, err, "invalid number for ndots option: -1")
+	assert.NilError(t, err)
+	currRC, err = resolvconf.GetSpecific(sb2.config.resolvConfPath)
+	assert.NilError(t, err)
+	dnsOptionsList = resolvconf.GetOptions(currRC.Content)
+	assert.Check(t, is.DeepEqual([]string{"ndots:0"}, dnsOptionsList))
 }