Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #12390: Doubled "\" in GM call #1896

Merged

Conversation

fanf
Copy link
Member

@fanf fanf commented Apr 10, 2018

https://www.rudder-project.org/redmine/issues/12390

This is an API breaking change for Windows DSC plugin.

So, the main problem was that we escaped value set in Directive only for a CFEngine Agent.
The pull request change two majors things.

First, it changes where the escape logic is in the code:

  • now, each agent has its own "escape" method bundled in its "agent specific generation" (where the agent-specific policy writing was stored).
  • this change make the fact that we needed an "agent register" apparent. So that AgentRegister is now a standalone class, with utility method to find what agent matches an AgentType/osDetails and does things with it.

Secondly, it changes the types of values in string template. Until now, we were allowing values of type Any. But we don't actually use anything but string, and I can't apply string escape logic on Any. So now, we only deal with strings (which will certainly allow to simplify things in the long term - because yes, String is better than Any, especially when we don't use other things).

@@ -59,13 +59,13 @@ bundle server access_rules

any::
"/var/rudder/share/root/"
maproot => { host2ip("server.rudder.local"), string_downcase(escape("server.rudder.local")) },
admit => { host2ip("server.rudder.local"), string_downcase(escape("server.rudder.local")) },
maproot => { string_downcase(escape("server.rudder.local")) },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change should not be linked with the modification at hand, but I'm not sure why I need it. I'm wondering if we didn't change something elsewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange, if not something in your PR then tests must failed but they passs ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, and I don't know what to think about it. @ncharles, do you have an idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found why, it's because we changed booleans in string template to string, which change conditionals behavior in stringtemplate

@fanf
Copy link
Member Author

fanf commented Apr 10, 2018

PR rebased

@fanf fanf force-pushed the bug_12390/doubled_in_gm_call branch from fd3ac28 to ff40e1c Compare April 10, 2018 22:59
@@ -104,7 +99,7 @@ object VTypeConstraint {
sealed trait StringVType extends VTypeConstraint with VTypeWithRegex {
def regex: Option[RegexConstraint]

override def getTypedValue(value:String, forField:String) : Box[Any] = regex match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Any was ... interesting ...

@@ -192,12 +187,12 @@ final case object LinuxDerivedPasswordVType extends DerivedPasswordVType { overr

case object BooleanVType extends VTypeConstraint {
override val name = "boolean"
override def getTypedValue(value:String, forField:String) : Box[Any] = {
try {
override def getFormatedValidated(value:String, forField:String, escapeString: String => String) : Box[String] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the cause of the issue in templates, the value was previous a boolean, not a string, then when used in conditional in string template, if considers truthy anything that has a value or check the boolean value if it's a boolean:

IF actions test the presence or absence of an attribute unless the object is a Boolean/bool, in which case it tests the attribute for true/false. The only operator allowed is "not" and means either "not present" or "not true". For example, "$if(!member)$...$endif$".

Copy link
Member

@VinceMacBuche VinceMacBuche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to still be able to use booleans in string template so we can still have ifs in string templates

@fanf
Copy link
Member Author

fanf commented Apr 11, 2018

PR rebased

@fanf fanf force-pushed the bug_12390/doubled_in_gm_call branch from ff40e1c to 6097ea2 Compare April 11, 2018 22:59
Copy link
Member

@VinceMacBuche VinceMacBuche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/1896
-- Your faithful QA

@fanf
Copy link
Member Author

fanf commented Apr 11, 2018

OK, merging this PR

@fanf fanf merged commit 6097ea2 into Normation:branches/rudder/4.2 Apr 11, 2018
@@ -221,53 +221,22 @@ final case class Cf3PolicyDraft(
*/
case class ParameterEntry(
parameterName : String
, parameterValue: String
, escapedValue : String
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i'm correct, this is used for the global parameters - here we are droping the escaping of these parametres, causing https://www.rudder-project.org/redmine/issues/12674

@fanf fanf deleted the bug_12390/doubled_in_gm_call branch March 15, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants