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

Allow exec creates to check for all files instead of at least one #9168

Closed
yakatz opened this issue Nov 22, 2023 · 11 comments
Closed

Allow exec creates to check for all files instead of at least one #9168

yakatz opened this issue Nov 22, 2023 · 11 comments
Labels
bug Something isn't working triaged Jira issue has been created for this

Comments

@yakatz
Copy link
Contributor

yakatz commented Nov 22, 2023

Describe the Bug

I tried to convert an exec resource from multiple invocations of test -f to use the built-in creates option, but it doesn't work as expected, it will not execute unless none of the files are present.

Original:

  exec {'generate snakeoil certificate':
    command => "/usr/bin/sscg -q \
     --cert-file     /etc/pki/tls/certs/localhost.crt   \
     --cert-key-file /etc/pki/tls/private/localhost.key \
     --ca-file       /etc/pki/tls/certs/localhost.crt   \
     --dhparams-file /tmp/dhparams.pem                  \
     --lifetime      3650                               \
     --hostname      ${facts['networking']['fqdn']}     \
     --email         root@${facts['networking']['fqdn']}",
    unless  => [
      '/usr/bin/test -f /etc/pki/tls/certs/localhost.crt',
      '/usr/bin/test -f /etc/pki/tls/private/localhost.key',
    ],
    notify  => Service['httpd'],
  }

Expected equivalent:

  exec {'generate snakeoil certificate':
    command => "/usr/bin/sscg -q \
     --cert-file     /etc/pki/tls/certs/localhost.crt   \
     --cert-key-file /etc/pki/tls/private/localhost.key \
     --ca-file       /etc/pki/tls/certs/localhost.crt   \
     --dhparams-file /tmp/dhparams.pem                  \
     --lifetime      3650                               \
     --hostname      ${facts['networking']['fqdn']}     \
     --email         root@${facts['networking']['fqdn']}",
    creates => [
      '/etc/pki/tls/certs/localhost.crt',
      '/etc/pki/tls/private/localhost.key',
    ],
    notify  => Service['httpd'],
  }

Debug output if both exist:

Info: Applying configuration version '1700663411'
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]/creates: Checking that 'creates' path '/etc/pki/tls/certs/localhost.crt' exists
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]: '/usr/libexec/httpd-ssl-gencerts' won't be executed because of failed check 'creates'
Debug: Finishing transaction 12600

Debug output if the first file is missing and the second one exists:

Info: Applying configuration version '1700663438'
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]/creates: Checking that 'creates' path '/etc/pki/tls/certs/localhost.crt' exists
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]/creates: Checking that 'creates' path '/etc/pki/tls/private/localhost.key' exists
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]: '/usr/libexec/httpd-ssl-gencerts' won't be executed because of failed check 'creates'
Debug: Finishing transaction 12600

Debug output if neither file exists (debug run without notify httpd):

Info: Applying configuration version '1700663608'
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]/creates: Checking that 'creates' path '/etc/pki/tls/certs/localhost.crt' exists
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]/creates: Checking that 'creates' path '/etc/pki/tls/private/localhost.key' exists
Debug: Exec[generate snakeoil certificate](provider=posix): Executing '/usr/libexec/httpd-ssl-gencerts'
Debug: Executing: '/usr/libexec/httpd-ssl-gencerts'
Notice: /Stage[main]/Main/Exec[generate snakeoil certificate]/returns: executed successfully
Debug: /Stage[main]/Main/Exec[generate snakeoil certificate]: The container Class[Main] will propagate my refresh event
Debug: Class[Main]: The container Stage[main] will propagate my refresh event
Debug: Finishing transaction 12600

Expected Behavior

I would expect the command to be executed if any of the files listed in creates are missing.

Environment

  • Version 7.27.0
  • Platform RHEL 9.2
@yakatz yakatz added the bug Something isn't working label Nov 22, 2023
@yakatz
Copy link
Contributor Author

yakatz commented Nov 22, 2023

Discussing on Slack, another option that preserves backwards compatibility would be to take a multi-dimensional array:

[
  [all of these files, 2, 3, 4],
  [or all of these files, 5, 6, 7],
  [or even all of these],
]

@yakatz yakatz changed the title Change behavior of exec creates to check for all files instead of at least one Allow exec creates to check for all files instead of at least one Nov 22, 2023
@cthorn42
Copy link
Collaborator

@yakatz I think the best solution here is to take your #9167 PR and get that merged in there.

@cthorn42 cthorn42 added the triaged Jira issue has been created for this label Nov 28, 2023
Copy link

Migrated issue to PUP-11989

@joshcooper
Copy link
Contributor

@yakatz I'm confused. The unless docs say

This `exec` would only run if every command in the array has a
non-zero exit code.

In your example, that means test -f <file> commands must return non-zero for the exec to run, which is the same as none of the files existing. And that's equivalent to how creates behaves:

will only run the command if both files don't exist.

In this example, note how both execs don't run if I just delete one of their related files:

❯ cat <<END > creates.pp 
exec { 'creates':
  command => '/usr/bin/touch /tmp/creates1 && /usr/bin/touch /tmp/creates2',
  creates => [
    '/tmp/creates1',
    '/tmp/creates2'
  ]
}

exec { 'unless':
  command => '/usr/bin/touch /tmp/unless1 && /usr/bin/touch /tmp/unless2',
  unless => [
    '/usr/bin/test -f /tmp/unless1',
    '/usr/bin/test -f /tmp/unless2'
  ]
}
END
❯ rm /tmp/creates* /tmp/unless*
❯ bundle exec puppet apply creates.pp
Notice: Compiled catalog for localhost in environment production in 0.05 seconds
Notice: /Stage[main]/Main/Exec[creates]/returns: executed successfully
Notice: /Stage[main]/Main/Exec[unless]/returns: executed successfully
Notice: Applied catalog in 0.03 seconds
❯ bundle exec puppet apply creates.pp
Notice: Compiled catalog for localhost in environment production in 0.05 seconds
Notice: Applied catalog in 0.02 seconds
❯ rm /tmp/creates1 /tmp/unless1
❯ bundle exec puppet apply creates.pp
Notice: Compiled catalog for localhost in environment production in 0.05 seconds
Notice: Applied catalog in 0.02 seconds
❯ ls /tmp/creates* /tmp/unless* 
/tmp/creates2  /tmp/unless2

It's only when I delete both files that the execs run:

❯ rm /tmp/creates* /tmp/unless*
❯ bundle exec puppet apply creates.pp
Notice: Compiled catalog for localhost in environment production in 0.05 seconds
Notice: /Stage[main]/Main/Exec[creates]/returns: executed successfully
Notice: /Stage[main]/Main/Exec[unless]/returns: executed successfully
Notice: Applied catalog in 0.04 seconds
❯ ls /tmp/creates* /tmp/unless*
/tmp/creates1  /tmp/creates2  /tmp/unless1  /tmp/unless2

@yakatz
Copy link
Contributor Author

yakatz commented Jul 25, 2024

It might have been a copy/paste error - I don't have it in front of me now to check, but it is working as described in production: we have a condition that will have the exec run if either file is missing and it would be nice to natively support that.

@joshcooper
Copy link
Contributor

we have a condition that will have the exec run if either file is missing and it would be nice to natively support that.

@yakatz could you provide steps to reproduce.

@yakatz
Copy link
Contributor Author

yakatz commented Sep 24, 2024

@yakatz could you provide steps to reproduce.

You're right, it probably never worked that way. At some point we changed it to a single unless test for both files.

unless  => '/usr/bin/test -f /etc/pki/tls/certs/localhost.crt && /usr/bin/test -f /etc/pki/tls/private/localhost.key'

@joshcooper
Copy link
Contributor

@yakatz ok thanks for letting us know. So IIUC we can close this? Please reopen if I'm confused.

@yakatz
Copy link
Contributor Author

yakatz commented Sep 24, 2024

It would still be a useful feature to have, so consider it a feature request.

@joshcooper
Copy link
Contributor

Given there's already a way to check that all files must exist using:

  unless => [
    '/usr/bin/test -f /tmp/unless1',
    '/usr/bin/test -f /tmp/unless2'
  ]

I'd be reluctant to add more complexity to the creates parameter. Trying to differentiate between creates => [ '/tmp/...'] behaves like OR while creates => [ [ '/tmp/...'] ] behaves like AND is magical and IMO should be avoided.

@yakatz
Copy link
Contributor Author

yakatz commented Sep 25, 2024

Given there's already a way to check that all files must exist using:

  unless => [
    '/usr/bin/test -f /tmp/unless1',
    '/usr/bin/test -f /tmp/unless2'
  ]

You can't do that, as I mentioned, my original post was wrong and we use a ridiculous shell boolean test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Jira issue has been created for this
Projects
None yet
Development

No branches or pull requests

3 participants