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

unit_file: allow arbitrary symlink targets #511

Closed
wants to merge 1 commit into from

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented Jan 15, 2025

Absolutepath is exactly what it says: it forces the link target to be an absolute path, that is to start with a slash (/). But I actually want to have relative symlinks.

This is particularly useful when doing a @.service template where the instance refers to the template, often in the same directory.

This, for example, should be allowed:

systemd::unit_file { "pgbackrest-backup-${kind}@${shortname}.service":
  enable => false,
  active => false,
  target => "pgbackrest-backup-${kind}@.service",
}

Yet it currently crashes with:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Systemd::Unit_file[[email protected]]: parameter 'target' expects a Stdlib::Absolutepath = Variant[Stdlib::Windowspath = Pattern[/\A(([a-zA-Z]:[\\\/])|([\\\/][\\\/][^\\\/]+[\\\/][^\\\/]+)|([\\\/][\\\/]\?[\\\/][^\\\/]+)).*\z/], Stdlib::Unixpath = Pattern[/\A\/([^\n\/\0]+\/*)*\z/]] value, got String (file: /etc/puppet/code/environments/prom_module_upgrade/3rdparty/modules/pgbackrest/manifests/repository/stanza.pp, line: 77) on node backup-storage-01.torproject.org

I don't know of another way to fix this than to make this a proper string.

Closes: #510

Absolutepath is exactly what it says: it forces the link target to be
an absolute path, that is to start with a slash (`/`). But I
actually *want* to have relative symlinks.

This is particularly useful when doing a @.service template where the
instance refers to the template, often in the same directory.

This, for example, should be allowed:

    systemd::unit_file { "pgbackrest-backup-${kind}@${shortname}.service":
      enable => false,
      active => false,
      target => "pgbackrest-backup-${kind}@.service",
    }

Yet it currently crashes with:

    Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Systemd::Unit_file[[email protected]]: parameter 'target' expects a Stdlib::Absolutepath = Variant[Stdlib::Windowspath = Pattern[/\A(([a-zA-Z]:[\\\/])|([\\\/][\\\/][^\\\/]+[\\\/][^\\\/]+)|([\\\/][\\\/]\?[\\\/][^\\\/]+)).*\z/], Stdlib::Unixpath = Pattern[/\A\/([^\n\/\0]+\/*)*\z/]] value, got String (file: /etc/puppet/code/environments/prom_module_upgrade/3rdparty/modules/pgbackrest/manifests/repository/stanza.pp, line: 77) on node backup-storage-01.torproject.org

I don't know of another way to fix this than to make this a proper string.

Closes: voxpupuli#510
@@ -73,7 +73,7 @@
Stdlib::Absolutepath $path = '/etc/systemd/system',
Optional[Variant[String, Sensitive[String], Deferred]] $content = undef,
Optional[String] $source = undef,
Optional[Stdlib::Absolutepath] $target = undef,
Optional[String] $target = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Empty string isn't valid here, right?

Suggested change
Optional[String] $target = undef,
Optional[String[1]] $target = undef,

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be reduced to Enum[Stdlib::Absolutepath,Systemd::Unit,Systemd::Unit] or really need things like ../sshd.service for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be reduced to Enum[Stdlib::Absolutepath,Systemd::Unit,Systemd::Unit] or really need things like ../sshd.service for instance.

that would work for me, i don't think there's a case for ../

@traylenator
Copy link
Contributor

systemd::unit_file { "pgbackrest-backup-${kind}@${shortname}.service":
enable => false,
active => false,
target => "pgbackrest-backup-${kind}@.service",
}

Presumably

target => '/etc/systemd/system/pgbackrest-backup-${kind}@.service'

would be okay ? Any reason it needs to be relative ?

Second question, The instance of a unit pgbackrest-backup-${kind}@${shortname}.service can be started , stopped , enabled, disabled without the symlink being created . Why even create the symlink in the first place?

@anarcat
Copy link
Contributor Author

anarcat commented Jan 16, 2025

Presumably

target => '/etc/systemd/system/pgbackrest-backup-${kind}@.service'

would be okay ? Any reason it needs to be relative ?

this is what i said in #510: the absolute path works, of course, but it's a lot of typing and obscures the fact that the target is in the same directory, for someone trying to debug things. it might also make some security policies harder to implement (although I admit we don't have those).

Second question, The instance of a unit pgbackrest-backup-${kind}@${shortname}.service can be started , stopped , enabled, disabled without the symlink being created . Why even create the symlink in the first place?

that, i admit ignorance. i guess i made this because i had a hammer and this looked like a nail?

how would you enable the service without the symlink in Puppet?

either way, it's not clear to me at all how to do this with this module, and if i'm doing this wrong, we should document how to do this right.

@traylenator
Copy link
Contributor

Second question, The instance of a unit pgbackrest-backup-${kind}@${shortname}.service can be started , stopped , enabled, disabled without the symlink being created . Why even create the symlink in the first place?

You litrally just do it:

service{`pgbackrest-backup-${kind}@${shortname}.service':
   ensure => true,
   enable => true,
 }

Just ignore the unit file is not there.

@anarcat
Copy link
Contributor Author

anarcat commented Jan 16, 2025

okay, then i'll reroll this as a docs update.

@anarcat anarcat closed this Jan 16, 2025
anarcat added a commit to anarcat/puppet-systemd that referenced this pull request Jan 16, 2025
I've bent over backwards to make this work, while the solution is much
simpler than I thought it was.

Closes: voxpupuli#510
Replaces: voxpupuli#511
@anarcat anarcat deleted the unit_file_relative branch January 16, 2025 20:30
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.

unit_file should accept relative paths
3 participants