Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[andrew bezella]

thanks for following up on this!

i think the stripping of the file extension (line 47 in the old code)
needs to be added back in, otherwise the defined classes would have to
match the basefile+extension:
https://github.com/faiproject/fai/pull/61

i also noticed that one problem with this approach is that it does
limit the basefiles to short hostnames.  but that's not new and seems
like an edge case.

On Thu, 2017-07-27 at 11:45 +0200, Thomas Lange wrote:
> I have patched fetch-basefile so this should now work as
> expected. Instead of an eval and indirect shell variables, I just use
> an array and loop over this. No performce penalty unless you have
> some
> thousands of classes defined for a client and a million of base files
> ;-)
> 
> Here are the diffs:
> https://github.com/faiproject/fai/commit/eedc1c27229aa77f81f8d718214a
> 5b4f4dc2c908
> https://github.com/faiproject/fai/commit/bc88a9e5d2ca21d7c19cb7b39c9e
> a64cf905c2fc
> 
> So, I think currently there's no need to substitute hyphen and dot
> with underscore for the FAI class names and we can leave this
> unchanged. But I will improve the documentation about class names.
-- 
andrew bezella 
internet archive

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Thomas Lange]

I have patched fetch-basefile so this should now work as
expected. Instead of an eval and indirect shell variables, I just use
an array and loop over this. No performce penalty unless you have some
thousands of classes defined for a client and a million of base files ;-)

Here are the diffs:
https://github.com/faiproject/fai/commit/eedc1c27229aa77f81f8d718214a5b4f4dc2c908
https://github.com/faiproject/fai/commit/bc88a9e5d2ca21d7c19cb7b39c9ea64cf905c2fc

So, I think currently there's no need to substitute hyphen and dot
with underscore for the FAI class names and we can leave this
unchanged. But I will improve the documentation about class names.
-- 
regards Thomas

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[andrew bezella]

On Wed, 2017-07-26 at 17:34 -0400, Arcady Genkin wrote:
[...]
> # chroot /data/fai-nfsroots/xenial-x64 /bin/bash --version |head -1
> GNU bash, version 4.4.12(1)-release (x86_64-pc-linux-gnu)

ah! ok, apparently a change in bash's behavior between 4.3 and 4.4:

% bash --version | head -n1
GNU bash, version 4.4.7(1)-release (x86_64-pc-linux-gnu)
% export classes="vmops0.us.archive.org"
% bash /usr/lib/fai/fetch-basefile
/usr/lib/fai/fetch-basefile: line 59: found_vmops0.us.archive.org: bad 
substitution
No basefile matching a class name was found at [FAI_BASEFILEURL]

vs

% bash --version | head -n1
GNU bash, version 4.3.48(1)-release (x86_64-pc-linux-gnu)
% export classes="vmops0.us.archive.org"
% bash /usr/lib/fai/fetch-basefile
No basefile matching a class name was found at [FAI_BASEFILEURL]


in bash 4.4 it appears that either "." or "-" breaks the indirect
variable usage:

% bash --version | head -n1
GNU bash, version 4.4.7(1)-release (x86_64-pc-linux-gnu)
% export classes="vmops0.us.archive.org"
% bash /usr/lib/fai/fetch-basefile
/usr/lib/fai/fetch-basefile: line 59: found_vmops0.us.archive.org: bad 
substitution
No basefile matching a class name was found at [FAI_BASEFILEURL]
% export classes="vm-ops0"
% bash /usr/lib/fai/fetch-basefile
/usr/lib/fai/fetch-basefile: line 59: found_vm-ops0: bad substitution
No basefile matching a class name was found at [FAI_BASEFILEURL]
% export classes="vmops0"
No basefile matching a class name was found at [FAI_BASEFILEURL]

possibly related to the announcement at https://lists.gnu.org/archive/h
tml/bug-bash/2015-07/msg00027.html which mentions a couple changes to
indirect variable expansion.  unsure if the above is intentional or
collateral damage.  "_" does appear to be ok.


-- 
andrew bezella 
internet archive

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Arcady Genkin]

> @Arcady - could you check the version of bash that you're using and
> confirm that `/bin/bash` on your system isn't a symlink to `dash` or
> something?  and/or try setting the $classes variable manually to
> exclude the hostname's hyphen and test again?

Hi, Andy,

By "your system" I am taking the NFS root in which the FAI scripts run. The
NFS root is freshly generated from Debian Stretch. There /bin/bash and
/bin/dash are two distinct binaries.  I won't get to trying setting
$classes manually for a few days; I'll report when I get a chance to do
that.

# chroot /data/fai-nfsroots/xenial-x64 /bin/bash --version |head -1
GNU bash, version 4.4.12(1)-release (x86_64-pc-linux-gnu)

root@ps2:/data/fai-nfsroots/xenial-x64/bin# stat dash bash
  File: `dash'
  Size: 117208  Blocks: 232IO Block: 4096   regular file
Device: 10307h/66311d   Inode: 284706  Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (0/root)   Gid: (0/root)
Access: 2017-07-19 11:39:31.414194403 -0400
Modify: 2017-01-24 00:16:56.0 -0500
Change: 2017-07-12 16:52:56.095031937 -0400
 Birth: -
  File: `bash'
  Size: 1099016 Blocks: 2152   IO Block: 4096   regular file
Device: 10307h/66311d   Inode: 284767  Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (0/root)   Gid: (0/root)
Access: 2017-07-19 11:39:27.522457272 -0400
Modify: 2017-05-15 15:45:32.0 -0400
Change: 2017-07-12 16:52:56.235022488 -0400
 Birth: -

-- 
Arcady Genkin

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[andrew bezella]

On Wed, 2017-07-26 at 21:17 +0200, Thomas Lange wrote:
> And we have dots in FQDN, which are currently also not allowed in
> class names. Should we truncate the FQDN before defining the hostname
> as a class or replace  the dots with the underscore?

if this is implemented, i'd say replace the dots w/underscores, similar
to the nis/yp example in the fai-class man page.  subdomains can have
meaning and short hostnames should not be presumed to be unique in an
organization.

however, please bear in mind that making this change is likely to prove
disruptive for anyone expecting the current behavior.  and the current
class-naming rules are documented, even to the hostname-class
exception.  also (see below) i'm not sure hyphens are the problem here.

@Thomas - are you able to reproduce this failure mode?  as i mentioned,
we install a lot of hosts w/hyphens in their name w/o issue.

@Arcady - could you check the version of bash that you're using and
confirm that `/bin/bash` on your system isn't a symlink to `dash` or
something?  and/or try setting the $classes variable manually to
exclude the hostname's hyphen and test again?

i can generate this error by trying to run `dash fetch-basefile`
regardless of whether $classes includes a hyphen or not.  might be a
bash-ism and somehow a non-bash shell is trying to run it?

% export classes="vmops0.us.archive.org"
% dash ./fetch-basefile
./fetch-basefile: 59: ./fetch-basefile: Bad substitution
% export classes="vm-ops0.us.archive.org"
% dash ./fetch-basefile
./fetch-basefile: 59: ./fetch-basefile: Bad substitution
% bash ./fetch-basefile
No basefile matching a class name was found at [FAI_BASEFILEURL]

andy

-- 
andrew bezella 
internet archive

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Thomas Lange]

> On Wed, 26 Jul 2017 11:45:20 -0400, Arcady Genkin  
> said:

> I think that the
> approach to replace hyphens and any other illegal characters with 
underscores
> is a very good one.

And we have dots in FQDN, which are currently also not allowed in
class names. Should we truncate the FQDN before defining the hostname
as a class or replace  the dots with the underscore?

-- 
regards Thomas

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[andrew bezella]

On Wed, 2017-07-26 at 08:35 -0700, andrew bezella wrote:
> it could perhaps be re-worded along the lines of "All class names are
> restricted to uppercase letters and underscores [A-Z_] (except the
> FAI-
> defined class matching the hostname)" and maybe placed more
> prominently
> in the man page and other documentation (and i just noticed there's a
> typo in there: "execpt" -> "except")

oh, i forgot that digits are also allowed.  so if a documentation
update comes out of this, maybe:
"All class names are restricted to uppercase alphanumeric and
underscores [[:upper:][:digit:]_] (except the FAI-defined class
matching the hostname)"

-- 
andrew bezella 
internet archive

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Arcady Genkin]

Hi, Thomas,

Since hyphens are legal in hostnames, but illegal in FAI classes, I think
that then the bug is not with the fetch-basfile script, but with whatever
defines that class. I thin that the hostname class is automatically defined
by FAI (i.e. not by our FAI configuration), but I may be wrong.  I think
that the approach to replace hyphens and any other illegal characters with
underscores is a very good one.

On Wed, Jul 26, 2017 at 9:51 AM, Thomas Lange  wrote:

> > On Thu, 13 Jul 2017 17:41:56 -0400, Arcady Genkin 
> said:
>
> > guessing that the hyphen in the host name is causing the problem
> (the hostname
> > is "eddie-vm.teach.cs.toronto.edu" which defines a FAI class of the
> same name).
> Hi Arcady,
>
> the problem is, that FAI class names should not contain a hyphen. IIRC
> this was done because cfengine classes also do not allow hyphens, but
> underscore.
>
> One solution would be to substitute the hyphen in the hostname into a
> underscore, which is allowed in FAI classes.
>
> I fear we will break more things if we allow hyphens in class names.
>
> Any comments on this from the mailing list (CC)?
> --
> regards Thomas
>



-- 
Arcady Genkin

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[andrew bezella]

in general, the hyphen not being allowed in a class name seems a
documented restriction and not a bug.  the fai-class man page states:
> All class names should be written in uppercase letters (execpt the class 
> of the hostname). Do not use a dash, use an underscore.

it could perhaps be re-worded along the lines of "All class names are
restricted to uppercase letters and underscores [A-Z_] (except the FAI-
defined class matching the hostname)" and maybe placed more prominently
in the man page and other documentation (and i just noticed there's a
typo in there: "execpt" -> "except")

i'd tend to agree that formally allowing hyphens in the class name
would take significant effort and likely result in some breakage.

for this bug in particular, there might be something else going on? 
we're trailing by a few versions, but the majority of our dirinstalls
are for hosts with hyphens in their name and we have (although not
recently) also had bare-metal hosts with hyphens.  unless i'm missing
something, i can't reproduce on a host w/a hyphen using the script from
5.3.4:
+ revclasses='LAST vm-ops0.us.archive.org XENIAL64 XENIAL UBUNTU DEBIAN FAIBASE 
DHCPC CHROOT LINUX DEFAULT '
+ for c in '$revclasses'
+ id=found_LAST
+ '[' X '!=' X ']'
+ for c in '$revclasses'
+ id=found_vm-ops0.us.archive.org
+ '[' X '!=' X ']'
+ for c in '$revclasses'
+ id=found_XENIAL64
+ '[' XXENIAL64.tar.xz '!=' X ']'
+ found=1

hth...

andy

-- 
andrew bezella 
internet archive

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Thomas Lange]

> On Thu, 13 Jul 2017 17:41:56 -0400, Arcady Genkin  
> said:

> guessing that the hyphen in the host name is causing the problem (the 
hostname
> is "eddie-vm.teach.cs.toronto.edu" which defines a FAI class of the same 
name).
Hi Arcady,

the problem is, that FAI class names should not contain a hyphen. IIRC
this was done because cfengine classes also do not allow hyphens, but
underscore.

One solution would be to substitute the hyphen in the hostname into a
underscore, which is allowed in FAI classes.

I fear we will break more things if we allow hyphens in class names.

Any comments on this from the mailing list (CC)?
-- 
regards Thomas

Bug#868267: fai-client: fetch-basefile breaks for hostnames with hyphens

[Arcady Genkin]

Package: fai-client
Version: 5.3.6
Severity: normal

Dear Maintainer,

I cannot get fetch-basefile to work. Investigating, I tried running
/usr/lib/fai/fetch-basefile by hand, and it breaks in line 59

root@eddie-vm:/usr/lib/fai# ./fetch-basefile 
Fetching basefile from http://pkgmirror0.teach.cs.toronto.edu/basefiles/
./fetch-basefile: line 59: found_eddie-vm.teach.cs.toronto.edu: bad substitution
No basefile matching a class name was found at 
http://pkgmirror0.teach.cs.toronto.edu/basefiles/

Line 59 contains a Bash variable substitution "if [ X${!id} != X ]; then", I am
guessing that the hyphen in the host name is causing the problem (the hostname
is "eddie-vm.teach.cs.toronto.edu" which defines a FAI class of the same name).

Thanks!

-- System Information:
Debian Release: 9.0
  APT prefers stable
  APT policy: (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 4.9.0-3-amd64 (SMP w/1 CPU core)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), 
LANGUAGE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages fai-client depends on:
ii  debconf-utils1.5.61
ii  file 1:5.30-1
ii  iproute2 4.9.0-1
ii  libapt-pkg-perl  0.1.32
ii  libfile-lchown-perl  0.02-2+b2
ii  perl 5.24.1-3

Versions of packages fai-client recommends:
ii  libgraph-perl  1:0.96-2

Versions of packages fai-client suggests:
pn  logtail  

-- Configuration Files:
/etc/fai/fai.conf changed [not included]

-- no debconf information