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 75B9C1E515 for ; Fri, 8 Mar 2024 22:13:13 +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=1709935995; cv=none; b=VRJYZO3pxlTNlShh24GBRa39/8Oy40E5XRRg1D0HvNtyoRrJfRCl0QLHkBc4yTgZlktTY9ttFaC9WkAwt3JS6cvM5XRTjmspUZczs7gT9vzc90jiAGU7YuF37UCVk3JP1CkCZtRrNkJ395Hfn9vLLvkcgyFaf5UZTYH0D/RH9S0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709935995; c=relaxed/simple; bh=1T3Uoxz0VyLX2EEw3TM8VR+EPTiAu5rp5eX4nd+gBPk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: In-Reply-To:Content-Type:Content-Disposition; b=ZvbgbR4LrxEHaK2a9Mcc1+Djy3DAEJmcpCmGH9HMaYfmrVoVscV+32Bkls66+4MknIp7X1iHmtYhNAdPQ4DdDVrNhnSE5OWm3N7RDIOF7I8AMEbVZ1BmObfzD0TYnl4/Mlzp44t/LWsWyH3UPBeDNqQfnIeHmGjn9lXkaPr3YPA= 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=Ng+EHxjr; 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="Ng+EHxjr" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709935992; 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=ZZrvJ/mft06+XhOpEd12kvFGToHryAdkQEATHFVYw2I=; b=Ng+EHxjrQ1Ov7yrCibMpkmyHKnF18KbYN3VL/5Hr5dGvx7RuwEuF5FTUO/9Rofs7oJIb0e 0OhvgC8KaL0DRfvyyCUA8E0NSGqw90tuINSfq+EGYUakSVp9u2Z63KHebt7FWuupL6MhEf wiih0qVH/IUt/PCglotL5iUKTZ2uCXQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-6-zuaK_wTsM0Gw4BecUhuovw-1; Fri, 08 Mar 2024 17:13:10 -0500 X-MC-Unique: zuaK_wTsM0Gw4BecUhuovw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (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 1F9B0800264; Fri, 8 Mar 2024 22:13:10 +0000 (UTC) Received: from aion.redhat.com (unknown [10.22.16.116]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0B94617AA6; Fri, 8 Mar 2024 22:13:10 +0000 (UTC) Received: by aion.redhat.com (Postfix, from userid 1000) id A099E12D01A; Fri, 8 Mar 2024 17:13:09 -0500 (EST) Date: Fri, 8 Mar 2024 17:13:09 -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.5 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: > 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 > > 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 >