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>
Signed-off-by: Sam Coward <scoward@pivotal.io>

Gemfile

@@ -30,7 +30,8 @@ gem 'public_suffix'
 gem 'railties'
 gem 'rake'
 gem 'rfc822'
-gem 'rubyzip'
+gem 'rubyzip', git: 'https://github.com/idoru/rubyzip.git', branch: 'handle-gpbit-3'
+
 gem 'sequel'
 gem 'sinatra', '~> 1.4'
 gem 'sinatra-contrib'

Gemfile.lock

@@ -16,6 +16,13 @@ GIT
   specs:
     vcap-concurrency (0.1.0)
 
+GIT
+  remote: https://github.com/idoru/rubyzip.git
+  revision: c787d94852b7d3e90212e8f7b08ad6ab6279c74d
+  branch: handle-gpbit-3
+  specs:
+    rubyzip (1.2.1)
+
 GIT
   remote: https://github.com/zipmark/rspec_api_documentation.git
   revision: de69a7a87adace29f39cbe937ae7bb8da9a4c9c1
@@ -360,7 +367,6 @@ GEM
     ruby-progressbar (1.9.0)
     ruby_parser (3.8.3)
       sexp_processor (~> 4.1)
-    rubyzip (1.2.1)
     safe_yaml (1.0.4)
     scientist (1.1.0)
     sequel (4.49.0)
@@ -488,7 +494,7 @@ DEPENDENCIES
   rspec_api_documentation!
   rubocop
   ruby-debug-ide (>= 0.6.1.beta4)
-  rubyzip
+  rubyzip!
   scientist
   sequel
   sinatra (~> 1.4)

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,16 @@ 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,42 @@
+require 'yaml'
+
+def stacks_yml
+  stacks_yml_path = ENV.fetch("STACKS_YML", nil)
+  YAML.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
+  return nil
+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[: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 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

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

@@ -1,8 +1,10 @@
 require 'vcap/digester'
 
 module VCAP::CloudController
+
   class UploadBuildpack
     attr_reader :buildpack_blobstore
+    ONE_MEGABYTE = 1024*1024
 
     def initialize(blobstore)
       @buildpack_blobstore = blobstore
@@ -24,6 +26,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,9 +36,20 @@ 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
+      rescue Sequel::ValidationFailed
+        if buildpack.errors.on([:name, :stack]).try(:include?, :unique)
+          raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', buildpack.name, new_stack)
+        end
+        if buildpack.errors.on(:stack).try(:include?, :buildpack_cant_change_stacks)
+          raise CloudController::Errors::ApiError.new_from_details('BuildpackStacksDontMatch', new_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', new_stack)
+        end
+      rescue Sequel::Error => e
         BuildpackBitsDelete.delete_when_safe(new_key, 0)
         return false
       end
@@ -49,6 +64,15 @@ def upload_buildpack(buildpack, bits_file_path, new_filename)
 
     private
 
+    def determine_new_stack(buildpack, bits_file_path)
+      extracted_stack = Buildpacks::StackNameExtractor.extract_from_file(bits_file_path)
+      new_stack = [extracted_stack, buildpack.stack, Stack.default.name].find { |s| s.present? }
+
+      new_stack
+    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 = '3655fc8ee4b47c9bbf4773e5743b65b52fa04305'.freeze
+  API_FOLDER_CHECKSUM = '88a6ea92882a662438d397872ea2a38662caa549'.freeze
 
   it 'tells the developer if the API specs change' do
     api_folder = File.expand_path('..', __FILE__)