The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #5046

Mixed invocation of parameterized classes leads to order dependencies, should be disallowed

Added by Paul Berry over 3 years ago. Updated 4 months ago.

Status:Needs DecisionStart date:01/13/2011
Priority:NormalDue date:
Assignee:eric sorenson% Done:

0%

Category:language
Target version:3.x
Affected Puppet version: Branch:
Keywords:parameterized_classes

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This ticket is now tracked at: https://tickets.puppetlabs.com/browse/PUP-1221


Description

When a parameterized class has default values for all of its parameters, it may be invoked using either “include” or “class { … }” notation. The “include” notation is idempotent; the “class { … }” notation isn’t. As a result, there is an order dependency. This works:

class foo($param = defaultvalue) {
  notify { $param: }
}

class { foo: param => value }
include foo

But this produces an error:

class foo($param = defaultvalue) {
  notify { $param: }
}

include foo
class { foo: param => value }

In large manifests, it is not always obvious (or even well-defined) what order statements will be executed in. To avoid user confusion, I think it would be preferable if both these examples produced an error message saying something like “class foo already invoked using ‘class { … }’ syntax, cannot also invoke using ‘include’”.

NF: Note that the “inherits” keyword declares the base class in a manner similar to “include,” which means that if you declare a subclass of class foo (class baz inherits foo {...} ... include baz) and then declare class foo with the resource-like syntax (class {'foo':}), it’ll blow up in the same way as described above.


Subtasks

Bug #5885: Reporting of errors from parameterized classes fails in n...Accepted


Related issues

Related to Puppet - Bug #7487: Multiple declarations of a parameterized class with the s... Accepted 05/11/2011
Duplicated by Puppet - Bug #5074: declaring a parameterized class does not make its enclose... Duplicate 10/21/2010
Duplicated by Puppet - Bug #5447: Fix duplicate definition error Duplicate 12/02/2010
Duplicated by Puppet - Bug #10972: Classes can be declared with "class {..." and "include ..... Duplicate 11/20/2011
Duplicated by Puppet - Bug #8702: "including" 2 inherited classes via class resource syntax... Duplicate 07/29/2011
Duplicated by Puppet - Bug #10848: When is "class { foo: }" not equivalent to "include foo"? Duplicate 11/15/2011

History

#1 Updated by Peter Meier over 3 years ago

So the idea is that in the future we can only do class{‘foo’: } multiple times OR include multiple times, but not mixed?

#2 Updated by Paul Berry over 3 years ago

Actually, doing class{‘foo’: } multiple times is problematic because this notation allows parameters to be passed to the class, and you might pass a different set of parameters each time you invoke the class. So doing class{‘foo’: } multiple times is already prohibited by the language.

So the current behavior is, you can only do:

  • class{‘foo’: } once OR
  • include multiple times OR
  • both, provided that the class{‘foo’: } happens before any of the includes.

My proposal is that we drop support for the third option, on the grounds that its behavior is already problematic because of the order dependency.

#3 Updated by Dan Bode over 3 years ago

I disagree with the proposal to drop number 3.

I don’t care about redeclaraing parametrized classes, but I do want to be able to mix declarations with include without having to worry about order.

Use cases:

classes should be self contained, this requires being able to specify that a class has a dependency on another class. I like the idea of all classes being able to indicate dependencies on other classes with include, but also allowing a single place to override variables.

Another use case:

If include is required for variables from a class’s scope to be accessible, then you have to be able to mix includes with class declarations. #5074

#4 Updated by Jeff McCune over 3 years ago

Dan Bode wrote:

I disagree with the proposal to drop number 3.

I think we should drop number 3 and move entirely to parameterized class declarations where possible. The convention addresses a number of problems and satisfies the use cases below.

I don’t care about redeclaraing parametrized classes, but I do want to be able to mix declarations with include without having to worry about order.

Use cases:

classes should be self contained, this requires being able to specify that a class has a dependency on another class. I like the idea of all classes being able to indicate dependencies on other classes with include, but also allowing a single place to override variables.

I consider this use case to be dangerous since the scope and therefore the behavior is unpredictable. Classes should not be included into the scope of other classes because they have relationships in the graph. We need some other mechanism other than include to express the relationship of classes.

Another use case:

If include is required for variables from a class’s scope to be accessible, then you have to be able to mix includes with class declarations. #5074

This use case should also be considered dangerous. Parameterized classes address this use case. If information stored in variables in class A is needed in class B, the information should be presented to the class as an attribute parameter, not through accessing variables outside of the class scope and requiring A to be nested in B’s scope.

This use case also requires a class only be included once in order to guarantee scope and behavior. If we already require a class is only included once to satisfy the use case, then a single parameterized class declaration also satisfies the use case.

-Jeff

#5 Updated by Jeff McCune over 3 years ago

Thinking about this, does the class declaration syntax support declaring “virtual” classes?

Working with a customer, they specifically asked if the following use case would be possible:

class dbserver {
  @class { "mysql::server":
    socket => "/var/lib/mysql/socket",
  }
}

# site.pp
node default {
  include dbserver
  Class <| title == "mysql::server" |>
}

The expectation of this use case is that multiple classes could “realize” the classes they require, but they’d be guaranteed the class is evaluated at a predictable scoping level, in this use case in the scope of Node[default]/Class[dbserver]/Class[mysql::server] and NOT at the scope of Node[default]/Class[mysql::server] or whatever other classes decided to realize the virtual class?

#6 Updated by Nigel Kersten over 3 years ago

  • Status changed from Accepted to Needs Decision

#7 Updated by Paul Berry over 3 years ago

  • Keywords set to parameterized_classes

#8 Updated by Nigel Kersten over 3 years ago

  • Target version changed from 69 to 2.7.x

To get down a discussion a few of us had in person, I believe there was general consensus of:

  • deprecate include and require functions for class declaration
  • use the new parameterized class declaration syntax only.
    class { foo: }
  • make appropriate metaparameters like before/require work with above syntax
  • probably a new function to replace include that could accept hashes for parameters, with some question over whether the title could be embedded in the hash.

e.g. with “declare” not necessarily being the function name:

declare myclass { require => Class["myclass2"], stage => "bootstrap" }

scales well to the simplest case, but perhaps we should also support:

declare { title => "myclass", require => Class["myclass2"], stage => "bootstrap" }

Opening for comments from people outside the room where the conversation happened.

#9 Updated by Dan Bode over 3 years ago

I agree with this, although its taken me a long time to come to this conclusion.

I have one major concern for functionality that needs to be replaced:

include foo
$var = $foo::var

I need some way of knowing that if a class has been declared, that I need to wait until its declaration is evaluated so that I can access variables from its scope.

note: since Jeff made this assumption above, I dont want the class to be in the scope where include is called, I just want to know that I can access variables from with-in its scope.

This is a massive change in the puppet language that will completely break backwards compatibility and require a rewrite of most people’s code. Is it that much of a burdon to maintain include and ween people off of it slowly?

Also, so that everyone is on the same page, have we considered that this effects all things that rely on dynamic scoping:

  • variables – behavior needs to change
  • resource defaults – the previous behavior was not desirable
  • stages – new feature, so it probably doesn’t matter
  • implicit tags

#10 Updated by Nan Liu over 3 years ago

Dan Bode wrote:

I have one major concern for functionality that needs to be replaced:

include foo
$var = $foo::var

Initially I was going to disagree, but after testing I realize I can’t access any variable declared with class {…}. This is definitely a problem until we have a replacement method to expose data:

class foo{
  $var = "hello"
  notice ("in foo")
}
class { "foo": }
notice ( $foo::var )

warning: Scope(Class[main]): Could not look up qualified variable 'foo::var'; class foo has not been evaluated

I need some way of knowing that if a class has been declared, that I need to wait until its declaration is evaluated so that I can access variables from its scope.

I’m not sure why required function was dropped, but I can live without it (assuming the first issue is fixed). The example above invariably either exist in another class or top scope. I would prefer it rewritten as a parameterized class so the dependency is exposed.

class bar {
  include foo
  $var = $foo::var
}
node baz {
  include bar
}

refacter:

class bar {
  $var = $foo::var
}
node baz {
  class { "bar":
    require => Class["foo"],
  }
}

or even better:

class bar ($var) {
  ...
}
node baz {
  class { "bar":
    var => $foo::var,
    require => Class["foo"],
  }
}

I consider this a positive change in the right direction.

  • variables – behavior needs to change

Dynamic scoping doesn’t propagate, parameterized classes as interfaces, improved decoupling.

  • resource defaults – the previous behavior was not desirable

We don’t need to make any changes to resource defaults if we adapt the changes above. The new class declaration makes them useful again since it isolates the scope like defined resources. No more worrying about includes causing tight coupling and affecting another class unintentionally.

#11 Updated by John Bollinger over 3 years ago

Nigel Kersten wrote:

To get down a discussion a few of us had in person, I believe there was general consensus of:

  • deprecate include and require functions for class declaration
  • use the new parameterized class declaration syntax only. […]

If that is going to be done, then I strongly desire that, at minimum, multiple declarations of the same class with all-default parameters be permitted. Transitioning my manifests will otherwise be a nightmare, and writing future ones will be much more difficult.

Ideally, though, I would prefer that multiple declarations of the same class be permitted with any parameter list, provided that the parameter list is everywhere the same.

Alternatively, I think Jeff’s virtual class declaration and realization would probably achieve the same goal: replacing the ability to “include” a class in multiple places in the same manifest set.

#12 Updated by Nigel Kersten over 3 years ago

John Bollinger wrote:

I strongly desire that, at minimum, multiple declarations of the same class with all-default parameters be permitted. Transitioning my manifests will >otherwise be a nightmare, and writing future ones will be much more difficult.

Ideally, though, I would prefer that multiple declarations of the same class be permitted with any parameter list, provided that the parameter list is everywhere the same.

Yes. I think this is somewhat orthogonal though, but I personally am leaning towards us allowing this completely.

Alternatively, I think Jeff’s virtual class declaration and realization would probably achieve the same goal: replacing the ability to “include” a class in multiple places in the same manifest set.

I also like this, but am leaning towards both.

#13 Updated by James Turnbull about 3 years ago

  • Assignee set to Nigel Kersten

#14 Updated by Nigel Kersten almost 3 years ago

  • Target version changed from 2.7.x to 3.x

#15 Updated by Nigel Kersten almost 2 years ago

  • Assignee changed from Nigel Kersten to eric sorenson

#16 Updated by Nick Fagerlund over 1 year ago

  • Description updated (diff)

Altering description, because #8702 is the same underlying issue and I want to capture that extra nuance here before closing it.

#17 Updated by Jon McKenzie 5 months ago

I’m having a similar seeming issue to this report (duplicate class declarations depending on manifest ordering), however I’m not declaring the same class multiple times nor are parameters involved. Here’s a way to reproduce my issue:

  1. Create a test module called ‘foo’ with the following classes/content:

init.pp:

class foo { }

bar.pp:

class foo::bar {
  require 'foo::baz'
}

baz.pp:

class foo::baz { }

bam.pp:

class foo::bam {
  include 'foo::bar'
  class {'foo::baz': }
}
  1. Run puppet apply -e ‘include foo::bam’. Running Puppet 3.3.1, I get a duplicate declaration error on the “foo::baz” class (even though it’s only declared a single time).

  2. Edit bam.pp and reverse the ordering of the ‘include’ and ‘class’ statements, e.g.:

class foo::bam {
  class {'foo::baz': }
  include 'foo::bar'
}
  1. Re-run the ‘puppet apply’. No errors are produced.

Any idea if this is related to this report?

#18 Updated by Felix Frank 4 months ago

Redmine Issue #5046 has been migrated to JIRA:

https://tickets.puppetlabs.com/browse/PUP-1221

Also available in: Atom PDF