[developer] Part 1 -> Review of 5198 & 5199 (ipf per-zone global-zone settings)

Discussion:

Dan McDonald via illumos-developer

2014-10-06 18:33:43 UTC

Back in the old days, this would merit an ARC case. It took me a couple of reading-the-man-page diffs and the review mail to understand it completely.

ANYWAY, on to the code...

usr/src/cmd/ipf/lib/common/load_hash.c
usr/src/cmd/ipf/lib/common/load_hashnode.c
usr/src/cmd/ipf/lib/common/load_pool.c
usr/src/cmd/ipf/lib/common/load_poolnode.c
usr/src/cmd/ipf/tools/Makefile.tools
usr/src/cmd/ipf/tools/ipfs.c
usr/src/cmd/ipf/tools/ipfstat.c

* Looks good, no comments needed.

usr/src/cmd/ipf/tools/ipf.c
usr/src/man/man1m/ipf.1m
usr/src/man/man1m/ipfs.1m
usr/src/man/man1m/ipfstat.1m
usr/src/man/man1m/ipmon.1m
usr/src/man/man1m/ipnat.1m
usr/src/man/man1m/ippool.1m
usr/src/cmd/ipf/tools/ipmon.c
usr/src/cmd/ipf/tools/ipnat.c
usr/src/cmd/ipf/tools/ippool.c

* I'm very disappointed that this one is different from the others. This may sound silly, but did you consider making all of the other commands behave like this one (zonename at the end, use or disuse of -G to determine with ngz ruleset)? I'm worried about UI consistency across the ipfilter commands, but perhaps I shouldn't be?

* This inconsistency is more problematic because other commands reference the "Zones" section in ipf(1M), which is the exception.

The following files will come in another mail, as I want to focus on their implementations, not their overall architecture.

usr/src/cmd/ipf/tools/ipfzone.c
usr/src/cmd/ipf/tools/ipfzone.h
usr/src/uts/common/inet/ipf/fil.c
usr/src/uts/common/inet/ipf/ip_fil_solaris.c
usr/src/uts/common/inet/ipf/ip_log.c
usr/src/uts/common/inet/ipf/ip_state.c
usr/src/uts/common/inet/ipf/netinet/ip_fil.h
usr/src/uts/common/inet/ipf/netinet/ipf_stack.h
usr/src/uts/common/inet/ipf/solaris.c
usr/src/uts/intel/ipf/ipf.global-objs.debug64
usr/src/uts/sparc/ipf/ipf.global-objs.debug64

Dan

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Rob Gulewich via illumos-developer

2014-10-07 04:01:27 UTC

Permalink

Hi Dan,

Post by Dan McDonald via illumos-developer
Back in the old days, this would merit an ARC case. It took me a couple of reading-the-man-page diffs and the review mail to understand it completely.

Is there a standard place to document changes like this? I'd be happy
to add an overview and explanation to the ticket at the very least,
if that's the appropriate place.

Post by Dan McDonald via illumos-developer
* I'm very disappointed that this one is different from the others. This may sound silly, but did you consider making all of the other commands behave like this one (zonename at the end, use or disuse of -G to determine with ngz ruleset)? I'm worried about UI consistency across the ipfilter commands, but perhaps I shouldn't be?

Fair point - I went back and forth on this quite a bit. My intention
was to make the ipf commands consistent with other illumos commands that
already use "-z <zonename>" to operate on a specific zone. The problem
with making all of the commands use -z is that ipf(1m) already has a -z
option. The problem with making all of them take a zone name as the last
argument is that ipmon(1m) already takes an optional argument. In other
words, I couldn't have the same UI for all commands - either ipf or
ipmon would have to be inconsistent.

Faced with this choice, I ended up going with -z for consistency with
other non-ipf commands. If folks feel strongly about this, I'd consider
going the other way - this admittedly isn't great. If there's a nicer,
more consistent solution here, I'd love to hear it.

Post by Dan McDonald via illumos-developer
* This inconsistency is more problematic because other commands reference the "Zones" section in ipf(1M), which is the exception.

Yikes - if I end up keeping the behaviour above, I'll update the man
page to make this clearer.

Thanks,

-- Rob

--
Rob Gulewich, Joyent (http://www.joyent.com)

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Dan McDonald via illumos-developer

2014-10-07 17:43:13 UTC

Permalink

Post by Rob Gulewich via illumos-developer
Hi Dan,

Post by Dan McDonald via illumos-developer
Back in the old days, this would merit an ARC case. It took me a couple of reading-the-man-page diffs and the review mail to understand it completely.

Is there a standard place to document changes like this? I'd be happy
to add an overview and explanation to the ticket at the very least,
if that's the appropriate place.

That's honestly as good as any place. I think there's a bigger question to be answered about larger-project design documentation, but for now, the bug report is as good a place as any. You at Joyent, and Nexenta, are likely to be the most prolific contributors of new materials, because you both have large staffs and resources to back them up. (E.g. SMB work, LX zones, etc.)

Post by Rob Gulewich via illumos-developer

I suspected as much. So I'm not going to be too concerned, except for what I mention later.

Post by Rob Gulewich via illumos-developer
The problem with making all of them take a zone name as the last
argument is that ipmon(1m) already takes an optional argument. In other
words, I couldn't have the same UI for all commands - either ipf or
ipmon would have to be inconsistent.
Faced with this choice, I ended up going with -z for consistency with
other non-ipf commands. If folks feel strongly about this, I'd consider
going the other way - this admittedly isn't great. If there's a nicer,
more consistent solution here, I'd love to hear it.

I can't think of a better idea. Having ipf(1M) itself be the exception is okay, as long as it's clear that ipf(1M) is the exception. What you just wrote above is EXACTLY the sort of text that belongs in a repository of design materials. Thank you - you're a copy-and-paste away from satisfying me on this front.

Post by Rob Gulewich via illumos-developer

Post by Dan McDonald via illumos-developer
* This inconsistency is more problematic because other commands reference the "Zones" section in ipf(1M), which is the exception.

Yikes - if I end up keeping the behaviour above, I'll update the man
page to make this clearer.

Excellent. I just want something that doesn't add to the confusion that your external constraints already impose upon you.

The ipfilter(5) man page may be a good place to drop overarching documentation about the GZ-controllable non-GZ rulesets. You can cover command inconsistencies there as well.

Thank you for your thorough answers, my concerns with part 1 will be addressed with proper documentation updates both in the bug report and in the man pages.

Dan

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Rob Gulewich via illumos-developer

2014-10-08 21:01:42 UTC

Permalink

Post by Dan McDonald via illumos-developer
I can't think of a better idea. Having ipf(1M) itself be the exception is okay, as long as it's clear that ipf(1M) is the exception. What you just wrote above is EXACTLY the sort of text that belongs in a repository of design materials. Thank you - you're a copy-and-paste away from satisfying me on this front.

OK - added that note and an overview to the ticket:
https://www.illumos.org/issues/5198

Post by Dan McDonald via illumos-developer
The ipfilter(5) man page may be a good place to drop overarching documentation about the GZ-controllable non-GZ rulesets. You can cover command inconsistencies there as well.

Here's an updated webrev:
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.html

And here's one with just the changes from your feedback:
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5197-1/index.html

I added an overview of how the GZ-controlled rulesets work to the ipfilter(5)
man page. I documented the argument inconsistencies in ipf(1m), since it seemed
out of place in ipfilter(5), which is more of a general overview.

-- Rob

--
Rob Gulewich, Joyent (http://www.joyent.com)

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Dan McDonald via illumos-developer

2014-10-09 15:31:31 UTC

Permalink

Post by Rob Gulewich via illumos-developer

https://www.illumos.org/issues/5198

Thank you, just what the doctor ordered.

Post by Rob Gulewich via illumos-developer

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.html
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5197-1/index.html

Thank you for the incremental webrev!

Post by Rob Gulewich via illumos-developer
I added an overview of how the GZ-controlled rulesets work to the ipfilter(5)
man page. I documented the argument inconsistencies in ipf(1m), since it seemed
out of place in ipfilter(5), which is more of a general overview.

I agree with both decisions you made. One small nit: in the ipfilter(5) man page, you talk about a three-layer approach, but I only see two. Are you counting the External Source as a layer? If so... well, I'll bite my tongue. I generally disdain middleboxes. If not, and this was a typo and you meant to say two-layer, please fix it.

Otherwise, MUCH better.

Dan

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Rob Gulewich via illumos-developer

2014-10-09 17:42:30 UTC

Permalink

Post by Dan McDonald via illumos-developer
I agree with both decisions you made. One small nit: in the ipfilter(5) man page, you talk about a three-layer approach, but I only see two. Are you counting the External Source as a layer? If so... well, I'll bite my tongue. I generally disdain middleboxes. If not, and this was a typo and you meant to say two-layer, please fix it.

The "three-layer" wording was in the ipfilter(5) man page before - it's
meant to refer to the "Global Override" / "Network Services" / "Global
Default" bits in that section. I just added the "In a given zone" to
try and clarify that the three layers are within a zone, not
system-wide.

-- Rob

--
Rob Gulewich, Joyent (http://www.joyent.com)

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com