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 7b1a59967a..94a74ac273 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), + ConstraintSyntaxNewBool("cancel", "true/false undefine the promiser class. Default value: false", 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 93e3bc7739..c86d335ca3 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -704,14 +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 - // Exception: persistent classes with the reset timer policy must not - // be skipped — the promise needs to fire so the timer gets reset. - // reset is the default, so only an explicit timer_policy => "absolute" - // restores the skip. + // if this is a class promise, check if it is already set, if so, skip. + // Two cases must still fire when the class is already set: + // - 'cancel' => undefine the class + // - persistent class with reset timer policy => reset the timer if (strcmp("classes", PromiseGetPromiseType(pp)) == 0) { - if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser))) + if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser)) && + PromiseGetConstraint(pp, "cancel") == NULL) { const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); int persistence = PromiseGetConstraintAsInt(ctx, "persistence", pp); diff --git a/libpromises/verify_classes.c b/libpromises/verify_classes.c index afb70fff9d..7b6d07c238 100644 --- a/libpromises/verify_classes.c +++ b/libpromises/verify_classes.c @@ -38,6 +38,7 @@ static bool EvalClassExpression(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 +74,19 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED return PROMISE_RESULT_FAIL; } + /* The 'cancel' attribute (a boolean) undefines the promiser class. Its + * trigger is the promise's own class-context guard, not an expression of + * its own. 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 ExpandDeRefPromise() in promises.c). + * VerifyClassCancel does its own attribute validation, so route to it + * before the timer_policy check below. */ + if (a.context.expression != NULL && + strcmp(a.context.expression->lval, "cancel") == 0) + { + return VerifyClassCancel(ctx, pp, &a); + } + /* timer_policy only governs the persistence timer, so it is meaningless * without 'persistence'. Error rather than silently ignoring it. */ if (a.context.persistent <= 0 && @@ -441,3 +455,98 @@ static bool EvalClassExpression(EvalContext *ctx, Constraint *cp, const Promise return false; } + +/* + Handle a classes promise that uses the 'cancel' attribute. 'cancel' is a + boolean: when true, the promiser class is undefined. Its trigger is the + promise's own class-context guard, not an expression of its own (write + "trigger:: \"c\" cancel => \"true\";" rather than a cancel expression). + 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 ExpandDeRefPromise() (see promises.c). + When the promiser class is currently defined, it 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); + + /* assert() is compiled out under NDEBUG, so guard the pointer + * dereferences below with an explicit runtime check as well. */ + if (ctx == NULL || pp == NULL || a == NULL) + { + Log(LOG_LEVEL_ERR, + "VerifyClassCancel internal diagnostic discovered an ill-formed condition"); + return PROMISE_RESULT_FAIL; + } + + /* 'cancel' only undefines a class; the attributes that tune how a class is + * defined or persisted are meaningless alongside it. Reject them rather + * than silently ignoring them. (The class-defining attributes are already + * mutually exclusive with 'cancel' via the "Irreconcilable constraints" + * count.) */ + if (PromiseGetConstraint(pp, "persistence") != NULL || + PromiseGetConstraint(pp, "scope") != NULL || + PromiseGetConstraint(pp, "timer_policy") != NULL) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "The 'cancel' attribute cannot be combined with 'persistence', " + "'scope', or 'timer_policy' (class '%s')", + pp->promiser); + return PROMISE_RESULT_FAIL; + } + + /* 'cancel' is a boolean; 'cancel => "false"' is a no-op. */ + if (!PromiseGetConstraintAsBoolean(ctx, "cancel", pp)) + { + return PROMISE_RESULT_NOOP; + } + + /* 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; + } + + /* Undefine the promiser class if it is currently 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; +} 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..92005f61ef --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute.cf @@ -0,0 +1,73 @@ +####################################################### +# +# Test the 'cancel' attribute of classes promises (CFE-4686). +# +# 'cancel' is a boolean (cfbool). A classes promise using 'cancel => "true"' +# undefines its promiser class. The triggering condition is the promise's own +# class-context guard, not an expression of its own. Unlike the class-defining +# attributes, a cancel promise 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 (cfbool) undefines a defined class when the promise guard is met"; + + classes: + # Define global classes so the check bundle can observe the outcome. + "to_cancel" expression => "any", scope => "namespace"; + "to_cancel_alias" expression => "any", scope => "namespace"; + "to_keep_guard" expression => "any", scope => "namespace"; + "to_keep_false" expression => "any", scope => "namespace"; + "trigger" expression => "any", scope => "namespace"; + + trigger:: + # Guard satisfied and cancel => "true": to_cancel must be undefined. + "to_cancel" cancel => "true"; + + # "yes" is an accepted boolean alias for "true". + "to_cancel_alias" cancel => "yes"; + + class_that_is_not_defined:: + # Guard not satisfied: to_keep_guard must be retained. + "to_keep_guard" cancel => "true"; + + any:: + # Guard satisfied but cancel => "false": no-op, to_keep_false retained. + "to_keep_false" cancel => "false"; + + # Cancelling a class that was never defined is a harmless no-op. + "never_defined" cancel => "true"; +} + +####################################################### + +bundle agent check +{ + classes: + "ok" expression => "!to_cancel.!to_cancel_alias.to_keep_guard.to_keep_false.!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..d784d02961 --- /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 => "true"; +} + +####################################################### + +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_modifier_mutex.cf b/tests/acceptance/02_classes/01_basic/cancel_attribute_modifier_mutex.cf new file mode 100644 index 0000000000..c9bd3847eb --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_modifier_mutex.cf @@ -0,0 +1,43 @@ +####################################################### +# +# Test that 'cancel' rejects the class-modifier attributes (CFE-4686). +# +# 'cancel' only undefines a class, so the attributes that tune how a class is +# defined or persisted - 'persistence', 'scope', 'timer_policy' - make no sense +# alongside it and must be rejected (rather than silently ignored). The +# sub-policy pairs 'cancel' with each; 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 reject 'persistence', 'scope', and 'timer_policy'"; +} + +####################################################### + +bundle agent check +{ + methods: + "" usebundle => dcs_passif_output1(".*'cancel' attribute cannot be combined with.*", + "$(sys.cf_agent) -Kf $(this.promise_filename).sub 2>&1", + $(this.promise_filename)); +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_modifier_mutex.cf.sub b/tests/acceptance/02_classes/01_basic/cancel_attribute_modifier_mutex.cf.sub new file mode 100644 index 0000000000..b4e28729b6 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_modifier_mutex.cf.sub @@ -0,0 +1,15 @@ +body common control +{ + bundlesequence => { "test" }; +} + +bundle agent test +{ + classes: + # 'cancel' paired with a class-MODIFIER attribute must be rejected: + # these only tune class definition/persistence and are meaningless + # when undefining a class. + "c_persist" cancel => "true", persistence => "10"; + "c_scope" cancel => "true", scope => "namespace"; + "c_timer" cancel => "true", timer_policy => "reset"; +} 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..6b5d77c706 --- /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 => "true", expression => "any"; + "c_and" cancel => "true", and => { "any" }; + "c_or" cancel => "true", or => { "any" }; + "c_xor" cancel => "true", xor => { "any" }; + "c_not" cancel => "true", not => "any"; + "c_dist" cancel => "true", dist => { "1", "1" }; + "c_select" cancel => "true", select_class => { "x", "y" }; +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf new file mode 100644 index 0000000000..2f6030ef8f --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf @@ -0,0 +1,83 @@ +####################################################### +# +# Test that 'cancel' clears a persistent class from the cf_state LMDB +# (CFE-4686) -- verified at the source with `cf-check dump`. +# +# A persistent class lives in the cf_state LMDB and is reloaded into the +# context on every agent run. Undefining it from policy must therefore remove +# the LMDB record too, otherwise the class would simply reappear on the next +# run. VerifyClassCancel() does this via EvalContextHeapPersistentRemove(). +# +# run 1 (sub) -> define the class persistently -> LMDB record present +# run 2 (sub2) -> 'cancel => "true"' the (reloaded) class -> LMDB record gone +# +# Each snapshot reads the state DB directly rather than parsing agent log +# text (cf. persistent_timer_policy_default_lmdb.cf). +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4686" } + string => "classes promise 'cancel' removes the persistent class record from the cf_state LMDB"; + + vars: + "state" string => "$(sys.workdir)/state/cf_state.lmdb"; + "sub" string => "$(this.promise_filename).sub"; + "sub2" string => "$(this.promise_filename).sub2"; + + commands: + # One shell invocation runs the whole sequence: + # run 1 -> define persistent class, snapshot LMDB record count (expect 1) + # run 2 -> cancel the class, snapshot LMDB record count (expect 0) + !done:: + "$(sys.cf_agent) -Kf $(sub) > /dev/null 2>&1 ; + $(sys.cf_check) dump -n $(state) | grep -c cfe4686_cancel_lmdb_class | tr -dc '0-9' > $(G.testdir)/cancel_lmdb_before ; + $(sys.cf_agent) -Kf $(sub2) > /dev/null 2>&1 ; + $(sys.cf_check) dump -n $(state) | grep -c cfe4686_cancel_lmdb_class | tr -dc '0-9' > $(G.testdir)/cancel_lmdb_after" + contain => in_shell, + classes => always("done"); +} + +bundle agent check +{ + vars: + "before" string => readfile("$(G.testdir)/cancel_lmdb_before", 4096); + "after" string => readfile("$(G.testdir)/cancel_lmdb_after", 4096); + + classes: + # The class was stored in the LMDB by run 1 ... + "present_before" expression => regcmp("[1-9][0-9]*", "$(before)"); + # ... and removed by the cancel in run 2. + "absent_after" expression => strcmp("$(after)", "0"); + + "ok" expression => "present_before.absent_after"; + + reports: + DEBUG:: + "LMDB record count before cancel = '$(before)' (expect >= 1), after = '$(after)' (expect 0)"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub new file mode 100644 index 0000000000..39808919f0 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub @@ -0,0 +1,11 @@ +body common control +{ + bundlesequence => { "main" }; +} + +bundle agent main +{ + classes: + # Define a persistent class; it is written to the cf_state LMDB. + "cfe4686_cancel_lmdb_class" expression => "any", persistence => "10"; +} diff --git a/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub2 b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub2 new file mode 100644 index 0000000000..dc22343ef9 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/cancel_attribute_persistent_lmdb.cf.sub2 @@ -0,0 +1,12 @@ +body common control +{ + bundlesequence => { "main" }; +} + +bundle agent main +{ + classes: + # The persistent class is reloaded from the LMDB at startup; cancel must + # undefine it AND remove its LMDB record. + "cfe4686_cancel_lmdb_class" cancel => "true"; +}