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

Modernize buildstep for cedar-14 stack and Heroku buildpack requirements #109

Merged
merged 12 commits into from
Oct 17, 2014
Merged

Modernize buildstep for cedar-14 stack and Heroku buildpack requirements #109

merged 12 commits into from
Oct 17, 2014

Conversation

mjonuschat
Copy link
Contributor

Goal

Modernization of the builder architecture so that the build process more closely resembles the Heroku
workflow. This involves the following high level changes:

  • ubuntu-deboostrap:14.04 base image for a smaller surface area to begin with.
  • unmodified cedar-14 stack install script from Heroku. Ensures the installed packages match closely.
  • integrate the flynn buildback installer. Removes code duplication and allows versioning installed buildpacks
  • correctly identify the stack as cedar-14 so that the buildpacks know which binaries to install
  • switch to Heroku buildpacks for Go and PHP
  • Remove all preinstalled custom buildpacks. Lesser maintenance burden, fair for all custom buildpack authors

Actions

Removed packages

The following packages are no longer be installed on build:

  • libjpeg62, libpng12-0, libpng12-dev, libreadline5
    By choosing the right binaries for the stack these packages are no longer required
  • libmagickcore-dev, postgresql-server-dev-9.3
    These packages are implicitly installed due to dependencies of other packages
  • nodejs
    The official heroku buildpacks install a node version if it is required
  • libssl0.9.8
    Superseded by libssl1.0.0
  • mercurial
    The official heroku buildpacks install mercurial as required
  • openjdk-7-jdk, openjdk-7-jre-headless
    Will be automatically installed by the java buildpack, otherwise the multi buildpack should be used
  • postgresql-client, mysql-client
    Open for discussion. In favor of an unmodified cedar-14 stack I removed them since the benefit of having them in the container is small with a database server that should be reachable from the admin workstation.
  • libsqlite3-dev, sqlite3
    SQLite is not available on cedar-14 and not really useful on ephemeral filesystems
  • libmcrypt-dev
    Removed in favor of an unmodified cedar-14 stack image. Could be installed by a custom buildpack if required.

Removed custom buildpacks

The following custom buildpacks are no longer preinstalled in the buildstep image:

  • Metorite - https://github.com/oortcloud/heroku-buildpack-meteorite.git
  • Perl - https://github.com/miyagawa/heroku-buildpack-perl.git
  • Dart - https://github.com/igrigorik/heroku-buildpack-dart.git
  • NGINX - https://github.com/rhy-jot/buildpack-nginx.git
  • Apache - https://github.com/Kloadut/heroku-buildpack-static-apache.git
  • Jekyll - https://github.com/bacongobbler/heroku-buildpack-jekyll.git

Validation

Validation of buildstep has been performed with all sample apps provided on the Getting Started on Heroku website.

The following deploys worked without modification:

  • Ruby - OK (Ruby 2.0.0, PostgreSQL)
  • PHP - OK (PHP 5.5.11, Apache 2.4.10)
  • node.js - OK (Node.JS 0.10.32)
  • Python - OK (Python 2.7.8)
  • Java - OK (OpenJDK 1.7, Jetty 7.6.0.v20120127)
  • Clojure - OK (OpenJDK 1.6, Leinigen 2.4.2, Jetty 7.x.y-SNAPSHOT)
  • Scala - OK (OpenJDK 1.7, SBT 0.13.5, Scala 2.10.4)

Known Quirks

Since buildstep runs as root within the container newer PHP Versions (after 5.5.11) don't work without a modified php-fpm.conf. Due to CVE-2014-0185 PHP changed the default permissions for the listening socket from 0666 to 0660. Since PHP-FPM and Apache both get run as root this results in the PHP-FPM process creating the listening socket belonging to root:root with mode 0660 and Apache running as daemon:daemon which can't read the PHP socket.

The workaround for dokku involves telling PHP-FPM to set the socket to 0666 mode:

Tell the startup script to include a config fragment for PHP-FPM:
Procfile

web: vendor/bin/heroku-php-apache2 -F conf/php-fpm.inc.conf web/

Add a conf folder with the following php-fpm.inc.conf to your application
php-fpm.inc.conf

listen.owner = nobody
listen.mode = 0666

With this workaround enable the example app has been validated using PHP 5.5.16 and PHP 5.6.0 as well.
Heroku doesn't have this problems as the daemons don't get started as root and run as the same unprivileged user.

Results

Closed issues

These changes should close the following issues:

Closed pull requests

These changes should close the following pull requests:

@JMSwag
Copy link

JMSwag commented Sep 27, 2014

Good work!

@mjonuschat
Copy link
Contributor Author

I have a few more fixes in my fork that might be interesting. Since they are unrelated or major changes and build upon the work in this pull request I didn't include them here.

@JMSwag
Copy link

JMSwag commented Sep 27, 2014

Hey @yabawock, I'm getting this error when trying to use this PR.

Error response from daemon: Cannot start container b8e531768fcb7c6ce8b55067a82131272e5724b0a9268a43574670a75c1eaf3c: exec: "/build/builder": stat /build/builder: no such file or directory

@mjonuschat
Copy link
Contributor Author

Sorry, forgot to send the needed pull request for dokku. As a quick fix you need to change the cache and builder paths in line 38 of the 'dokku' binary. The new cache path is /tmp/cache and the builder is /tmp/builder/build.sh

@mjonuschat
Copy link
Contributor Author

Hey @JohnyMoSwag , i've updated the PR to work with an unmodified dokku installation, so you might want to give the latestet revision a spin. If you want to test the latest mods (including running as unprivileged user and service supervision) I've published an image on Docker Hub.

@JMSwag
Copy link

JMSwag commented Sep 28, 2014

@yabawock that quick fix you provided worked like a charm, thank you. I'm testing a fresh install now with the updated PR. I will also be putting that docker image to good use.

@kenips
Copy link

kenips commented Sep 29, 2014

<3 <3 <3 <3 <3 going to test this now!

@progrium
Copy link
Owner

This is amazing! The only suggestion is to use the cedarish base image we've been collaborating on with Deis.

@mjonuschat
Copy link
Contributor Author

@progrium Separating the PR into a part for cedarish (cedar14 branch) and one for buildstep should be really easy since those two things are already separate steps now. I'll try to get that done shortly, I just have to see how the shellshock fixes @bacongobbler put in can be combined with my intention of not messing with the Heroku stack-image script. Also there might be one or two packages in there (pigz?) that are not in the base script - we might need some feedback if they are needed for deis.

@bacongobbler
Copy link
Contributor

I'll try to get that done shortly, I just have to see how the shellshock fixes @bacongobbler put in can be combined with my intention of not messing with the Heroku stack-image script.

This was a quick workaround for shellshock. Ultimately I'd like to see these fixes in Heroku's stack-image script instead of being downstream in cedarish so I understand your concern with the modifications to Heroku's build environment. Maybe we can contribute a fix upstream for them and see what they have to say about it.

IIRC pigz is an enhancement for both Flynn's and Deis' forks of slugbuilder as it helps increase the speed of tar'ing images significantly. These are not required but was a nice enhancement. I agree that we should stick to using strictly the tools available in Heroku's environment, so I'm all for it.

As for the future of Deis, I think we're going to use Heroku's stack-images dockerfile for the cedar-14 stack directly, but that's a different discussion and probably won't be for a little while as we're bogged down with other stuff at the moment to migrate apps over to the latest Heroku stack :)

@progrium
Copy link
Owner

Is their dockerfile good enough to replace cedarish?

On Mon, Sep 29, 2014 at 4:17 PM, Matthew Fisher notifications@github.com
wrote:

I'll try to get that done shortly, I just have to see how the shellshock
fixes @bacongobbler https://github.com/bacongobbler put in can be
combined with my intention of not messing with the Heroku stack-image
script.

This was a quick workaround for shellshock. Ultimately I'd like to see
these fixes in Heroku's stack-image script instead of being downstream in
cedarish so I understand your concern with the modifications to Heroku's
build environment.

IIRC pigz is an enhancement
https://github.com/flynn/flynn/blob/master/slugbuilder/builder/build.sh#L131
for both Flynn's and Deis' forks of slugbuilder as it helps increase the
speed of tar'ing images significantly. These are not required but was a
nice enhancement
flynn-archive/slugbuilder@111542d.
I agree that we should stick to using strictly the tools available in
Heroku's environment, so I'm all for it.

As for the future of Deis, I think we're going to use Heroku's
stack-images dockerfile for the cedar-14 stack directly, but that's a
different discussion :)


Reply to this email directly or view it on GitHub
#109 (comment).

Jeff Lindsay
http://progrium.com

@bacongobbler
Copy link
Contributor

Not yet, but I think the intention of their dockerfile is to create an environment similar to heroku's cedar14 stack which would effectively replace cedarish.

@mjonuschat
Copy link
Contributor Author

the dockerfile currently is a stripped down version of the cedar-14.sh script. It does most of the work but is missing some parts like cleaning up and fixing permissions. I would keep an eye on it because it might get to the point of being a cedarish replacement but it's not there yet.

@bacongobbler I'll leave out pigz from the coming PR then, should I send a PR to deis which adds the installation to your slug builder or are shall I leave it as it is since you pinned it down to the cedar branch anyhow?

@mjonuschat
Copy link
Contributor Author

@progrium, @bacongobbler PR for cedarish sent: progrium/cedarish#9
This PR has been rebased to build upon the modified cedarish image.

@rvalyi
Copy link

rvalyi commented Sep 30, 2014

kudos for that PR work and for the discussion happening here, this is an amazing move forward!

@mjonuschat
Copy link
Contributor Author

@progrium The cedarish base-image has been updated and successfully build on dockerhub, anything else you need from me to move forward with this PR?

@bacongobbler
Copy link
Contributor

FYI we have a fork of heroku-buildpack-php which fixes the issue we see with heroku-buildpack-php: https://github.com/deis/heroku-buildpack-php/commit/14ee1559369c23f69a1330c5309c70679f002443

@mjonuschat
Copy link
Contributor Author

Thanks for the info, I've got a patch in the pipeline that makes all user started processes in the container run as the user: yabawock/buildstep@8b47bc079e1a60435b15fe8ea525322fdb0b2562

That solves the problem with the permission mixup as the webserver process and php-fpm run as the same user so that 0660 on the socket is working just fine. Maybe there is something in there use can use for deis ...

@josegonzalez
Copy link
Collaborator

@yabawock is that known quirk still a quirk?

@josegonzalez
Copy link
Collaborator

@progrium What the holdup with this PR? There are quite a few issues that would go away in progrium/dokku with this...

@bacongobbler
Copy link
Contributor

@josegonzalez yes, it is still a known issue with heroku's PHP buildpack. The fix is either the one described here in the OP or by running PHP-FPM as a non-root user.

@mjonuschat
Copy link
Contributor Author

@progrium any update on getting this merged?

@progrium progrium merged commit 2f56cec into progrium:master Oct 17, 2014
@progrium
Copy link
Owner

Big step and HUGE thank you @yabawock ...

@progrium
Copy link
Owner

Are you interested in co-maintaining?

@JMSwag
Copy link

JMSwag commented Oct 18, 2014

Yea, thanks @yabawock. Your work is greatly appreciated.

@mjonuschat mjonuschat deleted the feature/modernize branch October 18, 2014 08:33
@mjonuschat
Copy link
Contributor Author

@progrium I'd be happy to help maintaining. If you have features or known issues you want to tackle which are not listed on GitHub send me an email and I'll help where I can.

@swistaczek
Copy link

Great work @yabawock!

@swistaczek
Copy link

I have one question @yabawock, is it possible now to deploy jRuby application using ruby buildpack (requires JVM)?

@stuartpb
Copy link

stuartpb commented Nov 3, 2014

Seeing as how the modernized buildstep container is based on the cedarish image that emulates Heroku's environment, everything you can do on Heroku, package-wise, you should be able to do with buildstep now.

@kenips
Copy link

kenips commented Nov 4, 2014

That's true. In fact I've been running JRuby even with the old buildstep. Is there any particular problem that you see @swistaczek?

@mjonuschat
Copy link
Contributor Author

@swistaczek That should have been possible before the buildpack update, maybe just not the latest JRuby. Generally speaking if you follow the instructions for selecting a version from Heroku https://devcenter.heroku.com/articles/ruby-versions#selecting-a-version-of-ruby and use one of the supported versions everything should work. Supported ruby versions are listed here https://devcenter.heroku.com/articles/ruby-support#ruby-versions just keep in mind that we might be one or two releases behind on the buildpacks, so if say 1.7.16 doesn't work try 1.7.15 and if that does work open a pull request to update the buildpack.

@kenips
Copy link

kenips commented Nov 4, 2014

Actually @yabawock as a follow up question what's the benefits of locking heroku buildpack versions and what's the criteria for issuing a PR in pure buildpack version bump?

@mjonuschat
Copy link
Contributor Author

@kenips The biggest advantage in my eyes is the deterministic building of the buildstep image. Before the versions of the included buildpacks was dependent on the build time. Now it's dependent on the revision of the buildstep repository and you can be sure that a buildstep image built from a specific commit will always and on all systems include the same buildpacks. It should allow better versioning of the images and ease support for buildstep related issues.

@kenips
Copy link

kenips commented Nov 5, 2014

Certainly! Should we add this info to README (I actually didn't realize this until seeing your comments so I still assumed that buildpacks are updated at build time)? Also any requirement for PR on buildpack version bump?

@kenips
Copy link

kenips commented Jan 6, 2015

@yabawock sorry I know this has been merged for a while... but just running into an issue with /exec symlinked to /start. Was that intentional and why? Need a way to source all files and run a command and /exec is now gone (while /start is doing way too much).

@progrium
Copy link
Owner

progrium commented Jan 6, 2015

I'm having a hard time understanding how it being symlinked is causing a
problem. Can you elaborate more on what you're doing?

On Tue, Jan 6, 2015 at 11:50 AM, Ken Ip notifications@github.com wrote:

@yabawock https://github.com/yabawock sorry I know this has been merged
for a while... but just running into an issue with /exec symlinked to
/start. Was that intentional and why? Need a way to source all files and
run a command and /exec is now gone (while /start is doing way too much).


Reply to this email directly or view it on GitHub
#109 (comment).

Jeff Lindsay
http://progrium.com

@kenips
Copy link

kenips commented Jan 6, 2015

I'm using dokku-supervisord and it overrides /start to do something else. In there it calls the old /exec to source profile.d files and run Procfile commands. I guess this isn't buildstep's fault in changing how /exec works, but just wondering if there's any migrate path for /exec or plugins should write their own to source profile.d file. See statianzo/dokku-supervisord#21 (comment).

@progrium
Copy link
Owner

progrium commented Jan 6, 2015

I'll have to double check but I'm pretty sure the reason /exec symlinks to /start is because you can use /start like /exec, so ... ohhhhh, no I see. If they override /start then we can't expect it to behave like /exec. Gotcha.

Perhaps if dokku-supervisord is overriding /start, it should first copy it to /exec overriding the symlink so that it can call it as expected.

In the long term, and I'll try to make a post about this for plugin maintainers to read if it applies, there is currently a branch called herokuish (https://github.com/progrium/buildstep/tree/herokuish) that hands off everything to the herokuish tool, and buildstep just becomes a backwards compatible wrapper around it. In that branch, I believe both are symlinked.

@stuartpb
Copy link

stuartpb commented Jan 6, 2015

Herokuish looks cool, I'll rewrite plushu-cedarish to be plushu-herokuish so I can deprecate my deviant build script.

@progrium
Copy link
Owner

progrium commented Jan 6, 2015

I haven't looked at plushu-cedarish, but cedarish and herokuish are made to
work together. In this case, they basically give you buildstep. My plan is
to turn buildstep into a more generic "buildkit" that can do Heroku-ish app
builds and Dockerfile builds, and whatever else via plugins.

On Tue, Jan 6, 2015 at 3:17 PM, Stuart P. Bentley notifications@github.com
wrote:

Herokuish looks cool, I'll rewrite plushu-cedarish to be plushu-herokuish
so I can deprecate my deviant build script.


Reply to this email directly or view it on GitHub
#109 (comment).

Jeff Lindsay
http://progrium.com

@stuartpb
Copy link

stuartpb commented Jan 7, 2015

Yeah, that's basically how plushu-cedarish works right now, except I was rolling my own script to do "herokuish build". I'll submit a few issues to incorporate the necessary features of plushu-cedarish's build script into herokuish (specifically, I have an app in the Plushu sandbox that kind of needs to run as root) and then I'll just replace the build script with herokuish, and rename the plugin (which is still gonna use cedarish to run herokuish, it's just that the main component will have a more descriptive name).

@kenips
Copy link

kenips commented Jan 7, 2015

@progrium looks great - like where this is heading.

As for copying /start to /exec, that still wouldn't work. Essentially I'm simply looking for the old /exec behavior (loads profiles.d and run whatever command you throw at it). I definitely can patch the plugin to restore that behavior, but I think buildstep / herokuish should provide this functionality.

The old way was clear: /start to start application; /exec to execute a command within the app env. I don't think symlinking /start to /exec makes sense at all as it now sounds like false advertisement (that /exec no longers run any arbitrary command, as it makes assumption about either a Procfile or .release in YAML format being present).

@progrium
Copy link
Owner

progrium commented Jan 7, 2015

I guess what's not clear is that the old /exec behavior is in there:
https://github.com/progrium/buildstep/blob/master/builder/builder#L52

If it's not called via something called start it will work like /exec ... hence, copying it to /exec will mean it will behave like old /exec. Does that make sense?

@kenips
Copy link

kenips commented Jan 7, 2015

You're right! Missed the start part in there. Thanks!

@progrium
Copy link
Owner

progrium commented Jan 7, 2015

And the herokuish branch works similarly, except both are symlinks. But I should point out it will have support for supervisord-like functionality built-in, by default using forego (Go port of foreman), which as far as I know is more ideal because it's simpler (no configuration other than Procfile) and all output goes to STDOUT by default.

@kenips
Copy link

kenips commented Jan 13, 2015

@progrium sorry couldn't look at this for the past few days - I'm just looking at this now. So, if I follow you and do copy /start to /exec before overriding /start does that mean that /start has to run as root? The problem I have is that now /exec runs setuidgid (which it didn't before) so it's breaking since my /start script does not run as root.

Thanks for your help!

@mjonuschat
Copy link
Contributor Author

@kenips Both /start and /exec (being the same executable) currently expect to be run as root and both drop privileges before executing the command using setuidgid.

If you start supervisord using the unprivileged account your new /exec shouldn't try to drop the privileges again as it won't work when called from supervisord. But this would lead to permission problems when calling /exec from something like dokku run <app> <command> since that would end up being run as root.

IMHO the correct way would be to start supervisord from /start as root and have /exec handle dropping the privileges to the correct user. I only had a cursory look at dokku-supervisord but if you would remove /exec und move /start to /exec you would have a /exec that already handles the privileges correctly. Then you would just need to write a /start that generates the config and starts the supervisor daemon.

@kenips
Copy link

kenips commented Jan 15, 2015

hey @yabawock thank you so much for taking a look! While you suggestion works in theory, the reason why we moved supervisord to the unprivileged account was that supervisord would print out a critical warning when running as root (and someone filed a bug for it). So the current behaviour of running supervisord as the unprivileged user is desired, UX wise.

For now I've restored /exec to the old behaviour within dokku-supervisord. I'll probably recommend people going with herokuish in the future as well. But let's just use this as a reference point for the need of the unprivileged user running /exec. If you hear more use cases in the future then you can decide on whether or not to change its behaviour.

Many thanks again for looking into this regardless!

@mjonuschat
Copy link
Contributor Author

Does supervisord pass down the environment to the processes it spawns? If it does you might be able to read in the environment once before starting supervisord and make the supervisor config just contain the unmodified command from the procfile.

Then /exec would drop permissions as it should, supervisord and all children would keep running unprivileged as well.

@kenips
Copy link

kenips commented Jan 15, 2015

Yes it does. And I think that might work! 👍

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.

9 participants