[PUP-9294] Raise Error when a legacy function illegally defines methods Created: 2018/10/31  Updated: 2019/01/31  Resolved: 2019/01/25

Status: Closed
Project: Puppet
Component/s: Docs, Functions, Language
Affects Version/s: PUP 6.0.0
Fix Version/s: PUP 6.0.7, PUP 6.2.0

Type: Bug Priority: Normal
Reporter: Henrik Lindberg Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: resolved-issue-added
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
is blocked by PUP-9359 PUP-9294 fails to load valid 3.x func... Closed
relates to PUP-9469 Remove the setting to turn off func 3... Accepted
relates to PUP-9268 5.5.7 breaks custom function Closed
Template: PUP Bug Template
Sub-team: Language
Team: Froyo
Method Found: Needs Assessment
Release Notes: New Feature
Release Notes Summary: Function loading of legacy functions will now protect against illegal method definitions inside of loaded legacy functions. Such methods could lead to mysterious and hard to find problems as they could disrupt the operation of the entire system, and cause leakage between environments. A new setting `--func3x_check` can be set to `false` to turn of the checking and thus allowing the illegal construct to be loaded (with bad side effects) and the function to be used on a best effort basis. See https://puppet.com/docs/puppet/latest/functions_refactor_legacy.html for more information.
QA Risk Assessment: Needs Assessment


When a 3.x function .rb file contains calls to `def` they will end up in a place where they are not supposed to be. It is illegal to have such definitions (they used to work in 3.x because they ended up being methods on Object (i.e. it polluted every class in Ruby which is obviously horrible and unwanted).

From 6.0.0, legacy functions are loaded by a new loader and it should catch these illegal definitions and raise an error instead of causing other hard to understand errors. (Currently the methods are defined, but in a place where the legacy function later cannot find and call them).

In the rare case that 3.x functions with illegal constructs are used in such a way that the illegal constructs are only loaded but never called users that want to prevent errors from being raised by the new check can set --func3x_check to false.

Comment by Henrik Lindberg [ 2018/11/14 ]

This turned out to be incredibly difficult in practice. There are 4 different things to protect against - 2 per way the functions are loaded

  • A 3x function loaded by the autoloader - which happens in lots of tests and when explicitly calling scope.function_xxx
  • A 3x function loaded by the 4x loader - which happens when language loads/evaluates calls

The two things to protect against are:

  • a def method inside of the 3x function body
  • a def method outside of the 3x function body

That means there are 4 different classes/modules that will have a method added. By defining callbacks that catch when a method is added in those four places it was possible to raise errors. However - since adding instance methods is also something that takes place when testing / mocking, the same guards are triggered by legitimate testing.

Two of the locations are hit when loading the function, so there it is possible to have a flag that indicates that loading is taking place, but for the methods defined inside of the code body, that does not take place until the function is called! That means it is impossible to catch those at load time unless the ruby code being loaded is parsed and walked and that has the cost of parsing the ruby code twice (once with Ripper, and once for real).

Att runtime, trying to protect the classes/methods is very kludgy as the triggering of the "method added" callbacks reacts to adding those very callbacks.

An alternative is to write a custom Rubocop rule to catch the illegal 3x functions. Armed with that, users can test their modules and the forge can use that to downflag modules with illegal functions. This solution does not burden the runtime - the illegal functions would simply not work but there will not be any special error messages.

At this time, I am exploring the option of using the Ripper (internal Ruby parser) to find illegal defines as the Ripper works in MRI since 1.9 as well as in jRuby.

Comment by Henrik Lindberg [ 2018/11/14 ]

Using the Ripper class works fine - will update the PR to use that to walk the parsed code instead of trying to protect the classes from getting added methods.

Comment by Henrik Lindberg [ 2018/11/14 ]

Added a link to predocs - which also describes how to refactor a 3x function to 4x. https://docs.google.com/document/d/1_KToXDDjbe0HQ6I4UqckNqcJ-AHreH_dmd_A1Cr85jU/edit#heading=h.7tr91p8l7qjp

Comment by Josh Cooper [ 2018/12/11 ]

We're seeing failures in PEZ. Submitted a PR to revert this. /cc Jean Bond

Comment by Josh Cooper [ 2018/12/13 ]

Assigned to Server team

Comment by Jean Bond [ 2019/01/10 ]

Kenn Hussey, Henrik Lindberg, did this make it in for 6.0.5, or should it be bumped to a future version?

Comment by Kenn Hussey [ 2019/01/10 ]

Jean Bond from what I can tell, the PR got merged to the 6.0.x branch so should make it into 6.0.5, but perhaps Branan Riley can confirm as part of ticket reconciliation.

Comment by Jean Bond [ 2019/01/10 ]

Thanks Kenn Hussey. Henrik Lindberg, maybe I missed it, but where does the user set this new option `--func3x_check`?

Comment by Henrik Lindberg [ 2019/01/10 ]

Jean Bond func3x_check is a (binary) setting - so handled just like any other setting (https://puppet.com/docs/puppet/latest/config_about_settings.html). It is true by default, and users can turn it off by setting it to false. (Documentation for the setting itself is auto generated from the puppet source - i.e. it will appear here: https://puppet.com/docs/puppet/latest/configuration.html).

Comment by Branan Riley [ 2019/01/11 ]

This missed the boat for 6.0.5 - it wasn't in the CI run that kicked off EOD Wednesday, which is what we base our "stop ship" line off of. I've updated the fixversion.

Comment by Casey Williams [ 2019/01/20 ]

Tests for this have passed in master; resolving in advance of the 6.2.0 release.

Generated at Fri Aug 07 09:29:46 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.