From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6376D17745 for ; Fri, 8 Mar 2024 21:09:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709932143; cv=none; b=IzfBaDlT8CoqMosAYIJng0auxC1KIouWJvp+h2pO0Sunv/eXzVBZQW6aCgCESAQZxcyNXO6z9pPTSX4/Hrk9sZfMe4jM4aCg/vv0JACWbsRTln3Dvt2F97ZZc2LtrzuzQq5DopFgFTsj0IOfPtZzarH8yRN7eRXhn/p6Gl3EIKg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709932143; c=relaxed/simple; bh=mb2BFR4eMuJmv3npYsaI1ytpQ8ubTesosum51ltfyY0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: In-Reply-To:Content-Type:Content-Disposition; b=tZhG8dT/84OO/OJ7sHh52Rybcdrr92rk+fjslIQ5oF58/RZZx9XHE16raECdatj2cUqlhs6tW6Z2abwbp96tpl9BcFACMQkYQvRYTsDVRBfLCOe7OS/vH09bfjcb2TnphGXjZ5oQUK32+cIrJe8O7aP9M0DuEU+m+uIVON917Gc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=DeJ3QXUw; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="DeJ3QXUw" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709932140; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=S28FuvOpAqiycgOgR91NRZshd/E6L9AXTyDicry/4Uw=; b=DeJ3QXUwQ36yYQ1yNMh14H135TiqQjSNhHkJX+bO7QypZvBY928r/p/SjdqKD8IrEOliFj 1ZdR1fah37NO8+zgPIkODX6auqmQUDkxirGsgofLLWV5QlxCIouKZ9mhjJYE3h2GJUJYs4 w9N8ri0eM/9ocVEm3BeTko/hEfWKH4k= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-515-qN5XhI-5Oi2W8iMPerKaww-1; Fri, 08 Mar 2024 16:08:58 -0500 X-MC-Unique: qN5XhI-5Oi2W8iMPerKaww-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 91F6C29AC005; Fri, 8 Mar 2024 21:08:57 +0000 (UTC) Received: from aion.redhat.com (unknown [10.22.16.116]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7BA071C060D6; Fri, 8 Mar 2024 21:08:57 +0000 (UTC) Received: by aion.redhat.com (Postfix, from userid 1000) id 0F7E712D015; Fri, 8 Mar 2024 16:08:57 -0500 (EST) Date: Fri, 8 Mar 2024 16:08:57 -0500 From: Scott Mayhew To: Luis Chamberlain Cc: kdevops@lists.linux.dev Subject: Re: [PATCH 5/5] fstests/nfs: add krb5 support Message-ID: References: <20240307131414.1244984-1-smayhew@redhat.com> <20240307131414.1244984-6-smayhew@redhat.com> Precedence: bulk X-Mailing-List: kdevops@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > >