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

[Scott Mayhew <smayhew@redhat.com>]

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?

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
>