Add stack association to buildpack model [#153256959]

- Add "stack" to buildpack model
- At migration time, stacks.yml will be read (location from STACKS_YML env var)
  to determine what stack should be assigned to existing buildpacks
- Buildpacks are now unique over name AND stack
- Sets buildpack stack from manifest.yml in buildpack zip on creation
- Validate buildpack model stack against stack in buildpack zip manifest.yml
- Validate stack exists upon buildpack bits upload
- Include stack name in serialized buildpack filename
- Only provide buildpacks for the relevant stack to the staging container
- Handle buildpack stacks appropriately in the buildpack installer

NOTE: The API checkshum has changed due to adding stack as an input

Signed-off-by: Dave Goddard <dave@goddard.id.au>
Signed-off-by: Victoria Henry <vhenry@pivotal.io>
Signed-off-by: Jackson Feeny <jacksonfeeny@gmail.com>
Signed-off-by: Sam Coward <scoward@pivotal.io>
Signed-off-by: Dave Goddard <dave@goddard.id.au>
Signed-off-by: Sam Coward <scoward@pivotal.io>

app/controllers/runtime/buildpacks_controller.rb

@@ -6,17 +6,18 @@ def self.dependencies
 
     define_attributes do
       attribute :name, String
+      attribute :stack, String, default: 'unknown'
       attribute :position, Integer, default: 0
       attribute :enabled, Message::Boolean, default: true
       attribute :locked, Message::Boolean, default: false
     end
 
-    query_parameters :name
+    query_parameters :name, :stack
 
     def self.translate_validation_exception(e, attributes)
-      buildpack_errors = e.errors.on(:name)
+      buildpack_errors = e.errors.on([:name, :stack])
       if buildpack_errors && buildpack_errors.include?(:unique)
-        CloudController::Errors::ApiError.new_from_details('BuildpackNameTaken', attributes['name'])
+        CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', attributes['name'], attributes['stack'])
       else
         CloudController::Errors::ApiError.new_from_details('BuildpackInvalid', e.errors.full_messages)
       end

app/fetchers/buildpack_lifecycle_fetcher.rb

@@ -4,14 +4,19 @@ class << self
       def fetch(buildpack_names, stack_name)
         {
           stack: Stack.find(name: stack_name),
-          buildpack_infos: ordered_buildpacks(buildpack_names),
+          buildpack_infos: ordered_buildpacks(buildpack_names, stack_name),
         }
       end
 
       private
 
-      def ordered_buildpacks(buildpack_names)
-        buildpacks = Buildpack.where(name: buildpack_names).all
+      ## NOTE: if a requested system buildpack is not on the requested stack,
+      ##       the BuildpackInfo object will have a name and not a record (even
+      ##       though the buildpack exists). At this point the error returned
+      ##       to the user will probably be VERY confusing
+
+      def ordered_buildpacks(buildpack_names, stack_name)
+        buildpacks = Buildpack.where(name: buildpack_names, stack: stack_name).all
 
         buildpack_names.map do |buildpack_name|
           buildpack_record = buildpacks.find { |b| b.name == buildpack_name }

app/jobs/runtime/buildpack_installer.rb

@@ -14,9 +14,9 @@ def perform
           logger = Steno.logger('cc.background')
           logger.info "Installing buildpack #{name}"
 
-          buildpack = Buildpack.find(name: name)
+          buildpack = find_existing_buildpack
           if buildpack.nil?
-            buildpack = Buildpack.create(name: name)
+            buildpack = Buildpack.create(name: name, stack: 'unknown')
             created = true
           elsif buildpack.locked
             logger.info "Buildpack #{name} locked, not updated"
@@ -34,6 +34,8 @@ def perform
 
           buildpack.update(opts)
           logger.info "Buildpack #{name} installed or updated"
+        rescue AmbiguousBuildpackException => abe
+          logger.error abe.message
         rescue => e
           logger.error("Buildpack #{name} failed to install or update. Error: #{e.inspect}")
           raise e
@@ -51,6 +53,22 @@ def buildpack_uploader
           buildpack_blobstore = CloudController::DependencyLocator.instance.buildpack_blobstore
           UploadBuildpack.new(buildpack_blobstore)
         end
+
+        private
+
+        def find_existing_buildpack
+          stack = buildpack_uploader.extract_stack_from_buildpack(file)
+          return Buildpack.find(name: name, stack: stack) if stack.to_s != ''
+
+          buildpacks = Buildpack.where(name: name)
+          if buildpacks.count > 1
+            raise AmbiguousBuildpackException.new("Buildpack #{name} has #{buildpacks.count} stacks, not updated")
+          end
+          buildpacks.first
+        end
+
+        class AmbiguousBuildpackException < RuntimeError
+        end
       end
     end
   end

app/models/runtime/buildpack.rb

@@ -2,11 +2,13 @@ module VCAP::CloudController
   class Buildpack < Sequel::Model
     plugin :list
 
-    export_attributes :name, :position, :enabled, :locked, :filename
-    import_attributes :name, :position, :enabled, :locked, :filename, :key
+    export_attributes :name, :stack, :position, :enabled, :locked, :filename
+    import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key
 
-    def self.list_admin_buildpacks
-      exclude(key: nil).exclude(key: '').order(:position).all
+    def self.list_admin_buildpacks(stack_name=nil)
+      scoped = exclude(key: nil).exclude(key: '')
+      scoped = scoped.where(stack: stack_name) if stack_name
+      scoped.order(:position).all
     end
 
     def self.at_last_position
@@ -18,8 +20,9 @@ def self.user_visibility_filter(user)
     end
 
     def validate
-      validates_unique :name
+      validates_unique [:name, :stack]
       validates_format(/\A(\w|\-)+\z/, :name, message: 'name can only contain alphanumeric characters')
+      validates_presence :stack
     end
 
     def locked?

app/presenters/v2/buildpack_presenter.rb

@@ -0,0 +1,17 @@
+module CloudController
+  module Presenters
+    module V2
+      class BuildpackPresenter < DefaultPresenter
+        extend PresenterProvider
+
+        present_for_class 'VCAP::CloudController::Buildpack'
+
+        def entity_hash(controller, buildpack, opts, depth, parents, orphans=nil)
+          entity = super
+          entity['filename'] = "#{buildpack.filename} (#{buildpack.stack})".strip
+          entity
+        end
+      end
+    end
+  end
+end

app/presenters/v2/presenter_provider.rb

@@ -33,3 +33,4 @@ def self.presenters
 require_relative 'space_presenter'
 require_relative 'organization_presenter'
 require_relative 'service_plan_presenter'
+require_relative 'buildpack_presenter'

db/migrations/20180102183100_add_stack_to_buildpack_table.rb

@@ -0,0 +1,30 @@
+require 'yaml'
+
+def default_stack
+  stacks_yml_path = ENV.fetch('STACKS_YML', nil)
+  YAML.safe_load(File.read(stacks_yml_path))['default'] if stacks_yml_path && File.exist?(stacks_yml_path)
+end
+
+Sequel.migration do
+  up do
+    alter_table(:buildpacks) do
+      add_column :stack, String, size: 255, null: true
+      drop_index :name, unique: true
+      add_index [:name, :stack], unique: true, name: :unique_name_and_stack
+    end
+
+    self['UPDATE buildpacks SET stack = ?', default_stack || 'unknown'].update
+
+    alter_table(:buildpacks) do
+      set_column_not_null :stack
+    end
+  end
+
+  down do
+    alter_table(:buildpacks) do
+      drop_index [:name, :stack], unique: true
+      drop_column :stack
+      add_index :name, unique: true, name: :unique_name
+    end
+  end
+end

lib/cloud_controller/diego/buildpack/buildpack_entry_generator.rb

@@ -6,8 +6,8 @@ def initialize(blobstore_url_generator)
           @blobstore_url_generator = blobstore_url_generator
         end
 
-        def buildpack_entries(buildpack_infos)
-          return default_admin_buildpacks if buildpack_infos.empty?
+        def buildpack_entries(buildpack_infos, stack_name)
+          return default_admin_buildpacks(stack_name) if buildpack_infos.empty?
 
           buildpack_infos.map do |buildpack_info|
             if buildpack_info.buildpack_exists_in_db? && buildpack_info.buildpack_enabled?
@@ -26,8 +26,8 @@ def custom_buildpack_entry(buildpack)
           { name: 'custom', key: buildpack.url, url: buildpack.url }
         end
 
-        def default_admin_buildpacks
-          VCAP::CloudController::Buildpack.list_admin_buildpacks.
+        def default_admin_buildpacks(stack_name)
+          VCAP::CloudController::Buildpack.list_admin_buildpacks(stack_name).
             select(&:enabled).
             collect { |buildpack| admin_buildpack_entry(buildpack) }
         end

lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb

@@ -18,20 +18,15 @@ def initialize(blobstore_url_generator=::CloudController::DependencyLocator.inst
           end
 
           def lifecycle_data(staging_details)
+            stack                                             = staging_details.lifecycle.staging_stack
             lifecycle_data                                    = Diego::Buildpack::LifecycleData.new
             lifecycle_data.app_bits_download_uri              = @blobstore_url_generator.package_download_url(staging_details.package)
             lifecycle_data.app_bits_checksum                  = staging_details.package.checksum_info
             lifecycle_data.buildpack_cache_checksum           = staging_details.package.app.buildpack_cache_sha256_checksum
-            lifecycle_data.build_artifacts_cache_download_uri = @blobstore_url_generator.buildpack_cache_download_url(
-              staging_details.package.app_guid,
-              staging_details.lifecycle.staging_stack
-            )
-            lifecycle_data.build_artifacts_cache_upload_uri = @blobstore_url_generator.buildpack_cache_upload_url(
-              staging_details.package.app_guid,
-              staging_details.lifecycle.staging_stack
-            )
+            lifecycle_data.build_artifacts_cache_download_uri = @blobstore_url_generator.buildpack_cache_download_url(staging_details.package.app_guid, stack)
+            lifecycle_data.build_artifacts_cache_upload_uri = @blobstore_url_generator.buildpack_cache_upload_url(staging_details.package.app_guid, stack)
             lifecycle_data.droplet_upload_uri                 = @blobstore_url_generator.droplet_upload_url(staging_details.staging_guid)
-            lifecycle_data.buildpacks                         = @buildpack_entry_generator.buildpack_entries(staging_details.lifecycle.buildpack_infos)
+            lifecycle_data.buildpacks                         = @buildpack_entry_generator.buildpack_entries(staging_details.lifecycle.buildpack_infos, stack)
             lifecycle_data.stack                              = staging_details.lifecycle.staging_stack
 
             lifecycle_data.message

lib/cloud_controller/diego/lifecycles/buildpack_lifecycle.rb

@@ -36,7 +36,7 @@ def staging_environment_variables
     end
 
     def staging_stack
-      requested_stack || app_stack || VCAP::CloudController::Stack.default.name
+      requested_stack || app_stack || buildpack_stack || VCAP::CloudController::Stack.default.name
     end
 
     private
@@ -57,6 +57,13 @@ def app_stack
       @package.app.buildpack_lifecycle_data.try(:stack)
     end
 
+    def buildpack_stack
+      stacks = Buildpack.where(name: buildpacks_to_use).select(:stack).uniq
+      if stacks.length == 1
+        stacks.first
+      end
+    end
+
     attr_reader :validator
   end
 end

lib/cloud_controller/upload_buildpack.rb

@@ -24,6 +24,8 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
 
       old_buildpack_key = nil
 
+      new_stack = determine_new_stack(buildpack, bits_file_path)
+
       begin
         Buildpack.db.transaction do
           buildpack.lock!
@@ -32,6 +34,7 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
             key: new_key,
             filename: new_filename,
             sha256_checksum: sha256,
+            stack: new_stack,
           )
         end
       rescue Sequel::Error
@@ -47,8 +50,31 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
       true
     end
 
+    def extract_stack_from_buildpack(bits_file_path)
+      bits_file_path = bits_file_path.path if bits_file_path.respond_to?(:path)
+      output, _, status = Open3.capture3('unzip', '-p', bits_file_path, 'manifest.yml')
+      YAML.safe_load(output).dig('stack') if status.success?
+    end
+
     private
 
+    def determine_new_stack(buildpack, bits_file_path)
+      extracted_stack = extract_stack_from_buildpack(bits_file_path)
+      if extracted_stack.to_s != '' && !Stack.where(name: extracted_stack).first
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackStackDoesNotExist', extracted_stack)
+      end
+      new_stack = [extracted_stack, buildpack.stack, Stack.default.name].find { |s| s.to_s != '' && s.to_s != 'unknown' }
+      if buildpack.stack != 'unknown' && buildpack.stack != new_stack
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackStacksDontMatch', new_stack, buildpack.stack)
+      end
+
+      if buildpack.stack != new_stack && Buildpack.find(name: buildpack.name, stack: new_stack)
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', buildpack.name, new_stack)
+      end
+
+      new_stack
+    end
+
     def new_bits?(buildpack, key)
       buildpack.key != key
     end

spec/api/api_version_spec.rb

@@ -2,7 +2,7 @@
 require 'vcap/digester'
 
 RSpec.describe 'Stable API warning system', api_version_check: true do
-  API_FOLDER_CHECKSUM = '8d08df242e77b29ef0374b7557031985fcdfba43'.freeze
+  API_FOLDER_CHECKSUM = '5e3e3c64074e05882c6748b578a9a62f80694c94'.freeze
 
   it 'tells the developer if the API specs change' do
     api_folder = File.expand_path('..', __FILE__)

spec/api/documentation/buildpacks_api_spec.rb

@@ -19,6 +19,7 @@
         required: opts[:required],
         example_values: ['Golang_buildpack']
 
+      field :stack, 'The name of the stack to associate this buildpack with', example_values: ['cflinuxfs2']
       field :position, 'The order in which the buildpacks are checked during buildpack auto-detection.'
       field :enabled, 'Whether or not the buildpack will be used for staging', default: true
       field :locked, 'Whether or not the buildpack is locked to prevent updates', default: false
@@ -33,7 +34,7 @@
       include_context 'updatable_fields', required: true
 
       example 'Creates an admin Buildpack' do
-        client.post '/v2/buildpacks', fields_json, headers
+        client.post '/v2/buildpacks', fields_json(stack: 'cflinuxfs2'), headers
         expect(status).to eq 201
         standard_entity_response parsed_response, :buildpack, expected_values: { name: 'Golang_buildpack' }
       end

spec/request/apps_spec.rb

@@ -4,6 +4,7 @@
   let(:user) { VCAP::CloudController::User.make }
   let(:user_header) { headers_for(user, email: user_email, user_name: user_name) }
   let(:space) { VCAP::CloudController::Space.make }
+  let(:stack) { VCAP::CloudController::Stack.make }
   let(:user_email) { Sham.email }
   let(:user_name) { 'some-username' }
 
@@ -14,14 +15,14 @@
 
   describe 'POST /v3/apps' do
     it 'creates an app' do
-      buildpack      = VCAP::CloudController::Buildpack.make
+      buildpack      = VCAP::CloudController::Buildpack.make(stack: stack.name)
       create_request = {
         name: 'my_app',
         environment_variables: { open: 'source' },
         lifecycle: {
           type: 'buildpack',
           data: {
-            stack: nil,
+            stack: buildpack.stack,
             buildpacks: [buildpack.name]
           }
         },
@@ -50,7 +51,7 @@
             'type' => 'buildpack',
             'data' => {
               'buildpacks' => [buildpack.name],
-              'stack'      => VCAP::CloudController::Stack.default.name,
+              'stack'      => stack.name,
             }
           },
           'relationships' => {

spec/support/fakes/blueprints.rb

@@ -26,6 +26,7 @@
   status              { |_| %w(active suspended cancelled).sample(1).first }
   error_message       { |index| "error-message-#{index}" }
   sequence_id         { |index| index }
+  stack               { |index| "cflinuxfs-#{index}" }
 end
 
 module VCAP::CloudController
@@ -395,6 +396,7 @@ module VCAP::CloudController
 
   Buildpack.blueprint do
     name { Sham.name }
+    stack { Sham.stack }
     key { Sham.guid }
     position { 0 }
     enabled { true }

spec/support/test_zip.rb

@@ -1,14 +1,15 @@
+require 'zip'
+
 module TestZip
-  def self.create(zip_name, file_count, file_size=1024)
-    files = []
-    file_count.times do |i|
-      tf = Tempfile.new("ziptest_#{i}")
-      files << tf
-      tf.write('A' * file_size)
-      tf.close
-    end
+  def self.create(zip_name, file_count, file_size=1024, &blk)
+    Zip::File.open(zip_name, Zip::File::CREATE) do |zipfile|
+      file_count.times do |i|
+        zipfile.get_output_stream("ziptest_#{i}") do |f|
+          f.write('A' * file_size)
+        end
+      end
 
-    child = POSIX::Spawn::Child.new('zip', zip_name, *files.map(&:path))
-    child.status.exitstatus == 0 || raise("Failed zipping:\n#{child.err}\n#{child.out}")
+      blk.call(zipfile) if blk
+    end
   end
 end