fix: prevent mkdef from creating partial objects on validation error

viniciusferrao · viniciusferrao · commit 1a7df26709a2 · 2026-04-27T13:06:36.000-03:00
Add preflight validation to defmk that runs before any destructive operations. Objects that fail only_if, groups-required, or group- specific checks are removed from FINALATTRS before the OBJ loop, preventing data loss from failed forced replacements and partial object creation. Semantics: - mkdef is per-object atomic. In a multi-object command, valid objects are created and invalid ones are rejected (command returns non-zero). - Command-level errors (e.g. from setFINALattrs) block ALL objects. Per-object preflight failures only block that specific object. - mkdef -f: old object deleted only after replacement passes preflight. - chdef: unchanged. Partial update semantics preserved. DBobjUtils.pm: - Add preflight_validate_mkdef() — checks only_if, groups requirement, dynamic group attrs/wherevals, static group members+-w conflict. Detects dynamic groups via both opt_d and grouptype=dynamic. - Fix setobjdefs only_if main loop to check group inheritance, not just the post-loop error handler. Ensures inherited attrs are written. - Remove dead if(0) block (disabled since 2009 refactor). DBobjectdefs.pm: - Separate command-level errors from per-object preflight failures. - Call preflight unconditionally. Failed objects removed per-object. - Command-level errors clear all FINALATTRS (whole-command atomic). - Empty-FINALATTRS early return for no-attributes case. Note: CLI attribute name validation is not in scope. Unknown attrs passed on the command line (not via -z) are silently ignored by setFINALattrs — this is pre-existing behavior. Fixes xcat2#3706 Fixes xcat2#2765
diff --git a/perl-xCAT/xCAT/DBobjUtils.pm b/perl-xCAT/xCAT/DBobjUtils.pm
@@ -1032,7 +1032,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 +1072,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;
@@ -2782,6 +2801,54 @@ sub preflight_validate_mkdef
             }
         }
 
+        # Check: group-specific constraints
+        # Mirrors the group-handling logic in defmk's OBJ loop exactly:
+        # 1. If grouptype not set by user:
+        #    - opt_d: reject if user attrs beyond objtype exist
+        #    - else: treat as static
+        # 2. If dynamic (via opt_d or grouptype=dynamic): require wherevals
+        # 3. If static: reject members + -w conflict
+        if ($type eq 'group') {
+            my $grouptype_set = defined($objhash_ref->{$objname}{grouptype}) &&
+                                $objhash_ref->{$objname}{grouptype};
+            my $is_dynamic = 0;
+
+            if (!$grouptype_set) {
+                if ($opts{opt_d}) {
+                    # -d flag: reject if any user-provided attrs beyond objtype
+                    # This matches: if (scalar(keys %{$::FINALATTRS{$obj}}) > 1)
+                    if (scalar(keys %{$objhash_ref->{$objname}}) > 1) {
+                        push @{$failures{$objname}}, {
+                            attr    => 'group',
+                            message => "Can not assign attributes to dynamic node group \'$objname\'.",
+                        };
+                    }
+                    $is_dynamic = 1;
+                }
+                # else: static, handled below
+            } elsif ($objhash_ref->{$objname}{grouptype} eq 'dynamic') {
+                $is_dynamic = 1;
+            }
+
+            if ($is_dynamic) {
+                # Dynamic groups require wherevals or -w
+                if (!$objhash_ref->{$objname}{wherevals} && !$opts{opt_w}) {
+                    push @{$failures{$objname}}, {
+                        attr    => 'wherevals',
+                        message => "The \'where\' attributes and values were not provided for dynamic group \'$objname\'.",
+                    };
+                }
+            } else {
+                # Static groups cannot combine members list with -w
+                if ($opts{opt_w} && $objhash_ref->{$objname}{members}) {
+                    push @{$failures{$objname}}, {
+                        attr    => 'members',
+                        message => "Cannot use a list of members together with the \'-w\' option.",
+                    };
+                }
+            }
+        }
+
         # Check: only_if conditions from Schema
         my $datatype = $xCAT::Schema::defspec{$type};
         next unless $datatype;
diff --git a/xCAT-server/lib/xcat/plugins/DBobjectdefs.pm b/xCAT-server/lib/xcat/plugins/DBobjectdefs.pm
@@ -1671,21 +1671,37 @@ sub defmk
     #   Pull all the pieces together for the final hash
     #        - combines the command line attrs and input file attrs if provided
     #
+    # Track command-level errors separately from per-object failures.
+    # Command-level errors (invalid attr names, bad type) mean the
+    # entire command's attribute set is suspect and no object should
+    # be created. Per-object failures (only_if, missing groups) only
+    # reject that specific object.
+    my $command_level_error = 0;
     if (&setFINALattrs != 0)
     {
         $error = 1;
+        $command_level_error = 1;
+    }
+
+    my $numobjrequest = scalar(@::allobjnames);
+
+    # If the command itself is invalid, do not proceed with any objects.
+    if ($command_level_error && %::FINALATTRS)
+    {
+        %::FINALATTRS = ();
     }
 
     # Preflight validation: check all object-level reject conditions
     # before any destructive operations (-f deletion, group mutation).
     # Objects that fail are removed from FINALATTRS so they never reach
-    # rmobjdefs() or group member mutation.
-    my $numobjrequest = scalar(@::allobjnames);
-    if (!$error && %::FINALATTRS)
+    # rmobjdefs() or group member mutation. Valid objects proceed.
+    if (%::FINALATTRS)
     {
         my %preflight_failures = xCAT::DBobjUtils->preflight_validate_mkdef(
             \%::FINALATTRS,
             use_group_attrs => 1,
+            opt_d           => $::opt_d,
+            opt_w           => $::opt_w,
         );
 
         foreach my $obj (keys %preflight_failures) {
@@ -1699,8 +1715,8 @@ sub defmk
         }
     }
 
-    # If no objects survived preflight, return early with useful message
-    if (!$error && !%::FINALATTRS && $numobjrequest > 0)
+    # If no objects survived, return early with useful message
+    if (!%::FINALATTRS && $numobjrequest > 0)
     {
         my $is_node_type = (grep { $_ eq "node" } @::finalTypeList) ||
                            ($::opt_t && $::opt_t =~ /\bnode\b/);
diff --git a/xCAT-test/autotest/testcase/mkdef/cases0 b/xCAT-test/autotest/testcase/mkdef/cases0
@@ -501,3 +501,103 @@ 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
