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

Move Teleport binaries from /usr/local/bin to /usr/bin #43441

Closed
wants to merge 2 commits into from

Conversation

fheinecke
Copy link
Contributor

@fheinecke fheinecke commented Jun 24, 2024

We currently install Teleport (and related binaries) under /usr/local/bin. Per FHS, "The /usr/local hierarchy is for use by the system administrator when installing software locally". On the other hand, FHS says that /usr/bin "is the primary directory of executable commands on the system." Only release artifacts that are intended to be installed to be "manually" installed on a system (i.e. tarballs) should be placed under /usr/local/bin, and the rest should be under /usr/bin.

Release artifacts added in the future should follow the standard as well.

This fixes #42130, which is a proposed goal for the internal tools team for next quarter.

Requires https://github.com/gravitational/teleport.e/pull/4478 to be merged at the same time, and an e ref update will be needed as well. This will require extensive testing prior to merging. It is my recommendation that we do not backport this, rather, we include this in v17 forward so that all release assets are properly tested prior to pushing to customers. This is arguably a breaking change.

changelog: Changed the install directory for all Teleport binaries from /usr/local/bin to /usr/bin.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

🤖 Vercel preview here: https://docs-em3xhcyab-goteleport.vercel.app/docs/ver/preview

@fheinecke
Copy link
Contributor Author

fheinecke commented Jun 24, 2024

There's a couple of whitespace/linting changes in this PR caused by my IDE. I'll get them reverted prior to merge.

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Two comments here:

  • This feels like it could be a massive breaking change, we should definitely not backport it. Probably risking a bunch of regressions too. Do we want to do this?
  • It's a bit difficult to tell from the massive changeset but does this affect only locations of binaries installed via package managers? I think /usr/local/bin is the correct location for binaries installed "out of band", and /usr/bin is for package-manager-installed programs. The original ticket also only mentions the rpm and deb packages.

@fheinecke
Copy link
Contributor Author

This feels like it could be a massive breaking change, we should definitely not backport it. Probably risking a bunch of regressions too. Do we want to do this?

IMO "in a vacuum" this is an objectively good change from a technical perspective, as it moves our product in line with a well-defined and accepted industry standard.

That said I think that it is pretty likely that this will introduce some bug(s) somewhere, and should go through a set of major release tests prior to pushing this to customers. I think that it'd be smart to merge this relatively early into the v17 development cycle, as it'll give everybody internally more time to hit these issues and fix them prior to customers finding them.

It's a bit difficult to tell from the massive changeset but does this affect only locations of binaries installed via package managers? I think /usr/local/bin is the correct location for binaries installed "out of band", and /usr/bin is for package-manager-installed programs. The original ticket also only mentions the rpm and deb packages.

This changes the following in release artifacts and the related supporting files (i.e. docs, service files, etc.):

  1. Install paths for debs, rpms, and macos pkg files for all "products" (oss, ent, fips, plugins, updater, etc.).
  2. Binary path in AMIs.
  3. Binary path in container images.
  4. Install scripts in tarballs, but not the binary paths inside tarballs. Note that the install scripts are optional - when we use tarballs in our release pipelines, we ignore the script and just cp the binaries.

Artifacts from (1) is directly justified by the FHS and the customer's ticket. Changes from (2) and (3) are correct because we distribute the complete OS (well almost complete OS in the case of container images). The change in (4) is debatable. Personally I think that because the binaries are being installed by something that we produce/manage (the install script) then they should go under /usr/bin as well. This also has the advantage of keeping our install path consistent, which makes future regressions less likely.

Copy link

🤖 Vercel preview here: https://docs-o374242ms-goteleport.vercel.app/docs/ver/preview

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is going to cause the symlink under the old location to be left on the user's device.

When we detect that an upgrade is being done, we don't remove symlinks to teleport-connect and tsh in after-remove. This is because RPM first runs after-install of the new version followed by after-remove of the old version. Without the check, it meant that upgrading an RPM package removed the symlink.

deb packages however run after-remove of the old version first and then they do after-install of the new version. This would handle this change seamlessly, unfortunately we return early during upgrade of the deb package as well.

This is all done because electron-builder incorrectly uses the same after-install and after-remove scripts for both deb and RPM. For RPM, the correct way to manage symlinks is to use SPEC files. See electron-userland/electron-builder#7326 and a similar SO question I just found.


The proper way to solve this would be to use a custom SPEC file for the RPM package so that we correctly handle the tsh symlink that the package adds. This would probably require quite a lot of work, as I'm not sure if electron-builder and fpm even support that. The ultimate issue is that our teleport-connect package needs to add one extra symlink for tsh and I don't think fpm is made for that.

Should we just extend after-install.tpl to remove the tsh symlink from the old location? We could add a TODO comment to get rid of that in v18.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we just extend after-install.tpl to remove the tsh symlink from the old location? We could add a TODO comment to get rid of that in v18.0.0.

We could do this, but might need to leave the logic in through v19... do we support upgrading more than one major version at a time (i.e. v14 to v16)? Can't remember.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we recommend upgrading to the next major version first during upgrades

Upgrade the Auth Service to the next major version first. If there are data format changes introduced in the new version, the Auth Service performs the necessary migrations. After the upgrade, start the Auth Service and CONFIRM that it's in a healthy state before continuing.

@jakule
Copy link
Contributor

jakule commented Jun 25, 2024

my2c:

  1. This is a huge breaking change. If you, for example, create a systemd script using teleport systemd command (or you created it by hand), the /usr/local/bin path is hardcoded, and after the upgrade, Teleport will stop working. If you don't have any automated tool, you will need to update those paths in all places for all agents "by hand". We also don't know how much automation our users build around Teleport and how many scripts need to be updated on their side. At that point, I really don't think it's worth changing the path, even if we're not fully complying with the Linux guidelines.
  2. If we really, really want to change it we should at least create symlink from the old directory to keep some backward compatibility (symlink won't work for all cases tho).

@fheinecke
Copy link
Contributor Author

If you, for example, create a systemd script using teleport systemd command (or you created it by hand), the /usr/local/bin path is hardcoded, and after the upgrade, Teleport will stop working. If you don't have any automated tool, you will need to update those paths in all places for all agents "by hand".

100% agree. This is definitely something that will break customer environments if they don't read the changelog (presumably we will have a nice large entry about this near the top, along with a migration guide).

At that point, I really don't think it's worth changing the path, even if we're not fully complying with the Linux guidelines.

While I personally think that this work is a good step forward, I completely get this and maybe we shouldn't implement this work. One of the reasons why I filed this as a PR (rather than starting a thread in slack or an issue not directly tied to file changes) was to create a discussion with some context as to they specific impact of this change - not to necessarily get this merged immediately.

If we really, really want to change it we should at least create symlink from the old directory to keep some backward compatibility (symlink won't work for all cases tho).

Unfortunately this also would violate the parameters of the linked ticket and would continue to violate FHS. Unless we do this only as a temporarily measure for a major version or two, I think that we'd be better off not making this change if it means that we'd have a filesystem entry in both places permanently.

@zmb3
Copy link
Collaborator

zmb3 commented Jun 26, 2024

#42130 (comment)

@zmb3 zmb3 closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPM/DEB Packages incorrectly installing binaries to /usr/local/bin
7 participants