From 8f043e10030a5d3856a67412108a636b726375a0 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 11 Jun 2026 16:00:25 -0500 Subject: [PATCH 1/2] Added acceptance tests for classes promise cancel attribute Covers undefining a defined class when the cancel expression is true, retaining the class when the expression is false, function-based triggers, the no-op case for an undefined class, refusal to cancel reserved hard classes, and mutual exclusion with the class-defining attributes. Ticket: CFE-4686 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) --- .../02_classes/01_basic/cancel_attribute.cf | 65 +++++++++++++++++++ .../01_basic/cancel_attribute_hardclass.cf | 48 ++++++++++++++ .../01_basic/cancel_attribute_mutex.cf | 43 ++++++++++++ .../01_basic/cancel_attribute_mutex.cf.sub | 18 +++++ 4 files changed, 174 insertions(+) create mode 100644 tests/acceptance/02_classes/01_basic/cancel_attribute.cf create mode 100644 tests/acceptance/02_classes/01_basic/cancel_attribute_hardclass.cf create mode 100644 tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf create mode 100644 tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf.sub diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute.cf b/tests/acceptance/02_classes/01_basic/cancel_attribute.cf new file mode 100644 index 0000000000..92f083d3d1 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute.cf @@ -0,0 +1,65 @@ +####################################################### +# +# Test the 'cancel' attribute of classes promises (CFE-4686). +# +# A classes promise using 'cancel' undefines its promiser class when the +# cancel expression evaluates to true. Unlike the class-defining attributes, +# it must be evaluated even when the class is already defined. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### + +bundle agent init +{ +} + +####################################################### + +bundle agent test +{ + meta: + "description" -> { "CFE-4686" } + string => "classes promise 'cancel' attribute undefines a defined class when its expression is true"; + + classes: + # Define global classes so the check bundle can observe the outcome. + "to_cancel" expression => "any", scope => "namespace"; + "to_keep" expression => "any", scope => "namespace"; + "trigger" expression => "any", scope => "namespace"; + "func_cancel" expression => "any", scope => "namespace"; + + # trigger is set, so to_cancel must be undefined. + "to_cancel" cancel => "trigger"; + + # Condition is an undefined class, so to_keep must be retained. + "to_keep" cancel => "class_that_is_not_defined"; + + # Function-based boolean trigger: the file does not exist, so not(...) + # is true and func_cancel must be undefined. + "func_cancel" cancel => not(fileexists("/this/does/not/exist/cfe4686")); + + # Cancelling a class that was never defined is a harmless no-op. + "never_defined" cancel => "trigger"; +} + +####################################################### + +bundle agent check +{ + classes: + "ok" expression => "!to_cancel.to_keep.!func_cancel.!never_defined.trigger"; + + reports: + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_hardclass.cf b/tests/acceptance/02_classes/01_basic/cancel_attribute_hardclass.cf new file mode 100644 index 0000000000..dd0927d393 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_hardclass.cf @@ -0,0 +1,48 @@ +####################################################### +# +# Test that the 'cancel' attribute cannot undefine a hard class (CFE-4686). +# +# Cancelling a reserved hard class must be refused (with a warning) and the +# class left in place, consistent with the cancel_* classes body attributes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### + +bundle agent init +{ +} + +####################################################### + +bundle agent test +{ + meta: + "description" -> { "CFE-4686" } + string => "classes promise 'cancel' attribute must not undefine a hard class"; + + classes: + # cfengine is a reserved hard class - the cancel must be ignored. + "cfengine" cancel => "any"; +} + +####################################################### + +bundle agent check +{ + classes: + "ok" expression => "cfengine"; + + reports: + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf b/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf new file mode 100644 index 0000000000..fded2a150f --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf @@ -0,0 +1,43 @@ +####################################################### +# +# Test that 'cancel' is mutually exclusive with the class-defining +# attributes of a classes promise (CFE-4686). +# +# Combining 'cancel' with expression/and/or/xor/not/dist/select_class must be +# rejected with "Irreconcilable constraints", exactly like combining any two +# of the defining attributes. The sub-policy exercises every combination; this +# test asserts the rejection appears in the agent output. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +####################################################### + +bundle agent init +{ +} + +####################################################### + +bundle agent test +{ + meta: + "description" -> { "CFE-4686" } + string => "'cancel' must be mutually exclusive with the class-defining attributes"; +} + +####################################################### + +bundle agent check +{ + methods: + "" usebundle => dcs_passif_output1(".*Irreconcilable constraints in classes.*", + "$(sys.cf_agent) -Kf $(this.promise_filename).sub 2>&1", + $(this.promise_filename)); +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf.sub b/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf.sub new file mode 100644 index 0000000000..b6174a84f4 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_mutex.cf.sub @@ -0,0 +1,18 @@ +body common control +{ + bundlesequence => { "test" }; +} + +bundle agent test +{ + classes: + # 'cancel' paired with each class-DEFINING attribute must be rejected + # with "Irreconcilable constraints". + "c_expression" cancel => "any", expression => "any"; + "c_and" cancel => "any", and => { "any" }; + "c_or" cancel => "any", or => { "any" }; + "c_xor" cancel => "any", xor => { "any" }; + "c_not" cancel => "any", not => "any"; + "c_dist" cancel => "any", dist => { "1", "1" }; + "c_select" cancel => "any", select_class => { "x", "y" }; +} From bf85569a6b983d71ee0d88bb15dff18d9a3c6474 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 11 Jun 2026 16:00:50 -0500 Subject: [PATCH 2/2] Added cancel attribute to classes promises to undefine a class Until now a classes promise could only define a class; the only way to undefine one from policy was a cancel_* classes body attached to another promise's outcome. This adds a 'cancel' attribute to the classes promise itself: when its class expression (a string, or a function returning a boolean) evaluates true, the promiser class is undefined if it is defined. The class-skipping optimization in ExpandDeRefPromise() normally excludes a classes promise whose promiser is already defined. A cancel promise must run precisely in that case, so the skip now makes an exception when the 'cancel' attribute is present. 'cancel' is mutually exclusive with the class-defining attributes (expression, and, or, xor, not, dist, select_class) via the existing "Irreconcilable constraints" check, since it is counted like any other class-body attribute. Reserved hard classes cannot be cancelled. The evaluation and undefine path mirrors the cancel_* classes body behaviour (DeleteAllClasses). Ticket: CFE-4686 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) --- libpromises/eval_context.h | 1 + libpromises/mod_common.c | 1 + libpromises/promises.c | 8 +- libpromises/verify_classes.c | 159 +++++++++++++++++++++++++++++------ 4 files changed, 141 insertions(+), 28 deletions(-) diff --git a/libpromises/eval_context.h b/libpromises/eval_context.h index 6e7699ba59..38d79eb9c6 100644 --- a/libpromises/eval_context.h +++ b/libpromises/eval_context.h @@ -169,6 +169,7 @@ bool EvalContextClassPutHard(EvalContext *ctx, const char *name, const char *tag Class *EvalContextClassGet(const EvalContext *ctx, const char *ns, const char *name); Class *EvalContextClassMatch(const EvalContext *ctx, const char *regex); bool EvalContextClassRemove(EvalContext *ctx, const char *ns, const char *name); +void EvalContextStackFrameRemoveSoft(EvalContext *ctx, const char *context); StringSet *EvalContextClassTags(const EvalContext *ctx, const char *ns, const char *name); ClassTableIterator *EvalContextClassTableIteratorNewGlobal(const EvalContext *ctx, const char *ns, bool is_hard, bool is_soft); diff --git a/libpromises/mod_common.c b/libpromises/mod_common.c index 3d41cd4270..434fefc0e6 100644 --- a/libpromises/mod_common.c +++ b/libpromises/mod_common.c @@ -219,6 +219,7 @@ const ConstraintSyntax CF_CLASSBODY[] = { ConstraintSyntaxNewOption("scope", "namespace,bundle", "Scope of the class set by this promise", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("and", "Combine class sources with AND", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewContext("cancel", "Undefine the promiser class when this string expression of classes evaluates to true", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewRealList("dist", "Generate a probabilistic class distribution (from strategies in cfengine 2)", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContext("expression", "Evaluate string expression of classes in normal form", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("or", "Combine class sources with inclusive OR", SYNTAX_STATUS_NORMAL), diff --git a/libpromises/promises.c b/libpromises/promises.c index 4c917ef581..46f8b047cf 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -704,10 +704,14 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) pcopy->conlist = SeqNew(10, ConstraintDestroy); pcopy->org_pp = pp->org_pp; - // if this is a class promise, check if it is already set, if so, skip + // if this is a class promise, check if it is already set, if so, skip. + // Exception: a promise using the 'cancel' attribute must be evaluated + // precisely when the class is already defined, since its purpose is to + // undefine it. if (strcmp("classes", PromiseGetPromiseType(pp)) == 0) { - if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser))) + if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser)) && + PromiseGetConstraint(pp, "cancel") == NULL) { Log(LOG_LEVEL_DEBUG, "Skipping evaluation of classes promise as class '%s' is already set", diff --git a/libpromises/verify_classes.c b/libpromises/verify_classes.c index 7f903e2eb6..d806aac8ae 100644 --- a/libpromises/verify_classes.c +++ b/libpromises/verify_classes.c @@ -38,6 +38,8 @@ static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise *pp); +static bool EvalClassConstraintRval(EvalContext *ctx, Constraint *cp, const Promise *pp); +static PromiseResult VerifyClassCancel(EvalContext *ctx, const Promise *pp, Attributes *a); static bool ValidClassName(const char *str) { @@ -73,6 +75,16 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED return PROMISE_RESULT_FAIL; } + /* The 'cancel' attribute undefines the promiser class when its expression + * evaluates true. It must be handled before the class-defining path + * because, by design, it operates on a class that is already defined and + * would otherwise be skipped (see EvalClassExpression()). */ + if (a.context.expression != NULL && + strcmp(a.context.expression->lval, "cancel") == 0) + { + return VerifyClassCancel(ctx, pp, &a); + } + if (a.context.expression == NULL || EvalClassExpression(ctx, a.context.expression, pp)) { @@ -295,32 +307,18 @@ static bool EvalBoolCombination(EvalContext *ctx, const Rlist *list, return result; } -static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise *pp) +/* + Expand the RHS of a class-context constraint in place and evaluate it as a + boolean. Shared by the class-defining attributes (expression, not, and, or, + xor, select_class, dist) and by the class-cancelling 'cancel' attribute, + whose trigger is evaluated exactly like 'expression'. This deliberately does + NOT consider whether the promiser class is already defined; that skip logic + is specific to class definition and lives in EvalClassExpression(). +*/ +static bool EvalClassConstraintRval(EvalContext *ctx, Constraint *cp, const Promise *pp) { - assert(pp); - - if (cp == NULL) // ProgrammingError ? We'll crash RSN anyway ... - { - Log(LOG_LEVEL_ERR, - "EvalClassExpression internal diagnostic discovered an ill-formed condition"); - } - - if (!IsDefinedClass(ctx, pp->classes)) - { - return false; - } - - if (IsDefinedClass(ctx, pp->promiser)) - { - if (PromiseGetConstraintAsInt(ctx, "persistence", pp) == 0) - { - Log(LOG_LEVEL_VERBOSE, - " ?> Cancelling cached persistent class %s", - pp->promiser); - EvalContextHeapPersistentRemove(pp->promiser); - } - return false; - } + assert(cp != NULL); + assert(pp != NULL); switch (cp->rval.type) { @@ -353,7 +351,11 @@ static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise break; } - if (strcmp(cp->lval, "expression") == 0) + /* The 'cancel' trigger is a plain class expression, evaluated like + * 'expression': true when the named class(es) resolve to a defined + * context. */ + if (strcmp(cp->lval, "expression") == 0 || + strcmp(cp->lval, "cancel") == 0) { return (cp->rval.type == RVAL_TYPE_SCALAR && IsDefinedClass(ctx, RvalScalarValue(cp->rval))); @@ -401,3 +403,108 @@ static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise return false; } + +static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise *pp) +{ + assert(pp != NULL); + assert(cp != NULL); + + /* assert() is compiled out under NDEBUG, so guard the pointer + * dereferences below with an explicit runtime check as well. */ + if (pp == NULL || cp == NULL) + { + Log(LOG_LEVEL_ERR, + "EvalClassExpression internal diagnostic discovered an ill-formed condition"); + return false; + } + + if (!IsDefinedClass(ctx, pp->classes)) + { + return false; + } + + if (IsDefinedClass(ctx, pp->promiser)) + { + if (PromiseGetConstraintAsInt(ctx, "persistence", pp) == 0) + { + Log(LOG_LEVEL_VERBOSE, + " ?> Cancelling cached persistent class %s", + pp->promiser); + EvalContextHeapPersistentRemove(pp->promiser); + } + return false; + } + + return EvalClassConstraintRval(ctx, cp, pp); +} + +/* + Handle a classes promise that uses the 'cancel' attribute. Unlike the + class-defining attributes, a cancel promise must be evaluated even when the + promiser class is already defined - that is the whole point - so it bypasses + the "already defined" skip in EvalClassExpression(). When the cancel + expression evaluates true and the promiser class is currently defined, the + class is undefined, mirroring how classes bodies cancel classes via + DeleteAllClasses(). +*/ +static PromiseResult VerifyClassCancel(EvalContext *ctx, const Promise *pp, Attributes *a) +{ + assert(ctx != NULL); + assert(pp != NULL); + assert(a != NULL); + + Constraint *cp = a->context.expression; + assert(cp != NULL); + + /* Respect the promise's class context guard. */ + if (!IsDefinedClass(ctx, pp->classes)) + { + return PROMISE_RESULT_NOOP; + } + + if (!ValidClassName(pp->promiser)) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Attempted to cancel a class '%s', which is an illegal class identifier", + pp->promiser); + return PROMISE_RESULT_FAIL; + } + + /* Hard classes describe the running system and must never be undefined + * (consistent with how the cancel_* class body attributes ignore them). + * Warn and leave the class in place rather than failing the promise. */ + const Class *promiser_class = EvalContextClassGet(ctx, NULL, pp->promiser); + if (promiser_class != NULL && !promiser_class->is_soft) + { + Log(LOG_LEVEL_WARNING, + "Cannot cancel reserved hard class '%s'", pp->promiser); + return PROMISE_RESULT_NOOP; + } + + /* Evaluate the cancel trigger. If it is false, leave the class as-is. */ + if (!EvalClassConstraintRval(ctx, cp, pp)) + { + return PROMISE_RESULT_NOOP; + } + + /* Trigger is true: undefine the promiser class if it is defined. */ + if (!IsDefinedClass(ctx, pp->promiser)) + { + Log(LOG_LEVEL_VERBOSE, + "C: - Class '%s' targeted by cancel is not defined, nothing to do", + pp->promiser); + return PROMISE_RESULT_NOOP; + } + + Log(LOG_LEVEL_VERBOSE, "C: - Cancelling class: %s", pp->promiser); + + EvalContextHeapPersistentRemove(pp->promiser); + { + ClassRef ref = ClassRefParse(pp->promiser); + EvalContextClassRemove(ctx, ref.ns, ref.name); + ClassRefDestroy(ref); + } + EvalContextStackFrameRemoveSoft(ctx, pp->promiser); + + return PROMISE_RESULT_NOOP; +}