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.133.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 9C7BB3C32 for ; Fri, 8 Mar 2024 19:33:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709926409; cv=none; b=N06vogPHKlJB3iSLAcb47kGud0CDre4z4CTAEtNWUErhGo3PSb86iohGPEhJ/Ah6oQzNQidi0SWhbVLtG2ty5vSvt+CsEkeSnBdQddCnWt2WJk7bBqDScnB2ExZ8NdTtAcWhJXIJabPhfepOJXSC8TkUuZLOXS0uC95lJwYrhBU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709926409; c=relaxed/simple; bh=VQ6YKXTLZMPZXLqOf+iT3eqxww2YzSIEfdA/r2oaxvI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: In-Reply-To:Content-Type:Content-Disposition; b=k1fT8egxf6uhf1CHrw9ZGx3hR36nGAwSE1zLbaIUnizXj1hEEjEC/Ehn9XfouVUU7SiDlhjg6Qr0McRHxqGPdveSrR9q6/Jfq5A2hVtsQHTboh6ulKGwgEpeOMQQ2QPTNZwqFggdlA4Ol2F2m3yP1S82riAIueYoO95yXsOEMVI= 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=dsJse6sY; arc=none smtp.client-ip=170.10.133.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="dsJse6sY" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709926406; 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=qGOZYFaxCr5VmXep8fzb9qj8l/vq92jVguA1pb3rSvw=; b=dsJse6sYaGoMgFOKHx+w+KldWuwUoEloXGHDpbyJYlvslj/KyU+mpGgyvpb9QxL3R6ADmw qKdCyruhMn/RQql4vyO1d/3fW9Ke7B/v6PeX76mh5lvNAl4gcDDi3tCjMzg4pu1L2yffbX c5XCD3Glm4vaAA1JtjOLHXbTPiXjYBE= 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-394-GTrm1DCQNReYoeuMIFbqYA-1; Fri, 08 Mar 2024 14:33:25 -0500 X-MC-Unique: GTrm1DCQNReYoeuMIFbqYA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 E420C1C05122; Fri, 8 Mar 2024 19:33:24 +0000 (UTC) Received: from aion.redhat.com (unknown [10.22.16.116]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D00242022EDB; Fri, 8 Mar 2024 19:33:24 +0000 (UTC) Received: by aion.redhat.com (Postfix, from userid 1000) id 7052412D00D; Fri, 8 Mar 2024 14:33:24 -0500 (EST) Date: Fri, 8 Mar 2024 14:33:24 -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.4 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, 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 >