Add stack association to buildpack model [#153256959]

- Add "stack" to buildpack model
- At migration time, buildpacks will be assigned default stack from
stacks.yml file if its path is set in the STACKS_YML env var
- hwc_buildpack is assigned the newer of windows2016,windows2012R2
stacks if present in stacks.yml
- if STACKS_YML is not set, buildpacks' stacks will be set to nil
- 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
- Use fork of rubyzip/rubyzip to work around rubyzip/rubyzip#236

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: Tyler Phelan <tphelan@pivotal.io>
Signed-off-by: Andrew Meyer <ameyer@pivotal.io>
Signed-off-by: Leah Hanson <lhanson@pivotal.io>

Gemfile

@@ -27,6 +27,7 @@ gem 'public_suffix'
 gem 'rake'
 gem 'rfc822'
 gem 'rubyzip'
+
 gem 'sequel'
 gem 'sinatra', '~> 1.4'
 gem 'sinatra-contrib'

app/controllers/runtime/buildpacks_controller.rb

@@ -6,17 +6,18 @@ def self.dependencies
 
     define_attributes do
       attribute :name, String
+      attribute :stack, String, default: nil
       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,7 +14,13 @@ def perform
           logger = Steno.logger('cc.background')
           logger.info "Installing buildpack #{name}"
 
-          buildpack = Buildpack.find(name: name)
+          buildpacks = find_existing_buildpacks
+          if buildpacks.count > 1
+            logger.error "Update failed: Unable to determine buildpack to update as there are multiple buildpacks named #{name} for different stacks."
+            return
+          end
+
+          buildpack = buildpacks.first
           if buildpack.nil?
             buildpacks_lock = Locking[name: 'buildpacks']
             buildpacks_lock.db.transaction do
@@ -55,6 +61,19 @@ def buildpack_uploader
           buildpack_blobstore = CloudController::DependencyLocator.instance.buildpack_blobstore
           UploadBuildpack.new(buildpack_blobstore)
         end
+
+        private
+
+        def find_existing_buildpacks
+          stack = VCAP::CloudController::Buildpacks::StackNameExtractor.extract_from_file(file)
+          if stack.present?
+            buildpacks_by_stack = Buildpack.where(name: name, stack: stack)
+            return buildpacks_by_stack if buildpacks_by_stack.any?
+            return Buildpack.where(name: name, stack: nil)
+          end
+
+          Buildpack.where(name: name)
+        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,11 @@ 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')
+
+      validate_stack_existence
+      validate_stack_change
     end
 
     def locked?
@@ -43,5 +48,17 @@ def to_json
     def custom?
       false
     end
+
+    private
+
+    def validate_stack_change
+      return if initial_value(:stack).nil?
+      errors.add(:stack, :buildpack_cant_change_stacks) if column_changes.key?(:stack)
+    end
+
+    def validate_stack_existence
+      return unless stack
+      errors.add(:stack, :buildpack_stack_does_not_exist) if Stack.where(name: stack).empty?
+    end
   end
 end

db/migrations/20180102183100_add_stack_to_buildpack_table.rb

@@ -0,0 +1,17 @@
+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
+  end
+
+  down do
+    alter_table(:buildpacks) do
+      drop_index [:name, :stack], unique: true, name: :unique_name_and_stack
+      drop_column :stack
+      add_index :name, unique: true, name: :buildpacks_name_index
+    end
+  end
+end

db/migrations/20180404165800_assign_stacks_to_buildpacks.rb

@@ -0,0 +1,30 @@
+require 'yaml'
+
+def stacks_yml
+  stacks_yml_path = ENV.fetch('STACKS_YML', nil)
+  YAML.safe_load(File.read(stacks_yml_path)) if stacks_yml_path && File.exist?(stacks_yml_path)
+end
+
+def default_stack
+  stacks_yml['default'] if stacks_yml
+end
+
+def latest_windows_stack
+  return unless stacks_yml
+  stack_names = stacks_yml['stacks'].map { |stack| stack['name'] }
+  if stack_names.include?('windows2016')
+    return 'windows2016'
+  end
+  if stack_names.include?('windows2012R2')
+    return 'windows2012R2'
+  end
+end
+
+Sequel.migration do
+  up do
+    self[:buildpacks].where(Sequel.negate(name: 'hwc_buildpack')).update(stack: default_stack) if default_stack
+    self[:buildpacks].where(name: 'hwc_buildpack').update(stack: latest_windows_stack) if latest_windows_stack
+  end
+
+  down {}
+end

lib/cloud_controller.rb

@@ -29,6 +29,7 @@ module VCAP::CloudController; end
 require 'cloud_controller/errors/invalid_app_relation'
 require 'cloud_controller/errors/invalid_route_relation'
 require 'cloud_controller/errors/no_running_instances'
+require 'cloud_controller/errors/buildpack_error'
 require 'delayed_job_plugins/deserialization_retry'
 require 'delayed_job_plugins/after_enqueue_hook'
 require 'sequel_plugins/sequel_plugins'
@@ -60,6 +61,7 @@ module VCAP::CloudController; end
 require 'cloud_controller/blobstore/client'
 require 'cloud_controller/blobstore/url_generator'
 require 'cloud_controller/blobstore/blob_key_generator'
+require 'cloud_controller/buildpacks/stack_name_extractor'
 require 'cloud_controller/dependency_locator'
 require 'cloud_controller/file_path_checker'
 require 'cloud_controller/rule_validator'

lib/cloud_controller/buildpacks/stack_name_extractor.rb

@@ -0,0 +1,24 @@
+module VCAP::CloudController
+  module Buildpacks
+    class StackNameExtractor
+      ONE_MEGABYTE = 1024 * 1024
+
+      def self.extract_from_file(bits_file_path)
+        bits_file_path = bits_file_path.path if bits_file_path.respond_to?(:path)
+        Zip::File.open(bits_file_path) do |zip_file|
+          zip_file.each do |entry|
+            if entry.name == 'manifest.yml'
+              raise CloudController::Errors::BuildpackError.new('buildpack manifest is too large') if entry.size > ONE_MEGABYTE
+              return YAML.safe_load(entry.get_input_stream.read).dig('stack')
+            end
+          end
+        end
+        nil
+      rescue Psych::Exception
+        raise CloudController::Errors::BuildpackError.new('buildpack manifest is not valid')
+      rescue Zip::Error
+        raise CloudController::Errors::BuildpackError.new('buildpack zipfile is not valid')
+      end
+    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).map(&:stack).uniq
+      if stacks.length == 1
+        stacks.first
+      end
+    end
+
     attr_reader :validator
   end
 end

lib/cloud_controller/errors/buildpack_error.rb

@@ -0,0 +1,5 @@
+module CloudController
+  module Errors
+    class BuildpackError < StandardError; end
+  end
+end

lib/cloud_controller/upload_buildpack.rb

@@ -3,6 +3,7 @@
 module VCAP::CloudController
   class UploadBuildpack
     attr_reader :buildpack_blobstore
+    ONE_MEGABYTE = 1024 * 1024
 
     def initialize(blobstore)
       @buildpack_blobstore = blobstore
@@ -24,6 +25,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,8 +35,11 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
             key: new_key,
             filename: new_filename,
             sha256_checksum: sha256,
+            stack: new_stack
           )
         end
+      rescue Sequel::ValidationFailed
+        raise_translated_api_error(buildpack)
       rescue Sequel::Error
         BuildpackBitsDelete.delete_when_safe(new_key, 0)
         return false
@@ -49,6 +55,25 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
 
     private
 
+    def raise_translated_api_error(buildpack)
+      if buildpack.errors.on([:name, :stack]).try(:include?, :unique)
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', buildpack.name, buildpack.stack)
+      end
+      if buildpack.errors.on(:stack).try(:include?, :buildpack_cant_change_stacks)
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackStacksDontMatch', buildpack.stack, buildpack.initial_value(:stack))
+      end
+      if buildpack.errors.on(:stack).try(:include?, :buildpack_stack_does_not_exist)
+        raise CloudController::Errors::ApiError.new_from_details('BuildpackStackDoesNotExist', buildpack.stack)
+      end
+    end
+
+    def determine_new_stack(buildpack, bits_file_path)
+      extracted_stack = Buildpacks::StackNameExtractor.extract_from_file(bits_file_path)
+      [extracted_stack, buildpack.stack, Stack.default.name].find(&:present?)
+    rescue CloudController::Errors::BuildpackError => e
+      raise CloudController::Errors::ApiError.new_from_details('BuildpackZipError', e.message)
+    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 = '8cd2d7ae629703413ef3f4c1e96368de5164f701'.freeze
+  API_FOLDER_CHECKSUM = '03f97a7ea720b63cfcd47683c34be4ea6f47efc1'.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/fixtures/config/stacks_windows2012R2.yml

@@ -0,0 +1,11 @@
+default: "default-stack-name"
+
+stacks:
+  - name: "cflinuxfs2"
+    description: "cflinuxfs2"
+  - name: "default-stack-name"
+    description: "default-stack-description"
+  - name: "cider"
+    description: "cider-description"
+  - name: "windows2012R2"
+    description: "windows2012R2-description"