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

Discussion:

Dan McDonald via illumos-developer

2014-10-06 19:22:39 UTC

NOTE: To be fair, I'm not sure how much of what I'm complaining about here is you just dealing with what you inherited. My apologies if anything I say here is shooting the messenger.

One last overarching architectural question --> How does one store /etc/ipf/* entries for the GZ-controlled non-GZ netstacks? I know you SmartOS folks use SMF and other oddness to affect these changes, but not everyone treats their global zone like SmartOS does.

Anyway, on to the code:

usr/src/uts/intel/ipf/ipf.global-objs.debug64
usr/src/uts/sparc/ipf/ipf.global-objs.debug64
usr/src/cmd/ipf/tools/ipfzone.h
usr/src/uts/common/inet/ipf/fil.c

* Looks good, no comments needed.

usr/src/uts/common/inet/ipf/netinet/ip_fil.h

* Even though it's u_32_t, ipfz_gz is essentially a boolean, right?

usr/src/cmd/ipf/tools/ipfzone.c

* Seems plausible. I had to look at ip_fil.h though, first.

* After seeing this, the diffs to ipf.c vs. the rest are even more confusing to me as a possible maintainer, never mind as a potential administrator.

usr/src/uts/common/inet/ipf/ip_fil_solaris.c

* Lines 1043-1073 & 603-645 look an awful lot alike. Can you possibly factor them out?

* maybe Lines 1126-1154 as well?!

usr/src/uts/common/inet/ipf/ip_log.c

* is it just me, or is this a completely different bug?

usr/src/uts/common/inet/ipf/ip_state.c

* I thought I didn't go to the omnibus webrev, but this I already reviewed. :)

usr/src/uts/common/inet/ipf/netinet/ipf_stack.h

* "pgz"? Does this name have to be that opaque? At least a comment here...

usr/src/uts/common/inet/ipf/solaris.c

* Okay, some of my previous comments may be obviated by the introductory text here.

Thanks!
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:06:52 UTC

Permalink

Hi Dan (Part 2),

Post by Dan McDonald via illumos-developer
One last overarching architectural question --> How does one store /etc/ipf/* entries for the GZ-controlled non-GZ netstacks? I know you SmartOS folks use SMF and other oddness to affect these changes, but not everyone treats their global zone like SmartOS does.

Good question - in SmartOS, we store a zone's GZ-controlled ipf.conf on
the zone's ZFS dataset (/zones/<zonename>/config/ipf.conf, to be exact).
The storing of configuration felt distribution-specific to me. Any
distribution would probably want to update the man pages with the location
of these conf files, though.

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/netinet/ip_fil.h
* Even though it's u_32_t, ipfz_gz is essentially a boolean, right?

Correct - I will update the comment to make that clear.

Post by Dan McDonald via illumos-developer
usr/src/cmd/ipf/tools/ipfzone.c
* Seems plausible. I had to look at ip_fil.h though, first.
* After seeing this, the diffs to ipf.c vs. the rest are even more confusing to me as a possible maintainer, never mind as a potential administrator.

Hmm, alright - as per my other e-mail, I'm open to changing this
if it's going to be too onerous.

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/ip_fil_solaris.c
* Lines 1043-1073 & 603-645 look an awful lot alike. Can you possibly factor them out?
* maybe Lines 1126-1154 as well?!

Yes - will do for all 3 parts.

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/ip_log.c
* is it just me, or is this a completely different bug?

No, this is necessary as a result of holding ifs_ipf_global when
entering ipflog_read(), which wasn't done before: see
usr/src/uts/common/inet/ipf/ip_fil_solaris.c, line 1071 for where it's
acquired.

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/netinet/ipf_stack.h
* "pgz"? Does this name have to be that opaque? At least a comment here...

No, probably not - suggestions welcome. :)

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/solaris.c
* Okay, some of my previous comments may be obviated by the introductory text here.

Maybe some pointers to this intro could go elsewhere - I'll take a look
while I'm updating the webrev.

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 18:05:31 UTC

Permalink

Post by Rob Gulewich via illumos-developer
Hi Dan (Part 2),

Interesting. Today, to my knowledge, no distro outside of SmartOS stores anything in /zones/<zonename>/ apart from "dev" and of course "root".

My gut reaction to posing this problem was to have:

/etc/ipf/GZ-controlled/zonename/ipf.conf

or even just "zonename". When, BTW, does this get populated? At zone boot time? During the zone boot? There are details other distros will want to know. Also, any distros with a static-state rpool-like thing would be well-served by being consistent in how they solve this problem (but that's not your problem).

I might block an RTI on this without a plan for addressing this (even if the plan's implementation doesn't go back immediately with these v. useful changes).

BEFORE I start a discussion thread about how non-SmartOS folks would cope, I DO need to know the mechanics of when these GZ-controlled policies get installed at system or zone boot time from your /zones/.../ipf.conf.

Post by Rob Gulewich via illumos-developer

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/netinet/ip_fil.h
* Even though it's u_32_t, ipfz_gz is essentially a boolean, right?

Correct - I will update the comment to make that clear.

Thanks.

Post by Rob Gulewich via illumos-developer

Hmm, alright - as per my other e-mail, I'm open to changing this
if it's going to be too onerous.

I like your overall idea in part 1 (ipf(1M) is the exception because of its pre-existing "-z"). Just make sure it's clear in the code as well as the man pages.

Post by Rob Gulewich via illumos-developer

Yes - will do for all 3 parts.

Thanks.

Post by Rob Gulewich via illumos-developer

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/ip_log.c
* is it just me, or is this a completely different bug?

No, this is necessary as a result of holding ifs_ipf_global when
entering ipflog_read(), which wasn't done before: see
usr/src/uts/common/inet/ipf/ip_fil_solaris.c, line 1071 for where it's
acquired.

Got it! Thank you.

Post by Rob Gulewich via illumos-developer

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/netinet/ipf_stack.h
* "pgz"? Does this name have to be that opaque? At least a comment here...

No, probably not - suggestions welcome. :)

This points to another stack-instance state, but its the GZ-controlled one. How about if_gz_controlled for the boolean, and if_gzcont_ifs for the ipf_stack_t?

Post by Rob Gulewich via illumos-developer

Post by Dan McDonald via illumos-developer
usr/src/uts/common/inet/ipf/solaris.c
* Okay, some of my previous comments may be obviated by the introductory text here.

Maybe some pointers to this intro could go elsewhere - I'll take a look
while I'm updating the webrev.

Excellent.

Thank you!
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:48:53 UTC

Permalink

Post by Dan McDonald via illumos-developer

Post by Rob Gulewich via illumos-developer

Interesting. Today, to my knowledge, no distro outside of SmartOS stores anything in /zones/<zonename>/ apart from "dev" and of course "root".
/etc/ipf/GZ-controlled/zonename/ipf.conf
or even just "zonename". When, BTW, does this get populated? At zone boot time? During the zone boot? There are details other distros will want to know. Also, any distros with a static-state rpool-like thing would be well-served by being consistent in how they solve this problem (but that's not your problem).
I might block an RTI on this without a plan for addressing this (even if the plan's implementation doesn't go back immediately with these v. useful changes).
BEFORE I start a discussion thread about how non-SmartOS folks would cope, I DO need to know the mechanics of when these GZ-controlled policies get installed at system or zone boot time from your /zones/.../ipf.conf.

/zones/.../ipf.conf is populated by the fwadm(1m) tool:

https://github.com/joyent/smartos-live/blob/master/src/fw/man/fwadm.1m.md

For a running zone, fwadm writes ipf.conf, then runs the appropriate set of
ipf commands to enable the GZ-controlled firewall, load the conf file,
and make that ruleset active.

When a zone boots, the brand code enables ipfilter and loads the conf
file if it exists:

https://github.com/joyent/smartos-live/blob/master/overlay/generic/usr/lib/brand/jcommon/statechange#L412

This is very closely coupled to both how SmartOS manages zones and tools
like vmadm(1m) that are only available on SmartOS, so I doubt this
approach would be suitable for other distros. /zones/.../config/ipf.conf
is a good fit for SmartOS, since it allows files in config/ to easily be zfs
sent along with the rest of the zone for backup or migration.

Storing GZ-controlled configs in /etc/ipf/GZ-controlled/zonename/ or
similar makes sense to me for other distros. What I'm less clear on is
what mechanism should be used for loading and reloading those configs, and
how best to integrate that with how other non-SmartOS distros use zones.

As for the rest of your suggestions, there's an updated webrev at:
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.html

And an incremental one with just the new changes:
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5197-1/index.html

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

Robert Mustacchi via illumos-developer

2014-10-08 22:33:17 UTC

Permalink

Post by Rob Gulewich via illumos-developer
Storing GZ-controlled configs in /etc/ipf/GZ-controlled/zonename/ or
similar makes sense to me for other distros. What I'm less clear on is
what mechanism should be used for loading and reloading those configs, and
how best to integrate that with how other non-SmartOS distros use zones.

Hey Dan,

Perhaps it'd be a good follow up RFE to figure out what the best
management strategy is for this and integrating that into the normal
global zone's ipf service. I could see a bunch of ways one might want to
specify this in the service, specifying a directory indexed by zone name
and the like, but its interaction with the zones service and zone boot
gets rather tricky, something would have to watch zone boot to know to
go apply this to zones, at a minimum. It's true that the work isn't as
broadly useful without this piece there, but what I'm not sure is if
it's something worth solving at this moment versus as some follow up
work (I've already argued with myself about a bunch of different messy
architectural ways one could go try and solve this while thinking about
and writing this e-mail).

What do you think?

Robert

-------------------------------------------
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:42:00 UTC

Permalink

Post by Robert Mustacchi via illumos-developer

Writing a followup RFE is a good idea.

I also think the required magic may be in the "brand" code, which for us in in pkg(5) sources, and is not something one can just upstream.

The handling of concurrent setting seems sane, and since you have a clearly defined enforcement order, this may be something to punt into the brand code. I hope someone from OI-land (the other user of pkg) is around to see this. I'd be curious to see what that someone has to say on this subject as well.

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

Dan McDonald via illumos-developer

2014-10-09 15:45:34 UTC

Permalink

SHOOT! I should've read/replied to this one first. Sorry Rob!

On Oct 8, 2014, at 5:48 PM, Rob Gulewich via illumos-developer <***@lists.illumos.org> wrote:

<SNIP!>

Post by Rob Gulewich via illumos-developer
When a zone boots, the brand code enables ipfilter and loads the conf
https://github.com/joyent/smartos-live/blob/master/overlay/generic/usr/lib/brand/jcommon/statechange#L412
This is very closely coupled to both how SmartOS manages zones and tools
like vmadm(1m) that are only available on SmartOS, so I doubt this
approach would be suitable for other distros. /zones/.../config/ipf.conf
is a good fit for SmartOS, since it allows files in config/ to easily be zfs
sent along with the rest of the zone for backup or migration.

I mentioned to Robert M. about brand code. Had I read this first, I'd have been more sure about it. We have state-change code in $PKG_SRC/src/brand/. in pkg(5), and again --> if there are any OI types who work on pkg(5), they should speak up! :)

That may be something for me to work out with other pkg(5) users.

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

I believe I already went over these with Pt. 1's thread, and liked what I saw.

Thanks a ton for this work!
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

Gordon Ross via illumos-developer

2014-10-09 20:41:26 UTC

Permalink

I've previously concluded that the brand code has a lot more knowledge
of how the system works, as compared with knowledge of what packaging
commands it should run (i.e. ipkg brand vs dpkg brand, etc.)
That's why I previously tried to get the brand scripts integrated into ON.
We did that in illumos-nexenta, but some folks here didn't like that idea.
Time to reopen that discussion, maybe?

Gordon

On Thu, Oct 9, 2014 at 11:45 AM, Dan McDonald via illumos-developer

Post by Dan McDonald via illumos-developer
SHOOT! I should've read/replied to this one first. Sorry Rob!
<SNIP!>

That may be something for me to work out with other pkg(5) users.

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

I believe I already went over these with Pt. 1's thread, and liked what I saw.
Thanks a ton for this work!
Dan
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175074-7782178a
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.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