Re: [PATCH 5/5] fstests/nfs: add krb5 support

[Scott Mayhew <smayhew@redhat.com>]

On Fri, 08 Mar 2024, Luis Chamberlain wrote:

> On Fri, Mar 08, 2024 at 02:33:24PM -0500, Scott Mayhew wrote:
> > On Fri, 08 Mar 2024, Luis Chamberlain wrote:
> > 
> > > My review comments are not requirements, they are how to enhance this
> > > so we can scale better and long term goals to keep in mind. Whether or
> > > not you do the work is up to you.
> > > 
> > > On Thu, Mar 07, 2024 at 08:14:14AM -0500, Scott Mayhew wrote:
> > > > diff --git a/Makefile b/Makefile
> > > > index 9ca3a5f3..df4aad7b 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -115,6 +115,11 @@ ifeq (y,$(CONFIG_KDEVOPS_SETUP_NFSD))
> > > >  include scripts/nfsd.Makefile
> > > >  endif # CONFIG_KDEVOPS_SETUP_NFSD
> > > >  
> > > > +ifeq (y,$(CONFIG_KDEVOPS_SETUP_KRB5))
> > > > +include scripts/kdc.Makefile
> > > > +include scripts/krb5.Makefile
> > > > +endif # CONFIG_KDEVOPS_SETUP_KRB5
> > > 
> > > This sort of clutter can be compartamentalized now, see right above:
> > > 
> > > include scripts/provision.Makefile                                               
> > > include scripts/systemd-timesync.Makefile                                        
> > > include scripts/journal-server.Makefile                                          
> > >                                                                                  
> > > KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)                          
> > > KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_PROVISIONED_DEVCONFIG) 
> > > 
> > > This let's us now split work which needs to be set up early
> > > and this can vary depending on if the dep is a localhost (hypervisor or
> > > command and control) setting or a target node (guest or taret node on
> > > cloud) setting.
> > > 
> > > So for example systemd-timesync has both parts:
> > > 
> > > LOCALHOST_SETUP_WORK += timesyncd-server
> > > KDEVOPS_BRING_UP_DEPS_EARLY += timesyncd-client
> > > 
> > > Then the clutter is kept on the target makefile. This let's us also keep
> > > ordering by the Makfile include order. So we should be able to move
> > > siw ktls nfs setup to this methodology too. That will let us scale this
> > > and keep our top level Makefile neat and makes orer explicit and clear.
> > > 
> > > It seems in this case it's all being set up on the target node so only
> > > KDEVOPS_BRING_UP_DEPS_EARLY is needed.
> > 
> > Just so I'm clear on what you're suggesting...
> > 
> > 1. move the ifeq...endif directives inside the target makefiles
> 
> Yeap
> 
> > 2. move the KDEVOPS_BRING_UP_DEPS stuff out of bringup.Makefile and into the
> >    target makefiles
> 
> Yes in that the order of the Makefile should suffice, then its a matter
> of just ordering the includes. Those other KDEVOPS_BRING_UP_DEPS += for
> nfsd, ktls and siw could also move out from scripts/bringup.Makefile
> to their own Makefile too.
> 
> > (and use KDEVOPS_BRING_UP_DEPS_EARLY instead)
> 
> Sorry about the confusion KDEVOPS_BRING_UP_DEPS_EARLY is for deps
> which neet to be run before the general devconfig playbook and
> in retrospect I don't think this is needed for the things you are
> adding.
> 
> > 3. move the includes up above this line:
> > KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
> 
> Yes but I think we need to make a change to make it work properly,
> so I can try do that later. But for now I think what we need is
> to end up with something like this:
> 
> include scripts/provision.Makefile
> include scripts/systemd-timesync.Makefile
> include scripts/journal-server.Makefile
> 
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_PROVISIONED_DEVCONFIG)
> 
> include scripts/siw.Makefile
> include scripts/ktls.Makefile
> <I guess your new stuff here?>
> include scripts/nfsd.Makefile
> 
> include workflows/Makefile                                                       
> 
> You would know best if the stuff you are adding goes before / after
> siw, or ktls, nfsd, etc.
> 
> > Also, did you see my reply to Chuck about doing the krb5 client setup
> > automatically?  In order to do that I need to have a "post" bringup
> > step, so that bringup target would look like this:
> > 
> > bringup: $(KDEVOPS_BRING_UP_DEPS) update_etc_hosts $(KDEVOPS_BRING_UP_POST)
> > 
> > Is that okay?  Note that the krb5 client setup has to run after update_etc_hosts,
> > so KDEVOPS_BRING_UP_LATE_DEPS wouldn't be appropriate for this.
> 
> Ah, no, just use KDEVOPS_BRING_UP_LATE_DEPS on your Makefile we already
> do this:
> 
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_LATE_DEPS)                           
>                                                                                  
> ifneq (,$(KDEVOPS_BRING_UP_DEPS))
> include
> scripts/bringup.Makefile
> endif 

Again, using KDEVOPS_BRING_UP_LATE_DEPS won't work for the krb5 setup
because everything in KDEVOPS_BRING_UP_LATE_DEPS still happens before
update_etc_hosts... so the clients & nfsd will all fail to contact the
KDC because they won't have the KDC's address yet.

Maybe update_etc_hosts needs to also be in its own makefile, and add it
to KDEVOPS_BRING_UP_LATE_DEPS.  Then I could add krb5 to
KDEVOPS_BRING_UP_LATE_DEPS... as long as it's *after* update_etc_hosts
it should work.

> 
> So moving the nfs/etc out and keeping the Makefiles in order will ensure
> that is setup correctly, then in terms of having each Makfile have a few
> things which need to go early or not, that's where these different
> targets come into play.
> 
> Technically we could move the eyesor eof having the top level Makefile
> do:
> 
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_PROVISIONED_DEVCONFIG)
> 
> And instead doing something like this later:
> 
> # Redefine KDEVOPS_BRING_UP_DEPS now with proper ordering in mind
> KDEVOPS_BRING_UP_DEPS := \
> 	$(KDEVOPS_BRING_UP_DEPS_EARLY) \
> 	$(KDEVOPS_PROVISIONED_DEVCONFIG) \
> 	$(KDEVOPS_BRING_UP_DEPS) \
> 	$(KDEVOPS_BRING_UP_LATE_DEPS)
> 
> ifneq (,$(KDEVOPS_BRING_UP_DEPS))
> include
> scripts/bringup.Makefile
> endif 
> 
> But I wasn't sure if this Make-foo works.
> 
> > > We should add an kdc_enable which defaults to False and if true then we
> > > include the clutter below.
> > > 
> > > In retrospect the same should be done for nfsd.
> > > 
> > > Ie, if no one enabled nfsd or kdc we should hide targets for these
> > > options too and so the user has no make targets to use them and so no
> > > reason to clutter exisitng hosts file for user who don't enable these
> > > things.
> > 
> > I did notice that those stanzas were present even if those options weren't
> > enabled.
> > 
> > Do I really need a separate kdc_enable or should I just use the
> > krb5_enable variable that you suggested below?  
> 
> That's up to you, you know your requirements better.
> 
> > > > diff --git a/playbooks/roles/kdc/vars/default.yml b/playbooks/roles/kdc/vars/default.yml
> > > > new file mode 100644
> > > > index 00000000..ed97d539
> > > > --- /dev/null
> > > > +++ b/playbooks/roles/kdc/vars/default.yml
> > > > @@ -0,0 +1 @@
> > > > +---
> > > 
> > > This is empty, it should have all sensible defaults and .. it's a good
> > > time to evaluate whether or not having things configurable is better,
> > > but I undersand that can be a second step. The other reason to have
> > > things configurable is it lets you document things. But that's totally
> > > optional.
> > 
> > I can add defaults, but they'll be the Red Hat defaults and might not
> > work with other distros.  Originally I didn't have those configurable at
> > all, and when I went and tested Debian and Suse I found that stuff
> > didn't work.
> 
> Sorry for the trouble and thanks for doing this!
> 
> Using default for the first distro that added support makes sense.
> Best effort for the others makes sense to me.
> 
> It's what we did with guestfs, we now are extending it slowly with
> debian stuff.
> 
> > Unfortunately the names of the systemd services and where
> > they look for configuration files and data differs from distro to
> > distro, so I had to have a least some of the stuff configurable... but I
> > tried to keep the number of variables to a minimum.
> 
> Sure, to ramp up it makse sense to minimize tunables, I figured I'd just
> point out the possible value in that Kconfig also serves as a way to let
> us document often obscure things, it serves as documentation for us
> too. But sure, makes sense to avoid adding knobs if you don't need
> them yet.
> 
>   Luis
>