Skip to content

(#22744) Static compiler doesn't filter resources#1962

Closed
fiddyspence wants to merge 1 commit intopuppetlabs:masterfrom
fiddyspence:ticket/master/22744
Closed

(#22744) Static compiler doesn't filter resources#1962
fiddyspence wants to merge 1 commit intopuppetlabs:masterfrom
fiddyspence:ticket/master/22744

Conversation

@fiddyspence
Copy link
Contributor

Copied filter method from compiler.rb and spec tests from compiler

Copied filter method from compiler.rb and spec tests
@puppetcla
Copy link

CLA signed by all contributors.

Copy link
Contributor

Choose a reason for hiding this comment

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

The static compiler works by retrieving a catalog from the normal compilerand stripping out all the file resources; this would lead me to believe that the catalog should already be filtered before the static compiler ran at all. Do you know why this isn't the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is a mismatch between the internal behaviour and where it's called elsewhere - i.e. the catalog terminus static_compiler doesn't support filter, even though the objects internally might and this matters because where it's tested externally, it doesn't have the filter method. Doing this any other way at this point looks like a lot of work :(

@zaphod42
Copy link
Contributor

We've taken a look at this, and would really like to move forward with this, but it seems like something else isn't working as expected that is preventing the normal filtering from working.

@adrienthebo
Copy link
Contributor

I was able to reproduce this behavior with the following configuration.

puppet.conf

[master]
catalog_terminus=static_compiler

site.pp

filebucket { 'puppet':
  path => false,
}

file { '/tmp/static-compiler':
  ensure  => file,
  source => 'puppet:///modules/thingy/stuff.txt',
}

# Doesn't get applied
@file { '/tmp/virtual.nope':
  ensure  => file,
  source => 'puppet:///modules/thingy/virtual.txt',
}

# Does get applied
@@file { '/tmp/exported.nope':
  ensure  => file,
  source => 'puppet:///modules/thingy/exported.txt',
}

In normal operation, calling indirected_class.indirection.find invokes #find on the terminus, and if the terminus response to #filter then this is invoked as well. However this doesn't occur with the static compiler because the static compiler directly accesses the compiler terminus and then invokes the #find method which bypasses the standard indirection behavior. Basically, the indirection system isn't really built to invoke terminuses without going through the normal indirection method calls.

I can see a few options for solving this.

First off, it's possible for an indirected class to define the #select_terminus method, which means that the catalog class could explicitly allow requests to select the terminus to use for the class. This would be a one-off implementation, but would allow us to use the same logic for invoking specific terminuses as well as invoking the default terminus.

Another approach could be implementing a general method for an indirection request to pick a specific terminus to handle the request. It's basically the previous solution but generalized, so it would be less hacky but more work.

In addition we could merge this pull request as-is. Taking this approach would mean that the static compiler is going to constantly run into edge cases where directly invoking a terminus yields a different result than invoking the terminus through the indirector. This will fix the immediate problem but won't help us out farther down the road.

@adrienthebo
Copy link
Contributor

@fiddyspence it's been a while since we've been able to get back to this; what would you like to do with this PR from here on out?

@adrienthebo
Copy link
Contributor

Although now that I think about it, allowing a request to specify an explicit terminus could be a great way to add some fun vulnerabilities to Puppet, which complicates things. :(

@adrienthebo
Copy link
Contributor

There's an additional solution that I think will work much better - we can have the static compiler inherit from the regular compiler and sidestep all the issues that comes from directly invoking another terminus. Taking this approach will be a much more standard approach to the problem, and I have an implementation of this that seems to work well.

I'm going to leave this issue open until I actually get a pull request submitted to replace this PR, but I don't think that we're going to take this approach in the long run. In any case thanks for raising this PR and getting the conversation started!

@adrienthebo
Copy link
Contributor

Closing this in favor of GH-2172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants