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

[Scott Mayhew <smayhew@redhat.com>]

On Fri, 08 Mar 2024, 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
> 2. move the KDEVOPS_BRING_UP_DEPS stuff out of bringup.Makefile and into the
>    target makefiles (and use KDEVOPS_BRING_UP_DEPS_EARLY instead)
> 3. move the includes up above this line:
> KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
> 
> Does that sound right?

I think I'm missing something, because doing the above puts those steps
before the ssh configuration, and they fail.

-Scott

> 
> 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.
> 
> > 
> > BTW you may benefit from CONFIG_DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD as it
> > sets up NTP on the host/nodes. But if you're going to enable that
> > you could just enable systemd-remote-journal too, which we now have
> > support for in guestfs.
> > 
> > > diff --git a/kconfigs/Kconfig.bringup.goals b/kconfigs/Kconfig.bringup.goals
> > > index 71948e9b..26ffac98 100644
> > > --- a/kconfigs/Kconfig.bringup.goals
> > > +++ b/kconfigs/Kconfig.bringup.goals
> > > @@ -109,3 +109,15 @@ menu "Configure the kernel NFS server"
> > >  source "kconfigs/Kconfig.nfsd"
> > >  endmenu
> > >  endif
> > > +
> > > +config KDEVOPS_SETUP_KRB5
> > > +	bool "Set up KRB5"
> > > +	default n
> > > +	help
> > > +	  Configure and bring up a MIT Kerberos V5 KDC.
> > > +
> > > +if KDEVOPS_SETUP_KRB5
> > > +menu "Configure the KRB5 KDC"
> > > +source "kconfigs/Kconfig.kdc"
> > > +endmenu
> > > +endif
> > 
> > I think its cleaner if we move the config and the if to the
> > kconfigs/Kconfig.kdc, the similar change could be done with
> > KDEVOPS_SETUP_NFSD so its easier to add things the the top level
> > kconfigs/Kconfig.nfsd is kept clean.
> 
> Will do.
> 
> > 
> > > diff --git a/playbooks/roles/fstests/templates/nfs/nfsmount.conf b/playbooks/roles/fstests/templates/nfs/nfsmount.conf
> > > new file mode 100644
> > > index 00000000..73b6a8e4
> > > --- /dev/null
> > > +++ b/playbooks/roles/fstests/templates/nfs/nfsmount.conf
> > > @@ -0,0 +1,2 @@
> > > +[ NFSMount_Global_Options ]
> > > +# Sec=sys
> > > diff --git a/playbooks/roles/gen_hosts/templates/fstests.j2 b/playbooks/roles/gen_hosts/templates/fstests.j2
> > > index 74057952..b94e89da 100644
> > > --- a/playbooks/roles/gen_hosts/templates/fstests.j2
> > > +++ b/playbooks/roles/gen_hosts/templates/fstests.j2
> > > @@ -27,3 +27,20 @@ ansible_python_interpreter =  "{{ kdevops_python_interpreter }}"
> > >  {% endif %}
> > >  [nfsd:vars]
> > >  ansible_python_interpreter =  "{{ kdevops_python_interpreter }}"
> > > +[kdc]
> > > +{% if krb5_realm is defined %}
> > > +{{ kdevops_hosts_prefix }}-kdc
> > > +{% endif %}
> > > +[kdc:vars]
> > > +ansible_python_interpreter =  "{{ kdevops_python_interpreter }}"
> > > +[krb5]
> > > +{% if krb5_realm is defined %}
> > > +{% for s in fstests_enabled_test_types %}
> > > +{{ kdevops_host_prefix }}-{{ s }}
> > > +{% endfor %}
> > > +{% if nfsd_threads is defined %}
> > > +{{ kdevops_hosts_prefix }}-nfsd
> > > +{% endif %}
> > > +{% endif %}
> > > +[krb5:vars]
> > > +ansible_python_interpreter =  "{{ kdevops_python_interpreter }}"
> > 
> > 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?  
> 
> > 
> > > diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml
> > > index 2f5c48b6..1181ef10 100644
> > > --- a/playbooks/roles/gen_nodes/tasks/main.yml
> > > +++ b/playbooks/roles/gen_nodes/tasks/main.yml
> > > @@ -55,6 +55,18 @@
> > >    when:
> > >      - nfsd_threads is defined
> > >  
> > > +- name: Set kdc_nodes list
> > > +  set_fact:
> > > +    kdc_nodes: "{{ [ kdevops_host_prefix + '-kdc' ] }}"
> > > +  when:
> > > +    - krb5_realm is defined
> > 
> > We shoudl have a respective krb5_enable or something like that
> > which defaults to False and here we shiould use krb5_realm_enable|bool
> > instead.
> > 
> > The respective kconfig option would be
> > 
> > CONFIG_KRB5_REALM_ENABLE
> > 
> > The rationale would be that we later extend kconfig support so
> > each kconfig option can have below say an extra tag to indicate
> > "generate yaml", so our extra_vars.yaml file is automatically generated
> > for us by kconfig itself. That is, we'd tell kconfig which kconfig
> > symbols we want it to generate respective yaml entries for. So it'd
> > just lowercase the symbol name and remove config prefix. Then later
> > we can remove tons of Makefile changes which modify something to True.
> 
> Will do.
> 
> > 
> > > +- name: Add a KRB5 KDC if one was selected
> > > +  set_fact:
> > > +    generic_nodes: "{{ generic_nodes + kdc_nodes }}"
> > > +  when:
> > > +    - krb5_realm is defined
> > 
> > Same.
> > 
> > > +
> > >  - name: Set fstests config file variable for {{ fstests_fstyp }}
> > >    set_fact:
> > >      is_fstests: True
> > > @@ -217,6 +229,13 @@
> > >      - is_fstests|bool
> > >      - nfsd_threads is defined
> > >  
> > > +- name: Add the KRB5 KDC if one was selected
> > > +  set_fact:
> > > +    fstests_enabled_nodes: "{{ fstests_enabled_nodes + kdc_nodes }}"
> > > +  when:
> > > +    - is_fstests|bool
> > > +    - krb5_realm is defined
> > 
> > Same.
> > 
> > > diff --git a/playbooks/roles/kdc/tasks/main.yml b/playbooks/roles/kdc/tasks/main.yml
> > > new file mode 100644
> > > index 00000000..b67f38d0
> > > --- /dev/null
> > > +++ b/playbooks/roles/kdc/tasks/main.yml
> > > @@ -0,0 +1,119 @@
> > > +---
> > > +- name: Get OS-specific variables
> > > +  ansible.builtin.include_vars: "{{ lookup('ansible.builtin.first_found', params) }}"
> > > +  vars:
> > > +    params:
> > > +      files:
> > > +        - '{{ansible_distribution}}.yml'
> > > +        - '{{ansible_os_family}}.yml'
> > > +        - default.yml
> > > +      paths:
> > > +        - 'vars'
> > 
> > Ah.. this is a good alternative to Kconfig defaults but ...
> > 
> > > 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.  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.
> 
> > 
> > > index 5a477847..5c6a59c3 100644
> > > --- a/scripts/bringup.Makefile
> > > +++ b/scripts/bringup.Makefile
> > > @@ -33,6 +33,10 @@ ifeq (y,$(CONFIG_KDEVOPS_SETUP_SIW))
> > >  KDEVOPS_BRING_UP_DEPS += siw
> > >  endif # KDEVOPS_SETUP_SIW
> > >  
> > > +ifeq (y,$(CONFIG_KDEVOPS_SETUP_KRB5))
> > > +KDEVOPS_BRING_UP_DEPS += kdc
> > > +endif # KDEVOPS_SETUP_KRB5
> > 
> > See same ordering thing here. Granted, I am not sure if this is
> > a dep which needs to be set up early, so you should decide, but as
> > our deps grow I thought it would be good to split them by regular
> > services Vs optional later things.
> > 
> >   Luis
> >