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

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Bug #2053

Relative namespacing of class/define names results in big surprises

Added by Lawrence Ludwig about 7 years ago. Updated over 2 years ago.

Status:AcceptedStart date:03/06/2009
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:modules
Target version:-
Affected Puppet version:0.24.7 Branch:
Keywords:telly deprecation modules classes namespaces names

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-1205


Description

Due to Puppet’s relative namespacing of class names, include bar does not mean “declare class bar.” It actually means “try to declare class <current namespace>::bar, class <parent of current namespace>::bar, and so on, declaring class bar only as a last resort.”

class bar {
  notice("From class bar")
}
class foo::bar {
  notice("From class foo::bar")
}
class foo {
  include bar
}
include foo

This is maximally surprising, especially considering the prevalence of things like:

class apache::nagios {
  include nagios # joke's on you, because this just includes apache::nagios again
}

(Test code from original report: http://pastie.org/409446)

409446.html Magnifier - saved pastie page (14.9 KB) Lawrence Ludwig, 03/06/2009 04:48 pm

Picture_1.png - viewable image (79.6 KB) Lawrence Ludwig, 03/06/2009 04:52 pm


Related issues

Related to Puppet - Feature #5208: Emit notice when class is not included because it is shad... Accepted 11/05/2010
Related to Puppet - Bug #1372: Class names that match the node name are not evaluated Investigating 06/16/2008
Related to Puppet - Bug #13349: Incorrect scope behavior: single include may load multipl... Closed 03/23/2012
Duplicated by Puppet - Bug #2523: Classes with the same name are not included Duplicate 08/10/2009
Duplicated by Puppet - Bug #10848: When is "class { foo: }" not equivalent to "include foo"? Duplicate 11/15/2011
Duplicated by Puppet - Bug #21428: puppet class namespace length limit causes incorrect load... Ready for Documentation

History

#1 Updated by Lawrence Ludwig about 7 years ago

#2 Updated by Luke Kanies about 7 years ago

  • Status changed from Unreviewed to Needs Decision

I can’t decide on this one. There’s an easy workaround:

class foo::xen {
  include ::xen
}

But I suppose there should at least be a warning.

Am I misreading expected behaviour that much? It seems reasonable that any mention of ‘xen’ inside of a xen class would normally refer to itself. Keep in mind that we’ve got a general class lookup mechanism – ‘include’ doesn’t have its own class lookup.

#3 Updated by Lawrence Ludwig about 7 years ago

Well I guess Puppet’s :: naming throws me for a loop. I’m more used to the $this-> convention to explicitly do this. Yea I had no idea what was going on. I assumed ‘include’ started from the root, not the class it’s currently in.

So when you start with :: it refers to the root? So:

::classname::subclass

$::classname::var

Are valid? If so, that’s good to know and maybe this is more of a documentation issue.

#4 Updated by Luke Kanies about 7 years ago

  • Category set to documentation
  • Status changed from Needs Decision to Accepted

lludwig wrote:

Well I guess Puppet’s :: naming throws me for a loop. I’m more used to the $this-> convention to explicitly do this. Yea I had no idea what was going on. I assumed ‘include’ started from the root, not the class it’s currently in.

So when you start with :: it refers to the root? So:

::classname::subclass

$::classname::var

Are valid? If so, that’s good to know and maybe this is more of a documentation issue.

Yep, those are all valid.

And this class search seems most intuitive. E.g., with this class structure:

class bar {}

class foo {
  class bar {}
  class baz {
    include bar
  }
}

I’d expect foo::bar to get included, not ::bar.

#5 Updated by James Turnbull almost 7 years ago

  • Assignee set to Puppet Community
  • Target version set to 4

#6 Updated by James Turnbull almost 7 years ago

  • Assignee deleted (Puppet Community)

#7 Updated by James Turnbull over 6 years ago

  • Assignee set to James Turnbull

#8 Updated by James Turnbull about 5 years ago

  • Assignee deleted (James Turnbull)

#9 Updated by James Turnbull over 4 years ago

  • Target version deleted (4)

#10 Updated by Nick Fagerlund over 3 years ago

  • Category changed from documentation to modules
  • Status changed from Accepted to Needs Decision
  • Assignee set to eric sorenson
  • Keywords set to telly deprecation modules classes namespaces names

Andy and Eric and I talked about this today, and we were pretty much in agreement:

  • The introduction of the autoloader imposed a new and complete meaning on namespaces: they reflect a class or define’s position in a module.
    • This conflicts with the older meaning of namespaces, which was that namespaced classes or defines were actually CONTAINED in another class.
  • In the modules era, the fact that a class or define shares a namespace with another class implies no particular relationship other than the fact that they share a module.
  • The depth of a class or define’s namespace chain carries no meaning.
  • Relative class names (i.e. include bar does not actually mean to include class bar, except as an absolute last resort) obey the principle of maximum surprise (and user rage).
  • Asking everyone to use absolute class names (include bar will never declare class foo::bar) isn’t an unreasonable burden…
    • …Especially since that’s what everyone THINKS you have to do already.

And therefore we should probably kill relative class/define names. But it’s going to require a big deprecation period, because it will absolutely break large amounts of older code out in the wild.

#11 Updated by Nick Fagerlund over 3 years ago

  • Subject changed from including a class with the same name subclass yields an unexpected result to Relative namespacing of class/define names results in big surprises
  • Description updated (diff)

(Let’s clean up the subject and description a bit — we’re talking about namespaces, not subclasses [aka “inherits”].)

#12 Updated by Anonymous over 3 years ago

I think that killing relative names would be great in terms of “principle of least surprise”, but I’m uncertain what the deprecation and replacement would look like. Simply warning that something would result in a relative lookup seems unfriendly because they would have to use the ugly :: prefix which during the dynamic scope deprecation period caused a lot of unhappiness.

So what would the deprecation and new world look like?

#13 Updated by Nick Fagerlund over 3 years ago

I think deprecation would look like:

  • Implement a slightly more aggressive version of #5208 — this is a feature request for smart warnings, so you’d ONLY get warned about “include nagios” if there were a relatively-named <something>::nagios class within the current chain of namespaces. (By “slightly more aggressive,” I mean we should warn whenever relative namespacing finds something, not just when the relatively named class blocks an absolutely named class.)
  • Provide a temporary pref (off by default) that turns on “strict mode” for absolute names (include nagios always means include nagios), so anyone who has investigated their warnings can start living in the future immediately.

I think this would:

  • Prevent anyone from having to use the ::nagios idiom, and free up anyone who currently has to use it.
  • Alert people who didn’t even KNOW that their namespaces were getting resolved relatively.
  • Give anyone who actually USES relative naming in their modules a clear idea of what they need to do and at which lines in their code to do it.

This is obviously more work than the scope warnings were, but hey, if it’s worth doing…

#14 Updated by Nick Fagerlund over 3 years ago

  • Description updated (diff)

#15 Updated by Jason Slagle over 3 years ago

As someone who just got bit by this, +1 here.

It’s even more bizarre when doing test driven puppet dev and the rspec tests failed in ways you can’t explain.

#16 Updated by Anonymous over 3 years ago

Nick, what you describe sounds pretty reasonable. I like that it also provides a clean way of transitioning from the old to the new where the user gets to choose the pace.

In the end I don’t think this is more work that the scope warnings, but the proof of that will be when we try to do it.

Eric, I think we should target this at the 3.x series.

#17 Updated by Gary Larizza over 3 years ago

We just traced through an issue with our SalesEng modules that hit this. I agree with Nick that the problem we hit was that we didn’t realize the relative namespacing – so something that would alert us to THAT fact is a big win (in my opinion). A +1 to nick’s suggestion from me.

#18 Updated by Corey Hickey over 3 years ago

I ran into some deeper trouble with this bug while parameterizing a class—it appears that include respects the :: prefix, but class {} does not. In my case, I have two classes webconf::php and ::php. I’m going to phase out the former in favor of the latter, but they both need to exist for a transitional period.

This loads ::php, as expected:

class webconf {
    include ::php
}

This tries to load both(?) ::php and webconf::php:

class webconf {
    class { "::php": }
}

…resulting in the error:

Duplicate declaration: Package[php] is already declared in file /usr/local/puppet/modules/webconf/manifests/php.pp at line 5; cannot redeclare at /usr/local/puppet/modules/php/manifests/init.pp:21

I admit it’s possible I’m doing this wrong, but if I rename webconf::php to webconf::oldphp, the class declaration works fine. This is with puppet 2.7.17.

#19 Updated by Anonymous over 3 years ago

  • Status changed from Needs Decision to Accepted
  • Assignee changed from eric sorenson to Anonymous

I’ve marked this as related to #1372 because of the discrepancy between include foo and class { foo: } pointed out by Cory Hickey. The investigation on the other bug turned up that these two forms end up behaving very differently and I think what we are seeing is the result of having duplicate code between the two execution paths.

Lee, I’m assigning this to you because it might intersect with your work on #1372. Feel free to un-assign it if you don’t want to try to tackle this as well.

#20 Updated by Nick Fagerlund over 3 years ago

Andy and Corey, this looks exactly like bug #13349 — the fix for it went into 3.0.0, but I’m pretty sure it didn’t go into 2.7; if it had, it’d be 2.7.20 at the earliest.

#21 Updated by Corey Hickey over 3 years ago

Nick Fagerlund wrote:

Andy and Corey, this looks exactly like bug #13349 — the fix for it went into 3.0.0, but I’m pretty sure it didn’t go into 2.7; if it had, it’d be 2.7.20 at the earliest.

Thanks guys, I agree—that definitely looks the same as what I reported. Nice to know it’s fixed. We’ll probably be looking at 3.0 early next year.

#22 Updated by Abhay Chrungoo about 3 years ago

in modules/foo/bar/ted.pp

define foo::bar::ted(){ code} define private(){more code}


I have tested that private is not available as private OR foo::private OR foo::bar::private OR foo::bar::ted::private to any code that’s not part of the module foo. (3.0.2)

This is a good thing. Why would you consider removing it? particularly since the DSL does not even have any looping constructs or iterations.

I dont want manifest writers using my module internals to hack out a quick result when I dont explicitly provide for it. The burden of every define/class doing something valuable that can be used independently is simply wayy too high.

least surprise is the least of my worries. Some degree of encapsulation is high on the agenda.

#23 Updated by Anonymous about 3 years ago

Abay C wrote:

in modules/foo/bar/ted.pp

define foo::bar::ted(){ code} define private(){more code}


I have tested that private is not available as private OR foo::private OR foo::bar::private OR foo::bar::ted::private to any code that’s not part of the module foo. (3.0.2)

You can get to private by doing:

foo::bar::ted { "pull in the .pp file": }
private { "now everything in it is available": }

You won’t be able to get to private initially. However, once that .pp file that it is contained in is loaded, which would normally by done via the autoloader, then private has been defined and can be used.

This is a good thing. Why would you consider removing it? particularly since the DSL does not even have any looping constructs or iterations.

I dont want manifest writers using my module internals to hack out a quick result when I dont explicitly provide for it. The burden of every define/class doing something valuable that can be used independently is simply wayy too high.

Being able to mark things private to a module would probably be a good feature. As you say, having access control to better create encapsulation greatly improves the maintainability of modules, but this doesn’t provide that. What this is is a shortcut for typing names, which seems to cause confusion sometimes about what will actually get loaded.


I tested that you can get to private by creating modules/testing/manifests/something.pp that contains

define testing::something() {
  notify { "testing::something": }
}

define private() {
  notify { "private": }
}

which when I run puppet apply -e "testing::something { hmm: } private { oops: }" produces

Notice: private
Notice: /Stage[main]//Private[oops]/Notify[private]/message: defined 'message' as 'private'
Notice: testing::something
Notice: /Stage[main]//Testing::Something[hmm]/Notify[testing::something]/message: defined 'message' as 'testing::something
Notice: Finished catalog run in 0.05 seconds

#24 Updated by John Bollinger about 3 years ago

It seems to me that the issue is not so much relative name resolution, but the fact that Puppet is so aggressive about trying to find a relative match. I think it’s right and proper to support name resolution relative to the current scope — it makes sense and it’s easy to understand. The problem is really with trying to resolve names against parent scopes other than the top scope.

Consider:

class my_module::foo {
  notify { 'I am ::my_module::foo': }
}
class foo {
  notify { 'I am ::foo': }
}
class my_module {
  include 'foo'
}

Is it not reasonable for the author to expect that class my_module is declaring class ::my_module::foo, rather than class ::foo?

Or indeed, Puppet still supports lexical class containment. If I write

class foo {
  notify { 'I am ::foo': }
}
class my_module {
  class foo {
    notify { 'I am ::my_module::foo': }
  }
  include 'foo'
}

then I would be inclined expect that class ::my_module::foo is the one declared.

On the other hand, I agree with the OP that it is strange, surprising, and generally unhelpful for Puppet to attempt to resolve relative names against parent intermediate scopes. The nagios example is particularly perverse, as it is never useful for a class to declare itself.

I suggest, therefore, that instead of removing relative name resolution altogether, its behavior be modified. When Puppet attempts to resolve a relative name, it should consider two scopes (only): the local scope and the top scope. This will save a lot of the code that complete removal of relative name resolution would break, while still making relative name resolution a lot less surprising.

#25 Updated by John Bollinger about 3 years ago

Ultimately, it might be useful to deprecate name resolution falling back to top scope, too. The only reason to keep that special case is that it is extensively used (and that’s a very good reason), but that fallback is still inconsistent with what I would generally expect from relative name resolution.

#26 Updated by Anonymous over 2 years ago

Redmine Issue #2053 has been migrated to JIRA:

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

Also available in: Atom PDF