Makes IPSet a fmt.Stringer. #210

schultz-is · 2021-07-26T19:23:48Z

Addresses: #193

dsnet · 2021-07-26T19:33:47Z

For all the other types, the XXX.String method is usable with associated ParseXXX, XXX.MarshalText, and XXX.UnmarshalText functionality.

It seems that we should add the full set of behavior that the other types have, but that probably requires committing to a stable serialization format.

schultz-is · 2021-07-26T19:37:27Z

Yeah, I was also considering updating the IPRange.String method to collapse ranges with the same to and from values, but figured that would have implications on parsing. I'm happy to modify this PR to accomodate and include whatever parsing format y'all would like.

josharian · 2021-07-27T17:56:31Z

The format proposed here (comma-separated list of IPRanges, all required to be valid) seems fine to me. None of the other types have a type description wrapper, though, so for consistency we should drop the IPSet(/).

josharian · 2021-07-27T17:56:57Z

Also, if/when we're ready to move forward with this change, it'll need tests.

schultz-is · 2021-07-27T23:57:46Z

That all sounds good to me. Couple questions for y'all:

How do y'all feel about trimming whitespace when unmarshaling? e.g. ParseIPSet("10.0.0.1-10.0.0.2, 10.1.0.1-10.1.0.2")
Should single IPs and prefixes be allowed in the string representation? I ask because the IPSetBuilder allows for these, and it's a little verbose to represent a single IP as a range.

dsnet · 2021-07-28T00:09:31Z

I'm hardly authoritative here, but here are my opinions.

How do y'all feel about trimming whitespace when unmarshaling?

I'd lightly argue against this for now:

We would then have to decide what is valid whitespace. Is it just space (U+0020) or everything under unicode.IsSpace?
The other ParseXXX functions don't seem to ignore whitespace.

Allowing whitespace is something that should considered holistically for all the types. You can imagine arguing that whitespace around the - in the IPRange would be visually nicer.

Should single IPs and prefixes be allowed in the string representation?

Personally, I think a single IP for a single address IPRange is nice, but I defer to others.

schultz-is · 2021-07-28T00:20:24Z

Ah yeah, those are good points about the whitespace. I was going to just lean on the strings.TrimSpace, but it's probably best to be explicit. Was also thinking this was a unique case and didn't consider spacing around the IPRange hyphen. I'll make this super simple for a first pass and add in fancy features in a followup if people would like them.

dsnet · 2021-07-28T00:23:24Z

I was going to just lean on the strings.TrimSpace

For future consideration: strings.TrimSpace uses unicode.IsSpace under the hood. One reason why I wouldn't recommend that definition is that it allows for the use of newlines (U+000A). I'd expect all of the strings produced by the types in this package to be a single line.

schultz-is · 2021-07-28T18:51:50Z

Added in ParseIPSet, MustParseIPSet, IPSet.MarshalText, and IPSet.UnmarshalText along with test coverage. Took the approach of bailing on first error during parsing. If y'all would rather follow the IPSetBuilder pattern of error accumulation and partial set return, that's possible, too.

A tangential thing I encountered when testing is that the IPSet.Equal method seems to take order into account. Not sure if that's intended behavior.

josharian · 2021-07-28T19:18:57Z

Took the approach of bailing on first error during parsing

SGTM

the IPSet.Equal method seems to take order into account.

From the code:

type IPSet struct {
	// rr is the set of IPs that belong to this IPSet. The IPRanges
	// are normalized according to IPSetBuilder.normalize, meaning
	// they are a sorted, minimal representation (no overlapping
	// ranges, no contiguous ranges). The implementation of various
	// methods rely on this property.
	rr []IPRange
}

Any other way of constructing an IPSet (Parse/Unmarshal) will need to normalize rr.

schultz-is · 2021-07-28T19:23:35Z

Right on, that'd explain it. I ran into it when I was manually constructing IPSets in tests. These Parse and Unmarshal methods are using the IPSetBuilder under the hood, so the results will be normalized.

josharian

First round of feedback. I'll want @bradfitz or @danderson to also weigh in before submitting, since it is new API (albeit parallel to other APIs).

Want to add fuzz support and do a bit of fuzzing? This kind of code is a prime fuzz target.

ipset.go

josharian · 2021-07-28T19:34:55Z

ipset.go

+	for _, rs := range rss {
+		r, err = ParseIPRange(rs)
+		if err != nil {
+			return &IPSet{}, fmt.Errorf("invalid IP range %q in set %q", rs, s)


Let's not repeat the entire input. Instead, let's tell them what's wrong with the input. Something like fmt.Errorf("invalid IP range %q: %w", rs, err).

Can do. Was mostly hesitant to wrap it because I didn't see wrapping used elsewhere.

josharian · 2021-07-28T19:39:16Z

ipset.go

+func (s IPSet) String() string {
+	var b strings.Builder
+	for i, r := range s.rr {
+		b.WriteString(r.String())


You'll allocate less if you use r.AppendTo with a buffer that you re-use across iterations.

var buf []byte for i, r := range s.rr { buf = r.AppendTo(buf[:0) b.Write(buf) // ... }

Great catch.

Bypassed the use of the strings.Builder altogether, since it's just using a []byte internally anyway.

strings.Builder will probably eliminate an allocation, because the internal []byte can be unsafely re-used as the resulting string. If you're curious, add a benchmark? (It probably doesn't matter in practice.)

Hey neat, you were absolutely right about the extra allocation. Sometimes I forget that stdlib doesn't stray away from unsafe. Thanks for the insight!

ipset.go

josharian · 2021-07-28T19:59:16Z

ipset.go

+		}
+		b.AddRange(r)
+	}
+	return b.IPSet()


I'm somewhat inclined to be defensive and return nil, err when b.IPSet() returns a non-nil error. I don't think it's possible now, but maybe that could change. And ISTM that ParseIPSet should be all-or-none. What do you think?

A nil, err return definitely feels most idiomatic, especially in the case of an all-or-nothing action. I'm struggling to come up with a use case where the resulting empty set would be put to valid use in case of an error.

Went ahead and changed it to a nil, err return, but am happy to change it back if desired.

I don't see that change reflected here.

josharian

Last comments from me. After this, leaving for @bradfitz, since it is adding new (if parallel) API.

josharian · 2021-07-29T19:59:08Z

ipset.go

+		}
+		b.AddRange(r)
+	}
+	return b.IPSet()


I don't see that change reflected here.

josharian · 2021-07-29T20:00:27Z

ipset.go

+
+// String returns a string representation of the IPSet.
+func (s IPSet) String() string {
+	var (


Please do

var b strings.Builder var buf []byte

Factored variable declarations are fairly rare in practice, so this reads as if you're telling us that b and buf are linked in some important and unusual way. And in this case, spelling out "var" twice takes us from 4 LOC to 2.

Added in an err check against the IPSetBuilder.IPSet call in the ParseIPSet function in order to be explicit about the return value. Also condensed the var declarations in the String method. I'd not heard of parens-wrapped var declarations implying an relationship before, but now that you mention it, it makes total sense. Plus it feels easier to read this way somehow.

josharian · 2021-07-29T20:01:36Z

Oh, and thanks for doing this!

schultz-is · 2021-07-30T02:44:02Z

Oh, and thanks for doing this!

Hey no problem at all. It's always nice to collaborate with other folks and learn how they write code. I appreciate the feedback y'all have provided so far!

Want to add fuzz support and do a bit of fuzzing? This kind of code is a prime fuzz target.

Also, happy to do this in the near future. Been looking for a reason to mess around with go-fuzz.

schultz-is · 2021-07-30T19:36:56Z

Ah, didn't read the go.mod version declaration before using the error wrapping fmt syntax in the ParseIPSet method. Would y'all prefer I just change that to an %s? Can also make an error-satisfying wrapper struct that contains the root error in a field if preserving the cause is desired.

josharian · 2021-08-02T16:14:45Z

Ah, didn't read the go.mod version declaration before using the error wrapping fmt syntax in the ParseIPSet method.

I forgot about that too. :) %s is fine, thanks.

Addresses: #193 Signed-off-by: Matt Schultz <[email protected]>

josharian reviewed Jul 28, 2021

View reviewed changes

josharian reviewed Jul 29, 2021

View reviewed changes

Makes IPSet a fmt.Stringer.

2b5f5f0

Addresses: #193 Signed-off-by: Matt Schultz <[email protected]>

schultz-is closed this by deleting the head repository Oct 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes IPSet a fmt.Stringer. #210

Makes IPSet a fmt.Stringer. #210

schultz-is commented Jul 26, 2021

dsnet commented Jul 26, 2021

schultz-is commented Jul 26, 2021

josharian commented Jul 27, 2021

josharian commented Jul 27, 2021

schultz-is commented Jul 27, 2021

dsnet commented Jul 28, 2021

schultz-is commented Jul 28, 2021 •

edited

Loading

dsnet commented Jul 28, 2021

schultz-is commented Jul 28, 2021

josharian commented Jul 28, 2021 •

edited

Loading

schultz-is commented Jul 28, 2021

josharian left a comment

josharian Jul 28, 2021

schultz-is Jul 28, 2021

josharian Jul 28, 2021

schultz-is Jul 28, 2021

schultz-is Jul 28, 2021

josharian Jul 28, 2021 •

edited

Loading

schultz-is Jul 28, 2021

josharian Jul 28, 2021

schultz-is Jul 28, 2021

schultz-is Jul 28, 2021

josharian Jul 29, 2021

josharian left a comment

josharian Jul 29, 2021

josharian Jul 29, 2021

schultz-is Jul 30, 2021

josharian commented Jul 29, 2021

schultz-is commented Jul 30, 2021 •

edited

Loading

schultz-is commented Jul 30, 2021

josharian commented Aug 2, 2021

Makes IPSet a fmt.Stringer. #210

Makes IPSet a fmt.Stringer. #210

Conversation

schultz-is commented Jul 26, 2021

dsnet commented Jul 26, 2021

schultz-is commented Jul 26, 2021

josharian commented Jul 27, 2021

josharian commented Jul 27, 2021

schultz-is commented Jul 27, 2021

dsnet commented Jul 28, 2021

schultz-is commented Jul 28, 2021 • edited Loading

dsnet commented Jul 28, 2021

schultz-is commented Jul 28, 2021

josharian commented Jul 28, 2021 • edited Loading

schultz-is commented Jul 28, 2021

josharian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josharian Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josharian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josharian commented Jul 29, 2021

schultz-is commented Jul 30, 2021 • edited Loading

schultz-is commented Jul 30, 2021

josharian commented Aug 2, 2021

schultz-is commented Jul 28, 2021 •

edited

Loading

josharian commented Jul 28, 2021 •

edited

Loading

josharian Jul 28, 2021 •

edited

Loading

schultz-is commented Jul 30, 2021 •

edited

Loading