2
0
mirror of https://github.com/xcat2/xcat-core.git synced 2026-05-17 19:57:18 +00:00

Merge pull request #7558 from VersatusHPC/fix/mkdef-partial-object-on-validation-error

fix: prevent mkdef partial writes on validation errors
This commit is contained in:
Markus Hilger
2026-05-07 11:50:41 +02:00
committed by GitHub
4 changed files with 467 additions and 49 deletions
+150 -25
View File
@@ -959,6 +959,24 @@ sub setobjdefs
%DBgroupsattr=xCAT::DBobjUtils->getobjdefs(\%tmpghash);
}
my @only_if_failures = xCAT::DBobjUtils->validate_only_if_attrs(
$objname,
$type,
$objhash{$objname},
$DBattrvals{$objname} || {},
\%DBgroupsattr,
);
if (@only_if_failures) {
foreach my $failure (@only_if_failures) {
my $rsp;
$rsp->{data}->[0] = $failure->{message};
xCAT::MsgUtils->message("E", $rsp, $::callback);
}
$objhash{$objname}{error} = 1;
$ret = 1;
next;
}
# check FINALATTRS to see if all the attrs are valid
foreach my $attr (keys %{ $objhash{$objname} }) {
@@ -1032,7 +1050,16 @@ sub setobjdefs
# as well as the attrs for this object that may be
# already set in DB
if (!($objhash{$objname}{$check_attr}) && !($DBattrvals{$objname}{$check_attr})) {
# Also check group inheritance for the condition
my $grp_has_check_attr = 0;
foreach my $tmpgrp (@tmplgrplist) {
if ($DBgroupsattr{$tmpgrp}{$check_attr}) {
$grp_has_check_attr = 1;
last;
}
}
if (!($objhash{$objname}{$check_attr}) && !($DBattrvals{$objname}{$check_attr}) && !$grp_has_check_attr) {
# if I didn't already check for this attr
my $rsp;
@@ -1063,7 +1090,17 @@ sub setobjdefs
next;
}
if (!($objhash{$objname}{$check_attr} =~ /\b$check_value\b/) && !($DBattrvals{$objname}{$check_attr} =~ /\b$check_value\b/)) {
my $grp_matches_check_value = 0;
foreach my $tmpgrp (@tmplgrplist) {
if ($DBgroupsattr{$tmpgrp}{$check_attr} && $DBgroupsattr{$tmpgrp}{$check_attr} =~ /\b$check_value\b/) {
$grp_matches_check_value = 1;
last;
}
}
my $obj_val = $objhash{$objname}{$check_attr} || '';
my $db_val = $DBattrvals{$objname}{$check_attr} || '';
if (!($obj_val =~ /\b$check_value\b/) && !($db_val =~ /\b$check_value\b/) && !$grp_matches_check_value) {
if ($invalidattr->{$attr_name}->{valid} != 1) {
$invalidattr->{$attr_name}->{valid} = 0;
$invalidattr->{$attr_name}->{condition}=$check_attr;
@@ -1211,6 +1248,7 @@ sub setobjdefs
}
my $rsp;
my $object_failed_validation = 0;
foreach my $att (keys %$invalidattr) {
my $pickvalidattr=0;
if ($invalidattr->{$att}->{valid} != 1) {
@@ -1230,33 +1268,17 @@ sub setobjdefs
$conditionlist->{$conditionkey}=~s/,/ or /g;
push @{ $rsp->{data} }, "Cannot set the attr=\'$att\' attribute unless $invalidattr->{$att}->{condition} value is $conditionlist->{$conditionkey}.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$object_failed_validation = 1;
}
}
}
# TODO - need to get back to this
if (0) {
#
# check to see if all the attrs got set
#
my @errlist;
foreach $a (@attrprovided) {
# is this attr was not set then add it to the error list
if (!grep(/^$a$/, @setattrlist)) {
push(@errlist, $a);
$ret = 2;
}
if ($object_failed_validation) {
$objhash{$objname}{error} = 1;
delete $objhash{$objname}{updated};
foreach my $table (keys %allupdates) {
delete $allupdates{$table}{$objname};
}
if ($ret == 2) {
my $rsp;
$rsp->{data}->[0] = "Could not set the following attributes for the \'$objname\' definition in the xCAT database: \'@errlist\'";
xCAT::MsgUtils->message("E", $rsp, $::callback);
}
$ret = 1;
}
} # end - foreach object
@@ -2766,4 +2788,107 @@ sub collapsenicsattr()
}
}
#----------------------------------------------------------------------------
=head3 validate_only_if_attrs
Validate Schema.pm only_if rules for attributes being set on one
object. The object is valid when any schema entry for the attribute is
unconditional or when one only_if condition is satisfied by the new
object attributes, existing database attributes, or group attributes.
Arguments:
$objname - object name
$type - object type
$attrs_ref - attributes being set
$dbattrs_ref - existing attributes to allow as conditions
$groupattrs_ref - optional group attributes, keyed by group name
Returns:
@failures:
({ attr => ..., message => ... }, ...)
=cut
#-----------------------------------------------------------------------------
sub validate_only_if_attrs
{
my ($class, $objname, $type, $attrs_ref, $dbattrs_ref, $groupattrs_ref) = @_;
my @failures;
my $datatype = $xCAT::Schema::defspec{$type};
return @failures unless $datatype;
$attrs_ref ||= {};
$dbattrs_ref ||= {};
my %groupattrs;
if ($groupattrs_ref) {
%groupattrs = %$groupattrs_ref;
} elsif ($attrs_ref->{groups}) {
my %groups;
foreach my $group (split(",", $attrs_ref->{groups})) {
$groups{$group} = "group";
}
%groupattrs = xCAT::DBobjUtils->getobjdefs(\%groups) if %groups;
}
my %attr_has_unconditional;
my %attr_has_valid_condition;
my %invalidattr;
my %conditionlist;
foreach my $this_attr (@{ $datatype->{'attrs'} }) {
my $attr_name = $this_attr->{attr_name};
next unless defined($attrs_ref->{$attr_name});
if (!exists($this_attr->{only_if})) {
$attr_has_unconditional{$attr_name} = 1;
next;
}
next unless $this_attr->{only_if} =~ /^([^!=]+)=(.+)$/;
my ($check_attr, $check_value) = ($1, $2);
if (_only_if_value_matches($attrs_ref, $dbattrs_ref, \%groupattrs, $check_attr, $check_value)) {
$attr_has_valid_condition{$attr_name} = 1;
next;
}
next if $attr_has_valid_condition{$attr_name};
$invalidattr{$attr_name}{condition} = $check_attr;
$conditionlist{$check_attr}{$check_value} = 1;
}
foreach my $att (keys %invalidattr) {
next if $attr_has_unconditional{$att};
next if $attr_has_valid_condition{$att};
my $conditionkey = $invalidattr{$att}{condition};
my $condlist = join(" or ", sort keys %{ $conditionlist{$conditionkey} });
push @failures, {
attr => $att,
message => "Cannot set the attr=\'$att\' attribute unless $conditionkey value is $condlist.",
};
}
return @failures;
}
sub _only_if_value_matches
{
my ($attrs_ref, $dbattrs_ref, $groupattrs_ref, $check_attr, $check_value) = @_;
foreach my $source ($attrs_ref, $dbattrs_ref) {
next unless $source;
my $value = $source->{$check_attr};
return 1 if defined($value) && $value =~ /\b\Q$check_value\E\b/;
}
foreach my $group (keys %{ $groupattrs_ref || {} }) {
my $value = $groupattrs_ref->{$group}{$check_attr};
return 1 if defined($value) && $value =~ /\b\Q$check_value\E\b/;
}
return 0;
}
1;
+104 -22
View File
@@ -1671,14 +1671,23 @@ sub defmk
# Pull all the pieces together for the final hash
# - combines the command line attrs and input file attrs if provided
#
my $command_level_error = $error ? 1 : 0;
if (&setFINALattrs != 0)
{
$error = 1;
$command_level_error = 1;
}
if ($command_level_error)
{
%::FINALATTRS = ();
}
# If no object definitions ended up in FINALATTRS, there is nothing to create
if (!$error && !%::FINALATTRS)
if (!%::FINALATTRS)
{
return 1 if $error;
my $rsp;
my $is_node_type = (grep { $_ eq "node" } @::finalTypeList) ||
($::opt_t && $::opt_t =~ /\bnode\b/);
@@ -1790,34 +1799,107 @@ sub defmk
}
# if object already exists
if (defined($objTypeListsHash{$type}{$obj}) && ($objTypeListsHash{$type}{$obj} == 1))
my $object_exists = (defined($objTypeListsHash{$type}{$obj}) && ($objTypeListsHash{$type}{$obj} == 1));
if ($object_exists && !$::opt_f)
{
if ($::opt_f)
# won't remove the old one unless the force option is used
my $rsp;
$rsp->{data}->[0] = "A definition for \'$obj\' already exists. No changes will be made. Run again with \'-f\' option to force replace.";
xCAT::MsgUtils->message("W", $rsp, $::callback);
delete $::FINALATTRS{$obj};
next;
}
# Reject object-level validation failures before -f removes an old
# definition or group handling mutates member nodes.
if (($type eq "node") && (!defined($::FINALATTRS{$obj}{groups}) || !$::FINALATTRS{$obj}{groups}))
{
my $rsp;
$rsp->{data}->[0] = "Attribute \'groups\' is not specified for node \'$obj\', skipping to the next node.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
if ($type eq 'group')
{
my $is_dynamic_group = 0;
if (!$::FINALATTRS{$obj}{grouptype})
{
# remove the old object
my %objhash;
$objhash{$obj} = $type;
if (xCAT::DBobjUtils->rmobjdefs(\%objhash) != 0)
if ($::opt_d)
{
$error = 1;
my $rsp;
$rsp->{data}->[0] = "Could not remove the definition for \'$obj\'.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
if (scalar(keys %{ $::FINALATTRS{$obj} }) > 1)
{
my $rsp;
$rsp->{data}->[0] = "Can not assign attributes to dynamic node group \'$obj\'.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
$is_dynamic_group = 1;
}
}
else
elsif ($::FINALATTRS{$obj}{grouptype} eq 'dynamic')
{
# won't remove the old one unless the force option is used
my $rsp;
$rsp->{data}->[0] = "A definition for \'$obj\' already exists. No changes will be made. Run again with \'-f\' option to force replace.";
xCAT::MsgUtils->message("W", $rsp, $::callback);
delete $::FINALATTRS{$obj};
next;
$is_dynamic_group = 1;
}
if ($is_dynamic_group && !$::FINALATTRS{$obj}{wherevals} && !$::opt_w)
{
my $rsp;
$rsp->{data}->[0] = "The \'where\' attributes and values were not provided for dynamic group \'$obj\'.";
$rsp->{data}->[1] = "Skipping to the next group.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
if (!$is_dynamic_group && $::opt_w && $::FINALATTRS{$obj}{members})
{
my $rsp;
$rsp->{data}->[0] = "Cannot use a list of members together with the \'-w\' option.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
}
my @only_if_failures = xCAT::DBobjUtils->validate_only_if_attrs(
$obj,
$type,
$::FINALATTRS{$obj},
{},
);
if (@only_if_failures)
{
foreach my $failure (@only_if_failures) {
my $rsp;
$rsp->{data}->[0] = $failure->{message};
xCAT::MsgUtils->message("E", $rsp, $::callback);
}
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
if ($object_exists && $::opt_f)
{
# remove the old object after the replacement definition is known valid
my %objhash;
$objhash{$obj} = $type;
if (xCAT::DBobjUtils->rmobjdefs(\%objhash) != 0)
{
$error = 1;
my $rsp;
$rsp->{data}->[0] = "Could not remove the definition for \'$obj\'.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
delete $::FINALATTRS{$obj};
next;
}
}
# need to handle group definitions - special!
@@ -2021,6 +2103,7 @@ sub defmk
$rsp->{data}->[0] = "Attribute \'groups\' is not specified for node \'$obj\', skipping to the next node.";
xCAT::MsgUtils->message("E", $rsp, $::callback);
$error = 1;
delete $::FINALATTRS{$obj};
next;
}
@@ -4835,4 +4918,3 @@ sub isobjnamevalid{
}
1;
+161 -2
View File
@@ -415,6 +415,7 @@ end
start:mkdef_node_no_attrs
description:mkdef -t node with no attributes should fail and mention groups
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_noattr
cmd:mkdef -t node -o testnode_noattr
check:rc!=0
check:output=~groups
@@ -425,12 +426,13 @@ end
start:mkdef_node_no_groups
description:mkdef -t node with attributes but no groups should fail
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_nogrp
cmd:mkdef -t node -o testnode_nogrp ip=10.0.0.99
check:rc!=0
check:output=~groups
cmd:lsdef testnode_nogrp
check:rc!=0
cmd:rmdef -t node -o testnode_nogrp
cmd:rmdef -t node testnode_nogrp
end
start:mkdef_nonnode_no_attrs
@@ -439,6 +441,163 @@ label:mn_only,ci_test,db
cmd:mkdef -t network -o testnet_noattr
check:rc!=0
check:output=~no attributes were specified
cmd:rmdef -t network -o testnet_noattr
cmd:rmdef -t network testnet_noattr
end
start:mkdef_validation_error_no_partial_object
description:mkdef should not create a partial node when an attribute fails validation
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_partial
cmd:mkdef -t node -o testnode_partial groups=test bmc=10.0.0.1
check:rc!=0
check:output=~Cannot set
cmd:lsdef testnode_partial
check:rc!=0
cmd:rmdef -t node testnode_partial
end
start:mkdef_f_validation_error_preserves_old
description:mkdef -f with validation error should preserve original object
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_keepme
cmd:mkdef -t node -o testnode_keepme groups=oldgroup mgt=openbmc bmc=1.2.3.4
check:rc==0
cmd:mkdef -f -t node -o testnode_keepme groups=newgroup bmc=5.6.7.8
check:rc!=0
cmd:lsdef -i groups testnode_keepme
check:rc==0
check:output=~groups=oldgroup
cmd:lsdef -i mgt testnode_keepme
check:rc==0
check:output=~mgt=openbmc
cmd:lsdef -i bmc testnode_keepme
check:rc==0
check:output=~bmc=1.2.3.4
cmd:rmdef -t node testnode_keepme
end
start:mkdef_f_missing_groups_preserves_old
description:mkdef -f without groups should preserve original node
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_keepgrp
cmd:mkdef -t node -o testnode_keepgrp groups=oldgroup mgt=openbmc bmc=1.2.3.4
check:rc==0
cmd:mkdef -f -t node -o testnode_keepgrp mgt=openbmc bmc=5.6.7.8
check:rc!=0
cmd:lsdef -i groups testnode_keepgrp
check:rc==0
check:output=~groups=oldgroup
cmd:rmdef -t node testnode_keepgrp
end
start:mkdef_only_if_satisfied_explicit
description:mkdef with only_if condition explicitly satisfied should succeed
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_onlyif
cmd:mkdef -t node -o testnode_onlyif groups=test mgt=openbmc bmc=10.0.0.1
check:rc==0
cmd:lsdef -i bmc testnode_onlyif
check:rc==0
check:output=~bmc=10.0.0.1
cmd:rmdef -t node testnode_onlyif
end
start:mkdef_inherited_validation_via_group
description:mkdef with only_if satisfied by group inheritance should write the attr
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_inherit
cmd:chtab node=openbmcgrp nodehm.mgt=openbmc
check:rc==0
cmd:mkdef -t node -o testnode_inherit groups=openbmcgrp bmc=10.0.0.1
check:rc==0
cmd:lsdef -i bmc testnode_inherit
check:rc==0
check:output=~bmc=10.0.0.1
cmd:rmdef -t node testnode_inherit
cmd:chtab -d node=openbmcgrp nodehm
end
start:mkdef_f_only_if_failure_preserves_old_bmc
description:mkdef -f with only_if failure should preserve original bmc value
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_keepbmc
cmd:mkdef -t node -o testnode_keepbmc groups=oldgroup mgt=openbmc bmc=1.2.3.4
check:rc==0
cmd:mkdef -f -t node -o testnode_keepbmc groups=newgroup bmc=5.6.7.8
check:rc!=0
cmd:lsdef -i bmc testnode_keepbmc
check:rc==0
check:output=~bmc=1.2.3.4
cmd:rmdef -t node testnode_keepbmc
end
start:mkdef_multi_object_valid_invalid
description:single mkdef command with one valid and one invalid object (per-object atomic)
label:mn_only,ci_test,db
cmd:rmdef -t node testnode_good
cmd:rmdef -t node testnode_bad
cmd:printf "testnode_good:\n objtype=node\n groups=test\n mgt=openbmc\n bmc=10.0.0.1\n\ntestnode_bad:\n objtype=node\n groups=test\n bmc=10.0.0.2\n" | mkdef -z
check:rc!=0
cmd:lsdef -i bmc testnode_good
check:rc==0
check:output=~bmc=10.0.0.1
cmd:lsdef testnode_bad
check:rc!=0
cmd:rmdef -t node testnode_good
cmd:rmdef -t node testnode_bad
end
start:mkdef_f_dynamic_group_missing_wherevals_preserves_old
description:mkdef -f with grouptype=dynamic but no wherevals should preserve old group
label:mn_only,ci_test,db
cmd:rmdef -t node testmember1
cmd:rmdef -t group testkeepgrp
cmd:mkdef -t node -o testmember1 groups=all
check:rc==0
cmd:mkdef -t group -o testkeepgrp members=testmember1
check:rc==0
cmd:mkdef -f -t group -o testkeepgrp grouptype=dynamic
check:rc!=0
cmd:lsdef -t group testkeepgrp
check:rc==0
cmd:lsdef -s testkeepgrp
check:output=~testmember1
cmd:rmdef -t group testkeepgrp
cmd:rmdef -t node testmember1
end
start:mkdef_f_dynamic_group_members_w_conflict_preserves_old
description:mkdef -f -d -w with members should preserve old group
label:mn_only,ci_test,db
cmd:rmdef -t node testmember2
cmd:rmdef -t group testkeepgrp2
cmd:mkdef -t node -o testmember2 groups=all
check:rc==0
cmd:mkdef -t group -o testkeepgrp2 members=testmember2
check:rc==0
cmd:mkdef -f -t group -o testkeepgrp2 -d -w mgt==openbmc members=testmember2
check:rc!=0
cmd:lsdef -t group testkeepgrp2
check:rc==0
cmd:lsdef -s testkeepgrp2
check:output=~testmember2
cmd:rmdef -t group testkeepgrp2
cmd:rmdef -t node testmember2
end
start:mkdef_failed_group_no_member_mutation
description:failed group creation should not mutate member node groups
label:mn_only,ci_test,db
cmd:rmdef -t node testmember3
cmd:rmdef -t group testbadgrp
cmd:mkdef -t node -o testmember3 groups=all
check:rc==0
cmd:mkdef -t group -o testbadgrp members=testmember3 bmc=10.0.0.1
check:rc!=0
cmd:lsdef -t group testbadgrp
check:rc!=0
cmd:lsdef -i groups testmember3
check:output!~testbadgrp
cmd:rmdef -t node testmember3
cmd:rmdef -t group testbadgrp
end
+52
View File
@@ -0,0 +1,52 @@
#!/usr/bin/env perl
use strict;
use warnings;
BEGIN {
$ENV{XCATCFG} ||= 'SQLite:/tmp';
}
use FindBin;
use lib "$FindBin::Bin/../../perl-xCAT";
use Test::More;
use xCAT::DBobjUtils;
my %missing_mgt = (
objtype => 'node',
groups => 'test',
bmc => '10.0.0.1',
);
my @failures = xCAT::DBobjUtils->validate_only_if_attrs('node01', 'node', \%missing_mgt, {});
is(scalar @failures, 1, 'bmc without mgt fails only_if validation');
like($failures[0]->{message}, qr/mgt value is .*openbmc/, 'failure explains accepted mgt values');
my %explicit_openbmc = (
objtype => 'node',
groups => 'test',
mgt => 'openbmc',
bmc => '10.0.0.1',
);
@failures = xCAT::DBobjUtils->validate_only_if_attrs('node01', 'node', \%explicit_openbmc, {});
is(scalar @failures, 0, 'explicit mgt=openbmc satisfies bmc only_if validation');
my %existing_openbmc = (
objtype => 'node',
groups => 'test',
bmc => '10.0.0.1',
);
my %dbattrs = (mgt => 'openbmc');
@failures = xCAT::DBobjUtils->validate_only_if_attrs('node01', 'node', \%existing_openbmc, \%dbattrs);
is(scalar @failures, 0, 'existing mgt=openbmc satisfies bmc only_if validation');
my %group_openbmc = (
objtype => 'node',
groups => 'openbmcgrp',
bmc => '10.0.0.1',
);
my %groupattrs = (openbmcgrp => { mgt => 'openbmc' });
@failures = xCAT::DBobjUtils->validate_only_if_attrs('node01', 'node', \%group_openbmc, {}, \%groupattrs);
is(scalar @failures, 0, 'group mgt=openbmc satisfies bmc only_if validation');
done_testing();