From 18602fa878939eafcbb31bf855639a10ba6b3d13 Mon Sep 17 00:00:00 2001 From: Sam Coward Date: Tue, 2 Jan 2018 14:53:56 -0500 Subject: [PATCH] Add stack association to buildpack model [#153256959] [#154527662] - Add "stack" to buildpack model, which will be nil by default - 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 (or those with a nil stack) to the staging container - Handle buildpack stacks appropriately in the buildpack installer - Use fork of rubyzip/rubyzip to work around https://github.com/rubyzip/rubyzip/issues/236 NOTE: The API checkshum has changed due to adding stack as an input Signed-off-by: Dave Goddard Signed-off-by: Victoria Henry Signed-off-by: Jackson Feeny Signed-off-by: Tyler Phelan Signed-off-by: Andrew Meyer Signed-off-by: Leah Hanson Signed-off-by: Eric Promislow Signed-off-by: Lisa Cho --- .../runtime/buildpacks_controller.rb | 7 +- app/fetchers/buildpack_lifecycle_fetcher.rb | 8 +- app/jobs/runtime/buildpack_installer.rb | 22 ++- app/models/runtime/buildpack.rb | 30 +++- ...0102183100_add_stack_to_buildpack_table.rb | 17 +++ lib/cloud_controller.rb | 2 + .../buildpacks/stack_name_extractor.rb | 24 +++ .../buildpack/buildpack_entry_generator.rb | 8 +- .../diego/buildpack/lifecycle_protocol.rb | 15 +- .../errors/buildpack_error.rb | 5 + lib/cloud_controller/upload_buildpack.rb | 25 +++ spec/api/api_version_spec.rb | 2 +- spec/api/documentation/buildpacks_api_spec.rb | 3 +- spec/fixtures/config/stacks_windows2012R2.yml | 11 ++ spec/fixtures/config/stacks_windows2016.yml | 11 ++ spec/fixtures/config/stacks_windows_all.yml | 13 ++ spec/request/app_manifests_spec.rb | 4 +- spec/request/apps_spec.rb | 7 +- spec/support/fakes/blueprints.rb | 6 + spec/support/test_zip.rb | 21 +-- spec/unit/actions/app_update_spec.rb | 4 +- .../runtime/buildpack_bits_controller_spec.rb | 111 +++++++++++--- .../runtime/buildpacks_controller_spec.rb | 43 ++++-- .../controllers/v3/builds_controller_spec.rb | 11 +- .../buildpack_lifecycle_fetcher_spec.rb | 42 +++++- .../jobs/runtime/buildpack_delete_spec.rb | 2 +- .../jobs/runtime/buildpack_installer_spec.rb | 142 +++++++++++++++--- .../buildpacks/stack_name_extractor_spec.rb | 82 ++++++++++ .../buildpack_entry_generator_spec.rb | 35 +++-- .../buildpack/lifecycle_protocol_spec.rb | 3 +- .../lifecycles/buildpack_lifecycle_spec.rb | 2 +- .../cloud_controller/upload_buildpack_spec.rb | 109 +++++++++++++- spec/unit/models/runtime/buildpack_spec.rb | 64 +++++++- vendor/errors/v2.yml | 20 +++ 34 files changed, 780 insertions(+), 131 deletions(-) create mode 100644 db/migrations/20180102183100_add_stack_to_buildpack_table.rb create mode 100644 lib/cloud_controller/buildpacks/stack_name_extractor.rb create mode 100644 lib/cloud_controller/errors/buildpack_error.rb create mode 100644 spec/fixtures/config/stacks_windows2012R2.yml create mode 100644 spec/fixtures/config/stacks_windows2016.yml create mode 100644 spec/fixtures/config/stacks_windows_all.yml create mode 100644 spec/unit/lib/cloud_controller/buildpacks/stack_name_extractor_spec.rb diff --git a/app/controllers/runtime/buildpacks_controller.rb b/app/controllers/runtime/buildpacks_controller.rb index 88f56f4d457..deee30897eb 100644 --- a/app/controllers/runtime/buildpacks_controller.rb +++ b/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 diff --git a/app/fetchers/buildpack_lifecycle_fetcher.rb b/app/fetchers/buildpack_lifecycle_fetcher.rb index a8ef6224a4c..c997570ba61 100644 --- a/app/fetchers/buildpack_lifecycle_fetcher.rb +++ b/app/fetchers/buildpack_lifecycle_fetcher.rb @@ -4,17 +4,17 @@ 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 + def ordered_buildpacks(buildpack_names, stack_name) + buildpacks_with_stacks, buildpacks_without_stacks = Buildpack.list_admin_buildpacks(stack_name).partition{|buildpack| buildpack.stack } buildpack_names.map do |buildpack_name| - buildpack_record = buildpacks.find { |b| b.name == buildpack_name } + buildpack_record = buildpacks_with_stacks.find { |b| b.name == buildpack_name} || buildpacks_without_stacks.find { |b| b.name == buildpack_name } BuildpackInfo.new(buildpack_name, buildpack_record) end end diff --git a/app/jobs/runtime/buildpack_installer.rb b/app/jobs/runtime/buildpack_installer.rb index 9432ad95fbc..a7737c6e1c7 100644 --- a/app/jobs/runtime/buildpack_installer.rb +++ b/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,20 @@ 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) + # XTEAM: We were reconsidering whether or not we should overwrite buildpacks of unknown stack during install + end + + Buildpack.where(name: name) + end end end end diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 6ab63186a41..e0db921db35 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -2,11 +2,16 @@ 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.filter(Sequel.or([ + [:stack, stack_name], + [:stack, nil] + ])) if stack_name.present? + scoped.order(:position).all end def self.at_last_position @@ -18,8 +23,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 +51,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 diff --git a/db/migrations/20180102183100_add_stack_to_buildpack_table.rb b/db/migrations/20180102183100_add_stack_to_buildpack_table.rb new file mode 100644 index 00000000000..14eee2db21f --- /dev/null +++ b/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 diff --git a/lib/cloud_controller.rb b/lib/cloud_controller.rb index a15117d49fc..c6784cb7b2a 100644 --- a/lib/cloud_controller.rb +++ b/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' diff --git a/lib/cloud_controller/buildpacks/stack_name_extractor.rb b/lib/cloud_controller/buildpacks/stack_name_extractor.rb new file mode 100644 index 00000000000..c11f8568e6c --- /dev/null +++ b/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 diff --git a/lib/cloud_controller/diego/buildpack/buildpack_entry_generator.rb b/lib/cloud_controller/diego/buildpack/buildpack_entry_generator.rb index 792ddcbb80c..f8aa9517b28 100644 --- a/lib/cloud_controller/diego/buildpack/buildpack_entry_generator.rb +++ b/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 diff --git a/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb b/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb index 5e921c9ffa5..cc296df1ff6 100644 --- a/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb +++ b/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb @@ -18,21 +18,16 @@ 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.stack = staging_details.lifecycle.staging_stack + lifecycle_data.buildpacks = @buildpack_entry_generator.buildpack_entries(staging_details.lifecycle.buildpack_infos, stack) + lifecycle_data.stack = stack lifecycle_data.message rescue Membrane::SchemaValidationError => e diff --git a/lib/cloud_controller/errors/buildpack_error.rb b/lib/cloud_controller/errors/buildpack_error.rb new file mode 100644 index 00000000000..16c16df430a --- /dev/null +++ b/lib/cloud_controller/errors/buildpack_error.rb @@ -0,0 +1,5 @@ +module CloudController + module Errors + class BuildpackError < StandardError; end + end +end diff --git a/lib/cloud_controller/upload_buildpack.rb b/lib/cloud_controller/upload_buildpack.rb index 3b5865cefb2..f06189e7316 100644 --- a/lib/cloud_controller/upload_buildpack.rb +++ b/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 diff --git a/spec/api/api_version_spec.rb b/spec/api/api_version_spec.rb index 010f2554f6b..f184522fb71 100644 --- a/spec/api/api_version_spec.rb +++ b/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__) diff --git a/spec/api/documentation/buildpacks_api_spec.rb b/spec/api/documentation/buildpacks_api_spec.rb index 5e88b80e225..3ee54d27e9b 100644 --- a/spec/api/documentation/buildpacks_api_spec.rb +++ b/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 diff --git a/spec/fixtures/config/stacks_windows2012R2.yml b/spec/fixtures/config/stacks_windows2012R2.yml new file mode 100644 index 00000000000..8ae503bdc66 --- /dev/null +++ b/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" diff --git a/spec/fixtures/config/stacks_windows2016.yml b/spec/fixtures/config/stacks_windows2016.yml new file mode 100644 index 00000000000..4d40fcd55e1 --- /dev/null +++ b/spec/fixtures/config/stacks_windows2016.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: "windows2016" + description: "windows2016-description" diff --git a/spec/fixtures/config/stacks_windows_all.yml b/spec/fixtures/config/stacks_windows_all.yml new file mode 100644 index 00000000000..4c9cd5bdd37 --- /dev/null +++ b/spec/fixtures/config/stacks_windows_all.yml @@ -0,0 +1,13 @@ +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" + - name: "windows2016" + description: "windows2016-description" diff --git a/spec/request/app_manifests_spec.rb b/spec/request/app_manifests_spec.rb index 822d3421d82..d05cf86b745 100644 --- a/spec/request/app_manifests_spec.rb +++ b/spec/request/app_manifests_spec.rb @@ -25,7 +25,7 @@ 'memory' => '2048MB', 'disk_quota' => '1.5GB', 'buildpack' => buildpack.name, - 'stack' => 'cflinuxfs2', + 'stack' => buildpack.stack, 'command' => 'new-command', 'health_check_type' => 'http', 'health_check_http_endpoint' => '/health', @@ -69,7 +69,7 @@ app_model.reload lifecycle_data = app_model.lifecycle_data expect(lifecycle_data.buildpacks).to include(buildpack.name) - expect(lifecycle_data.stack).to eq('cflinuxfs2') + expect(lifecycle_data.stack).to eq(buildpack.stack) expect(app_model.environment_variables).to match( 'k1' => 'mangos', 'k2' => 'pears', diff --git a/spec/request/apps_spec.rb b/spec/request/apps_spec.rb index 8a111fe9bc7..54f13292229 100644 --- a/spec/request/apps_spec.rb +++ b/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' => { diff --git a/spec/support/fakes/blueprints.rb b/spec/support/fakes/blueprints.rb index e7f186f75a9..df9b8031166 100644 --- a/spec/support/fakes/blueprints.rb +++ b/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 @@ -403,6 +404,7 @@ module VCAP::CloudController Buildpack.blueprint do name { Sham.name } + stack { Stack.make.name } key { Sham.guid } position { 0 } enabled { true } @@ -410,6 +412,10 @@ module VCAP::CloudController locked { false } end + Buildpack.blueprint(:nil_stack) do + stack { nil } + end + BuildpackLifecycleDataModel.blueprint do buildpacks { nil } stack { Stack.make.name } diff --git a/spec/support/test_zip.rb b/spec/support/test_zip.rb index 199249c44fe..cbca72070eb 100644 --- a/spec/support/test_zip.rb +++ b/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 diff --git a/spec/unit/actions/app_update_spec.rb b/spec/unit/actions/app_update_spec.rb index 44406c9374f..f9e0eb803e0 100644 --- a/spec/unit/actions/app_update_spec.rb +++ b/spec/unit/actions/app_update_spec.rb @@ -11,7 +11,7 @@ module VCAP::CloudController let(:user_audit_info) { UserAuditInfo.new(user_email: user_email, user_guid: user_guid) } let(:buildpack) { 'http://original.com' } let(:app_name) { 'original name' } - let!(:ruby_buildpack) { Buildpack.make(name: 'ruby') } + let!(:ruby_buildpack) { Buildpack.make(name: 'ruby', stack: stack.name) } let(:stack) { Stack.make(name: 'SUSE') } before do @@ -89,7 +89,7 @@ module VCAP::CloudController it 'raises an AppUpdate::InvalidApp error' do expect { app_update.update(app_model, message, lifecycle) - }.to raise_error(AppUpdate::InvalidApp, 'Stack must be an existing stack') + }.to raise_error(AppUpdate::InvalidApp, 'Buildpack "ruby" must be an existing admin buildpack or a valid git URI, Stack must be an existing stack') end end diff --git a/spec/unit/controllers/runtime/buildpack_bits_controller_spec.rb b/spec/unit/controllers/runtime/buildpack_bits_controller_spec.rb index 4261739fe02..1721d0c9264 100644 --- a/spec/unit/controllers/runtime/buildpack_bits_controller_spec.rb +++ b/spec/unit/controllers/runtime/buildpack_bits_controller_spec.rb @@ -8,6 +8,26 @@ module VCAP::CloudController let(:sha_valid_zip) { Digester.new(algorithm: Digest::SHA256).digest_file(valid_zip) } let(:sha_valid_zip2) { Digester.new(algorithm: Digest::SHA256).digest_file(valid_zip2) } let(:sha_valid_tar_gz) { Digester.new(algorithm: Digest::SHA256).digest_file(valid_tar_gz) } + let(:valid_zip_manifest) do + zip_name = File.join(tmpdir, filename) + TestZip.create(zip_name, 1, 1024) do |zipfile| + zipfile.get_output_stream('manifest.yml') do |f| + f.write("---\nstack: stack-from-manifest\n") + end + end + zip_file = File.new(zip_name) + Rack::Test::UploadedFile.new(zip_file) + end + let(:valid_zip_unknown_stack) do + zip_name = File.join(tmpdir, filename) + TestZip.create(zip_name, 1, 1024) do |zipfile| + zipfile.get_output_stream('manifest.yml') do |f| + f.write("---\nstack: unknown-stack\n") + end + end + zip_file = File.new(zip_name) + Rack::Test::UploadedFile.new(zip_file) + end let(:valid_zip) do zip_name = File.join(tmpdir, filename) TestZip.create(zip_name, 1, 1024) @@ -39,7 +59,7 @@ module VCAP::CloudController after { FileUtils.rm_rf(tmpdir) } context 'Buildpack binaries' do - let(:test_buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', position: 0 }) } + let(:test_buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', stack: nil, position: 0 }) } before { CloudController::DependencyLocator.instance.register(:upload_handler, UploadHandler.new(TestConfig.config_instance)) } @@ -48,6 +68,9 @@ module VCAP::CloudController TestConfig.override(directories: { tmpdir: File.dirname(valid_zip.path) }) @cache = Delayed::Worker.delay_jobs Delayed::Worker.delay_jobs = false + + Stack.create(name: 'stack') + Stack.create(name: 'stack-from-manifest') end after { Delayed::Worker.delay_jobs = @cache } @@ -67,25 +90,67 @@ module VCAP::CloudController end it 'takes a buildpack file and adds it to the custom buildpacks blobstore with the correct key' do + test_buildpack.update(stack: 'stack') + allow(CloudController::DependencyLocator.instance.upload_handler).to receive(:uploaded_file).and_return(valid_zip.path) buildpack_blobstore = CloudController::DependencyLocator.instance.buildpack_blobstore expected_key = "#{test_buildpack.guid}_#{sha_valid_zip}" put "/v2/buildpacks/#{test_buildpack.guid}/bits", upload_body + expect(last_response.status).to eq(201) + buildpack = Buildpack.find(name: 'upload_binary_buildpack') expect(buildpack.key).to eq(expected_key) expect(buildpack.filename).to eq(filename) + expect(buildpack.stack).to eq('stack') expect(buildpack_blobstore.exists?(expected_key)).to be true end it 'gets the uploaded file from the upload handler' do upload_handler = CloudController::DependencyLocator.instance.upload_handler - expect(upload_handler).to receive(:uploaded_file). - with(hash_including('buildpack_name' => filename), 'buildpack'). - and_return(valid_zip) + expect(upload_handler).to receive(:uploaded_file).with(hash_including('buildpack_name' => filename), 'buildpack').and_return(valid_zip) put "/v2/buildpacks/#{test_buildpack.guid}/bits", upload_body end + it 'sets the buildpack stack if it is unset and in buildpack manifest' do + put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip_manifest, buildpack_name: valid_zip_manifest.path } + expect(last_response.status).to eql 201 + + buildpack = Buildpack.find(name: 'upload_binary_buildpack') + expect(buildpack.stack).to eq('stack-from-manifest') + end + + it 'returns ERROR (422) if provided stack does not exist' do + put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip_unknown_stack, buildpack_name: valid_zip_unknown_stack.path } + expect(last_response.status).to eql 422 + + buildpack = Buildpack.find(name: 'upload_binary_buildpack') + expect(buildpack.stack).to be_nil + end + + it 'sets the buildpack stack to default if it is unset and NOT in buildpack manifest' do + put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip, buildpack_name: valid_zip.path } + expect(last_response.status).to eql 201 + + buildpack = Buildpack.find(name: 'upload_binary_buildpack') + expect(buildpack.stack).to eq('default-stack-name') + end + + it 'requires an existing stack to be the same as one in the manifest if it exists' do + Stack.make(name: 'not-from-manifest') + test_buildpack.update(stack: 'not-from-manifest') + + put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip_manifest, buildpack_name: valid_zip_manifest.path } + expect(last_response.status).to eql 422 + + json = MultiJson.load(last_response.body) + expect(json['code']).to eq(390011) + expect(json['description']).to eql 'Uploaded buildpack stack (stack-from-manifest) does not match not-from-manifest' + + buildpack = Buildpack.find(name: 'upload_binary_buildpack') + expect(buildpack.stack).to eq('not-from-manifest') + end + it 'requires a filename as part of the upload' do put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: 'abc' } expect(last_response.status).to eql 400 @@ -115,6 +180,8 @@ module VCAP::CloudController end it 'removes the old buildpack binary when a new one is uploaded' do + test_buildpack.update(stack: 'stack') + put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip2 } expected_sha = "#{test_buildpack.guid}_#{sha_valid_zip2}" @@ -123,19 +190,29 @@ module VCAP::CloudController put "/v2/buildpacks/#{test_buildpack.guid}/bits", upload_body response = MultiJson.load(last_response.body) - entity = response['entity'] - expect(entity['name']).to eq('upload_binary_buildpack') - expect(entity['filename']).to eq(filename) + expect(response['entity']['name']).to eq('upload_binary_buildpack') + expect(response['entity']['filename']).to eq(filename) expect(buildpack_blobstore.exists?(expected_sha)).to be false end it 'reports a no content if the same buildpack is uploaded again' do - put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip } - put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip } + 2.times { put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip } } expect(last_response.status).to eq(204) end + it 'does not allow uploading a buildpack which will update the stack that already has a buildpack with the same name' do + first_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: 'nice_buildpack', stack: nil, position: 0 }) + put "/v2/buildpacks/#{first_buildpack.guid}/bits", { buildpack: valid_zip_manifest } + + expect(Buildpack.find(name: 'nice_buildpack').stack).to eq('stack-from-manifest') + + new_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: first_buildpack.name, stack: nil, position: 0 }) + put "/v2/buildpacks/#{new_buildpack.guid}/bits", { buildpack: valid_zip_manifest } + + expect(last_response.status).to eq(422) + end + it 'allowed when same bits but different filename are uploaded again' do put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip } new_name = File.join(File.dirname(valid_zip.path), 'newfilename.zip') @@ -152,13 +229,13 @@ module VCAP::CloudController end it 'does not allow upload if the buildpack is locked' do - locked_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: 'locked_buildpack', locked: true, position: 0 }) + locked_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: 'locked_buildpack', stack: 'stack', locked: true, position: 0 }) put "/v2/buildpacks/#{locked_buildpack.guid}/bits", { buildpack: valid_zip2 } expect(last_response.status).to eq(409) end it 'does allow upload if the buildpack has been unlocked' do - locked_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: 'locked_buildpack', locked: true, position: 0 }) + locked_buildpack = VCAP::CloudController::Buildpack.create_from_hash({ name: 'locked_buildpack', stack: 'stack', locked: true, position: 0 }) put "/v2/buildpacks/#{locked_buildpack.guid}", '{"locked": false}' put "/v2/buildpacks/#{locked_buildpack.guid}/bits", { buildpack: valid_zip2 } @@ -174,7 +251,7 @@ module VCAP::CloudController end context 'when the same bits are uploaded twice' do - let(:test_buildpack2) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'buildpack2', position: 0 }) } + let(:test_buildpack2) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'buildpack2', stack: 'stack', position: 0 }) } before do put "/v2/buildpacks/#{test_buildpack.guid}/bits", { buildpack: valid_zip2 } put "/v2/buildpacks/#{test_buildpack2.guid}/bits", { buildpack: valid_zip2 } @@ -193,20 +270,14 @@ module VCAP::CloudController let(:staging_password) { 'password' } let(:staging_config) do { - staging: { - timeout_in_seconds: 240, - auth: { - user: staging_user, - password: staging_password, - }, - }, + staging: { timeout_in_seconds: 240, auth: { user: staging_user, password: staging_password } }, directories: { tmpdir: File.dirname(valid_zip.path) }, } end before do TestConfig.override(staging_config) - VCAP::CloudController::Buildpack.create_from_hash({ name: 'get_binary_buildpack', key: 'xyz', position: 0 }) + VCAP::CloudController::Buildpack.create_from_hash({ name: 'get_binary_buildpack', stack: nil, key: 'xyz', position: 0 }) end it 'returns NOT AUTHENTICATED (401) users without correct basic auth' do diff --git a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb index 705d1cf34f3..358b9f0bca5 100644 --- a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb @@ -7,7 +7,8 @@ def ordered_buildpacks end let(:user) { make_user } - let(:req_body) { MultiJson.dump({ name: 'dynamic_test_buildpack', position: 1 }) } + let(:stack) { Stack.make } + let(:req_body) { MultiJson.dump({ name: 'dynamic_test_buildpack', stack: stack.name, position: 1 }) } before { set_current_user_as_admin } @@ -19,6 +20,7 @@ def ordered_buildpacks it do expect(VCAP::CloudController::BuildpacksController).to have_creatable_attributes({ name: { type: 'string', required: true }, + stack: { type: 'string' }, position: { type: 'integer', default: 0 }, enabled: { type: 'bool', default: true }, locked: { type: 'bool', default: false } @@ -28,6 +30,7 @@ def ordered_buildpacks it do expect(VCAP::CloudController::BuildpacksController).to have_updatable_attributes({ name: { type: 'string' }, + stack: { type: 'string' }, position: { type: 'integer' }, enabled: { type: 'bool' }, locked: { type: 'bool' } @@ -59,12 +62,30 @@ def ordered_buildpacks expect(buildpack.position).to eq(1) end + it 'defaults stack to nil' do + expect do + post '/v2/buildpacks', MultiJson.dump({ name: 'a_buildpack', position: 1 }) + expect(last_response.status).to eq(201) + end.to change { Buildpack.count }.from(0).to(1) + buildpack = Buildpack.first + expect(buildpack.stack).to be_nil + end + + it 'uses specified stack' do + expect do + post '/v2/buildpacks', MultiJson.dump({ name: 'a_buildpack', stack: stack.name, position: 1 }) + expect(last_response.status).to eq(201) + end.to change { Buildpack.count }.from(0).to(1) + buildpack = Buildpack.first + expect(buildpack.stack).to eq(stack.name) + end + it 'respects position param' do - Buildpack.create(name: 'pre-existing-buildpack', position: 1) - Buildpack.create(name: 'pre-existing-buildpack-2', position: 2) + Buildpack.create(name: 'pre-existing-buildpack', stack: stack.name, position: 1) + Buildpack.create(name: 'pre-existing-buildpack-2', stack: stack.name, position: 2) expect { - post '/v2/buildpacks', MultiJson.dump({ name: 'new-buildpack', position: 2 }) + post '/v2/buildpacks', MultiJson.dump({ name: 'new-buildpack', stack: stack.name, position: 2 }) }.to change { ordered_buildpacks }.from( [['pre-existing-buildpack', 1], ['pre-existing-buildpack-2', 2]] ).to( @@ -73,15 +94,15 @@ def ordered_buildpacks end it 'returns duplicate name message correctly' do - Buildpack.make(name: 'dynamic_test_buildpack') + Buildpack.make(name: 'dynamic_test_buildpack', stack: stack.name) post '/v2/buildpacks', req_body - expect(last_response.status).to eq(400) - expect(decoded_response['code']).to eq(290001) + expect(last_response.status).to eq(422) + expect(decoded_response['code']).to eq(290000) expect(Buildpack.count).to eq(1) end it 'returns buildpack invalid message correctly' do - post '/v2/buildpacks', MultiJson.dump({ name: 'invalid_name!' }) + post '/v2/buildpacks', MultiJson.dump({ name: 'invalid_name!', stack: stack.name }) expect(last_response.status).to eq(400) expect(decoded_response['code']).to eq(290003) expect(Buildpack.count).to eq(0) @@ -97,8 +118,8 @@ def ordered_buildpacks end describe '#update' do - let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', key: 'xyz', filename: 'a', position: 1 }) } - let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', key: 'xyz', filename: 'b', position: 2 }) } + let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', stack: stack.name, key: 'xyz', filename: 'a', position: 1 }) } + let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', stack: stack.name, key: 'xyz', filename: 'b', position: 2 }) } it 'can update the buildpack name' do set_current_user_as_admin @@ -127,7 +148,7 @@ def ordered_buildpacks end describe '#delete' do - let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', key: 'xyz', position: 1 }) } + let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', stack: stack.name, key: 'xyz', position: 1 }) } it 'returns NOT AUTHORIZED (403) for non admins' do set_current_user(user) diff --git a/spec/unit/controllers/v3/builds_controller_spec.rb b/spec/unit/controllers/v3/builds_controller_spec.rb index 9326b36e70f..9fcbf2c8a33 100644 --- a/spec/unit/controllers/v3/builds_controller_spec.rb +++ b/spec/unit/controllers/v3/builds_controller_spec.rb @@ -156,6 +156,7 @@ let(:user) { set_current_user(VCAP::CloudController::User.make) } let(:org) { VCAP::CloudController::Organization.make } let(:space) { VCAP::CloudController::Space.make(organization: org) } + let(:stack) { VCAP::CloudController::Stack.default.name } let(:app_model) { VCAP::CloudController::AppModel.make(space: space) } let(:package) do VCAP::CloudController::PackageModel.make( @@ -177,7 +178,7 @@ before do allow(CloudController::DependencyLocator.instance).to receive(:stagers).and_return(stagers) allow(stagers).to receive(:stager_for_app).and_return(stager) - app_model.lifecycle_data.update(buildpacks: nil, stack: VCAP::CloudController::Stack.default.name) + app_model.lifecycle_data.update(buildpacks: nil, stack: stack) set_current_user_as_admin end @@ -250,14 +251,14 @@ end describe 'buildpack lifecycle' do - let(:buildpack) { VCAP::CloudController::Buildpack.make } + let(:buildpack) { VCAP::CloudController::Buildpack.make(stack: stack) } let(:buildpack_request) { 'http://dan-and-zach-awesome-pack.com' } let(:buildpack_lifecycle) do { type: 'buildpack', data: { buildpacks: [buildpack_request], - stack: 'cflinuxfs2' + stack: stack }, } end @@ -318,7 +319,7 @@ type: 'buildpack', data: { buildpacks: [], - stack: 'cflinuxfs2' + stack: stack }, } end @@ -337,7 +338,7 @@ type: 'buildpack', data: { buildpacks: nil, - stack: 'cflinuxfs2' + stack: stack }, } end diff --git a/spec/unit/fetchers/buildpack_lifecycle_fetcher_spec.rb b/spec/unit/fetchers/buildpack_lifecycle_fetcher_spec.rb index 51a1dbb0718..cf259d68092 100644 --- a/spec/unit/fetchers/buildpack_lifecycle_fetcher_spec.rb +++ b/spec/unit/fetchers/buildpack_lifecycle_fetcher_spec.rb @@ -6,16 +6,49 @@ module VCAP::CloudController let(:fetcher) { BuildpackLifecycleFetcher.new } describe '#fetch' do - let(:stack) { Stack.make } - let!(:buildpack) { Buildpack.make(name: 'buildpack-1') } - let!(:buildpack2) { Buildpack.make(name: 'buildpack-2') } + let!(:stack) { Stack.make } + let!(:stack2) { Stack.make } - it 'returns the stack and buildpack' do + let!(:buildpack) { Buildpack.make(name: 'buildpack-1', stack: stack.name) } + let!(:buildpack2) { Buildpack.make(name: 'buildpack-2', stack: stack.name) } + let!(:buildpack3) { Buildpack.make(name: 'buildpack-2', stack: stack2.name) } + + it 'returns the stack and buildpack for the given stack' do returned_hash = BuildpackLifecycleFetcher.fetch([buildpack2.name, buildpack.name, 'http://buildpack.example.com'], stack.name) expect(returned_hash[:stack]).to eq(stack) buildpack_infos = returned_hash[:buildpack_infos] expect(buildpack_infos.map(&:buildpack)).to eq(['buildpack-2', 'buildpack-1', 'http://buildpack.example.com']) + expect(buildpack_infos.map(&:buildpack_record)).to eq([buildpack2, buildpack, nil]) + end + + context 'buildpacks with unknown stack exist' do + context 'only buildpack with nil stack exists' do + let!(:stack3) { Stack.make } + let!(:buildpack4) { Buildpack.make(:nil_stack, name: 'buildpack-3') } + + it 'returns the stack and buildpack' do + returned_hash = BuildpackLifecycleFetcher.fetch(['buildpack-3'], stack3.name) + expect(returned_hash[:stack]).to eq(stack3) + + buildpack_infos = returned_hash[:buildpack_infos] + expect(buildpack_infos.map(&:buildpack)).to eq(['buildpack-3']) + expect(buildpack_infos.map(&:buildpack_record)).to eq([buildpack4]) + end + end + + context 'buildpack with nil stack and matching stack both exist' do + let!(:buildpack4) { Buildpack.make(:nil_stack, name: 'buildpack-2') } + + it 'chooses the buildpack with non-nil stack' do + returned_hash = BuildpackLifecycleFetcher.fetch([buildpack2.name, buildpack.name, 'http://buildpack.example.com'], stack.name) + expect(returned_hash[:stack]).to eq(stack) + + buildpack_infos = returned_hash[:buildpack_infos] + expect(buildpack_infos.map(&:buildpack)).to eq(['buildpack-2', 'buildpack-1', 'http://buildpack.example.com']) + expect(buildpack_infos.map(&:buildpack_record)).to eq([buildpack2, buildpack, nil]) + end + end end context 'when the stack and buildpack are not found' do @@ -25,6 +58,7 @@ module VCAP::CloudController buildpack_infos = returned_hash[:buildpack_infos] expect(buildpack_infos.map(&:buildpack)).to eq(['bogus-buildpack']) expect(returned_hash[:stack]).to be_nil + expect(buildpack_infos.map(&:buildpack_record)).to eq([nil]) end end diff --git a/spec/unit/jobs/runtime/buildpack_delete_spec.rb b/spec/unit/jobs/runtime/buildpack_delete_spec.rb index 3d89f71baca..75c319541e4 100644 --- a/spec/unit/jobs/runtime/buildpack_delete_spec.rb +++ b/spec/unit/jobs/runtime/buildpack_delete_spec.rb @@ -7,7 +7,7 @@ module Jobs::Runtime let(:buildpack_guid) { buildpack.guid } let(:timeout) { 90000 } - let(:buildpack) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', key: 'xyz', position: 1 }) } + let(:buildpack) { VCAP::CloudController::Buildpack.make } before do allow(BuildpackBitsDelete).to receive(:delete_when_safe) diff --git a/spec/unit/jobs/runtime/buildpack_installer_spec.rb b/spec/unit/jobs/runtime/buildpack_installer_spec.rb index 6fbef48f115..2bcb37660d3 100644 --- a/spec/unit/jobs/runtime/buildpack_installer_spec.rb +++ b/spec/unit/jobs/runtime/buildpack_installer_spec.rb @@ -20,30 +20,132 @@ module Jobs::Runtime context 'when the buildpack is enabled and unlocked' do let(:options) { { locked: true } } - it 'creates a new buildpack' do - expect { - job.perform - }.to change { Buildpack.count }.from(0).to(1) - - buildpack = Buildpack.find(name: buildpack_name) - expect(buildpack).to_not be_nil - expect(buildpack.name).to eq(buildpack_name) - expect(buildpack.key).to start_with(buildpack.guid) - expect(buildpack.filename).to end_with(File.basename(zipfile)) - expect(buildpack).to be_locked + context 'buildpack zip does not specify stack' do + it 'creates a new buildpack with default stack' do + expect { + job.perform + }.to change { Buildpack.count }.from(0).to(1) + + buildpack = Buildpack.first + expect(buildpack).to_not be_nil + expect(buildpack.name).to eq(buildpack_name) + expect(buildpack.stack).to eq(Stack.default.name) + expect(buildpack.key).to start_with(buildpack.guid) + expect(buildpack.filename).to end_with(File.basename(zipfile)) + expect(buildpack).to be_locked + end + + it 'updates an existing buildpack' do + buildpack1 = Buildpack.make(name: buildpack_name, key: 'new_key') + + update_job = BuildpackInstaller.new(buildpack_name, zipfile2, { enabled: false }) + update_job.perform + + buildpack2 = Buildpack.find(name: buildpack_name) + expect(buildpack2).to_not be_nil + expect(buildpack2.enabled).to be false + expect(buildpack2.filename).to end_with(File.basename(zipfile2)) + expect(buildpack2.key).to_not eql(buildpack1.key) + end + + it 'does nothing if multiple buildpacks with same name' do + Stack.make(name: 'stack-1') + Stack.make(name: 'stack-2') + Buildpack.make(name: buildpack_name, stack: 'stack-1', filename: nil) + Buildpack.make(name: buildpack_name, stack: 'stack-2', filename: nil) + + update_job = BuildpackInstaller.new(buildpack_name, zipfile2, { enabled: false }) + expect { + update_job.perform + }.to_not change { Buildpack.count } + + buildpack1 = Buildpack.find(name: buildpack_name, stack: 'stack-1') + expect(buildpack1).to_not be_nil + expect(buildpack1.filename).to be_nil + + buildpack2 = Buildpack.find(name: buildpack_name, stack: 'stack-2') + expect(buildpack2).to_not be_nil + expect(buildpack2.filename).to be_nil + end end - it 'updates an existing buildpack' do - buildpack1 = Buildpack.make(name: buildpack_name, key: 'new_key') + context 'buildpack zip specifies stack' do + before { Stack.make(name: 'manifest-stack') } + let(:zipfile) do + path = Tempfile.new('bp-zip-with-stack').path + TestZip.create(path, 1, 1024) do |zipfile| + zipfile.get_output_stream('manifest.yml') do |f| + f.write("---\nstack: manifest-stack\n") + end + end + path + end + after { FileUtils.rm(zipfile) } - update_job = BuildpackInstaller.new(buildpack_name, zipfile2, { enabled: false }) - update_job.perform + it 'creates a new buildpack with that stack' do + expect { + job.perform + }.to change { Buildpack.count }.from(0).to(1) + + buildpack = Buildpack.first + expect(buildpack).to_not be_nil + expect(buildpack.name).to eq(buildpack_name) + expect(buildpack.stack).to eq('manifest-stack') + expect(buildpack.key).to start_with(buildpack.guid) + expect(buildpack.filename).to end_with(File.basename(zipfile)) + expect(buildpack).to be_locked + end - buildpack2 = Buildpack.find(name: buildpack_name) - expect(buildpack2).to_not be_nil - expect(buildpack2.enabled).to be false - expect(buildpack2.filename).to end_with(File.basename(zipfile2)) - expect(buildpack2.key).to_not eql(buildpack1.key) + it 'updates an existing buildpack' do + buildpack1 = Buildpack.make(name: buildpack_name, stack: 'manifest-stack', key: 'new_key') + + update_job = BuildpackInstaller.new(buildpack_name, zipfile, { enabled: false }) + update_job.perform + + buildpack2 = Buildpack.find(name: buildpack_name) + expect(buildpack2).to_not be_nil + expect(buildpack2.enabled).to be false + expect(buildpack2.filename).to end_with(File.basename(zipfile)) + expect(buildpack2.key).to_not eql(buildpack1.key) + end + + it 'updates an existing buildpack with nil stack' do + buildpack1 = Buildpack.make(name: buildpack_name, stack: nil, key: 'new_key') + + update_job = BuildpackInstaller.new(buildpack_name, zipfile, { enabled: false }) + update_job.perform + + buildpack2 = Buildpack.find(name: buildpack_name) + expect(buildpack2).to_not be_nil + expect(buildpack2.enabled).to be false + expect(buildpack2.filename).to end_with(File.basename(zipfile)) + expect(buildpack2.key).to_not eql(buildpack1.key) + expect(buildpack2.stack).to eql('manifest-stack') + end + + it 'creates a new buildpack if existing buildpacks have different stacks' do + Stack.make(name: 'stack-1') + Stack.make(name: 'stack-2') + Buildpack.make(name: buildpack_name, stack: 'stack-1', filename: nil) + Buildpack.make(name: buildpack_name, stack: 'stack-2', filename: nil) + + update_job = BuildpackInstaller.new(buildpack_name, zipfile, { enabled: false }) + expect { + update_job.perform + }.to change { Buildpack.count }.by(1) + + buildpack1 = Buildpack.find(name: buildpack_name, stack: 'stack-1') + expect(buildpack1).to_not be_nil + expect(buildpack1.filename).to be_nil + + buildpack2 = Buildpack.find(name: buildpack_name, stack: 'stack-2') + expect(buildpack2).to_not be_nil + expect(buildpack2.filename).to be_nil + + buildpack3 = Buildpack.find(name: buildpack_name, stack: 'manifest-stack') + expect(buildpack3).to_not be_nil + expect(buildpack3.filename).to_not be_nil + end end end diff --git a/spec/unit/lib/cloud_controller/buildpacks/stack_name_extractor_spec.rb b/spec/unit/lib/cloud_controller/buildpacks/stack_name_extractor_spec.rb new file mode 100644 index 00000000000..bacecd22a97 --- /dev/null +++ b/spec/unit/lib/cloud_controller/buildpacks/stack_name_extractor_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +module VCAP::CloudController::Buildpacks + RSpec.describe StackNameExtractor do + let(:tmpdir) { Dir.mktmpdir } + let(:zip_path) { File.join(tmpdir, 'file.zip') } + + def zip_with_manifest_content(manifest_content) + TestZip.create(zip_path, 1, 1024) do |zipfile| + if manifest_content + zipfile.get_output_stream('manifest.yml') do |f| + f.write(manifest_content) + end + end + end + end + + describe '.extract_from_file' do + context 'buildpack zip is not a valid zip file' do + before do + File.write(zip_path, 'INVALID_FOR_A_ZIP_FILE_THIS_IS') + end + + it 'raises an error' do + expect { StackNameExtractor.extract_from_file(zip_path) }.to raise_error(CloudController::Errors::BuildpackError, 'buildpack zipfile is not valid') + end + end + + context 'buildpack zip contains a manifest specifying the stack' do + before do + zip_with_manifest_content("---\nstack: ITSaSTACK\n") + end + + it 'returns the stack in the manifest' do + expect(StackNameExtractor.extract_from_file(zip_path)).to eq('ITSaSTACK') + end + end + + context 'buildpack zip contains manifest that does not specify stack' do + before do + zip_with_manifest_content("---\nsomethingelse: NOTstackTHOUGH\n") + end + + it 'returns nil' do + expect(StackNameExtractor.extract_from_file(zip_path)).to be_nil + end + end + + context 'buildpack zip contains manifest which is not valid' do + before do + zip_with_manifest_content("\0xde\0xad\0xbe\0xef") + end + + it 'raises an error' do + expect { StackNameExtractor.extract_from_file(zip_path) }.to raise_error(CloudController::Errors::BuildpackError, 'buildpack manifest is not valid') + end + end + + context 'buildpack zip does not contain manifest' do + before do + TestZip.create(zip_path, 3, 1024) + end + + it 'returns nil' do + expect(StackNameExtractor.extract_from_file(zip_path)).to be_nil + end + end + + context 'buildpack zip contains manifest but it is too large (>1mb)' do + before do + alphachars = [*'A'..'Z'] + megabyte_string = (0...(1024 * 1024)).map { alphachars.sample }.join + zip_with_manifest_content("---\nstack: cflinuxfs2\nabsurdly_long_value: " + megabyte_string) + end + + it 'raises an error' do + expect { StackNameExtractor.extract_from_file(zip_path) }.to raise_error(CloudController::Errors::BuildpackError, 'buildpack manifest is too large') + end + end + end + end +end diff --git a/spec/unit/lib/cloud_controller/diego/buildpack/buildpack_entry_generator_spec.rb b/spec/unit/lib/cloud_controller/diego/buildpack/buildpack_entry_generator_spec.rb index 97d78d45582..58d6ba164b8 100644 --- a/spec/unit/lib/cloud_controller/diego/buildpack/buildpack_entry_generator_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/buildpack/buildpack_entry_generator_spec.rb @@ -12,12 +12,17 @@ module Buildpack let(:build_artifacts_cache_download_uri) { 'http://buildpack-artifacts-cache.example.com' } let(:blobstore_url_generator) { double('fake url generator') } + let(:stack) { Stack.make } + let(:stack2) { Stack.make } let!(:java_buildpack) do - VCAP::CloudController::Buildpack.create(name: 'java', key: 'java-buildpack-key', position: 1, sha256_checksum: 'checksum') + VCAP::CloudController::Buildpack.create(name: 'java', stack: stack.name, key: 'java-buildpack-key', position: 1, sha256_checksum: 'checksum') end let!(:ruby_buildpack) do - VCAP::CloudController::Buildpack.create(name: 'ruby', key: 'ruby-buildpack-key', position: 2, sha256_checksum: 'checksum') + VCAP::CloudController::Buildpack.create(name: 'ruby', stack: stack.name, key: 'ruby-buildpack-key', position: 2, sha256_checksum: 'checksum') + end + let!(:ruby_buildpack_other_stack) do + VCAP::CloudController::Buildpack.create(name: 'ruby', stack: stack2.name, key: 'ruby-buildpack-stack2-key', position: 3, sha256_checksum: 'checksum') end before do @@ -40,8 +45,8 @@ module Buildpack let(:admin_buildpack_info) { BuildpackInfo.new('java', VCAP::CloudController::Buildpack.find(name: 'java')) } let(:buildpack_infos) { [custom_buildpack_info, admin_buildpack_info] } - it 'returns both buildpacks' do - expect(buildpack_entry_generator.buildpack_entries(buildpack_infos)).to eq([ + it 'returns both buildpacks for the stack' do + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name)).to eq([ { name: 'custom', key: 'http://example.com/my_buildpack_url.zip', url: 'http://example.com/my_buildpack_url.zip', skip_detect: true }, { name: 'java', key: 'java-buildpack-key', url: admin_buildpack_download_url, skip_detect: true, sha256: 'checksum' }, ]) @@ -53,7 +58,7 @@ module Buildpack let(:buildpack) { 'http://example.com/my_buildpack_url.zip' } it "should use the buildpack_uri and name it 'custom', and use the url as the key" do - expect(buildpack_entry_generator.buildpack_entries(buildpack_infos)).to eq([ + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name)).to eq([ { name: 'custom', key: 'http://example.com/my_buildpack_url.zip', url: 'http://example.com/my_buildpack_url.zip', skip_detect: true } ]) end @@ -63,7 +68,7 @@ module Buildpack let(:buildpack) { 'http://example.com/my_buildpack_url' } it "should use the buildpack_uri and name it 'custom', and use the url as the key" do - expect(buildpack_entry_generator.buildpack_entries(buildpack_infos)).to eq([ + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name)).to eq([ { name: 'custom', key: 'http://example.com/my_buildpack_url', url: 'http://example.com/my_buildpack_url', skip_detect: true } ]) end @@ -74,7 +79,7 @@ module Buildpack let(:buildpack) { 'java' } it 'should use that buildpack' do - expect(buildpack_entry_generator.buildpack_entries(buildpack_infos)).to eq([ + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name)).to eq([ { name: 'java', key: 'java-buildpack-key', url: admin_buildpack_download_url, skip_detect: true, sha256: 'checksum' } ]) end @@ -85,7 +90,7 @@ module Buildpack end it 'fails fast with a clear error' do - expect { buildpack_entry_generator.buildpack_entries(buildpack_infos) }.to raise_error /Unsupported buildpack type/ + expect { buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name) }.to raise_error /Unsupported buildpack type/ end end end @@ -93,10 +98,18 @@ module Buildpack context 'when the user has not requested a buildpack' do let(:buildpack_infos) { [] } - it 'should use the list of admin buildpacks' do - expect(buildpack_entry_generator.buildpack_entries(buildpack_infos)).to eq([ + it 'should use the list of admin buildpacks for the specified stack' do + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name)).to eq([ + { name: 'java', key: 'java-buildpack-key', url: admin_buildpack_download_url, sha256: 'checksum', skip_detect: false }, + { name: 'ruby', key: 'ruby-buildpack-key', url: admin_buildpack_download_url, sha256: 'checksum', skip_detect: false }, + ]) + end + + it 'should use the list of all admin buildpacks if no stack is specified' do + expect(buildpack_entry_generator.buildpack_entries(buildpack_infos, nil)).to eq([ { name: 'java', key: 'java-buildpack-key', url: admin_buildpack_download_url, sha256: 'checksum', skip_detect: false }, { name: 'ruby', key: 'ruby-buildpack-key', url: admin_buildpack_download_url, sha256: 'checksum', skip_detect: false }, + { name: 'ruby', key: 'ruby-buildpack-stack2-key', url: admin_buildpack_download_url, sha256: 'checksum', skip_detect: false }, ]) end end @@ -105,7 +118,7 @@ module Buildpack let(:buildpack) { '???' } it 'fails fast with a clear error' do - expect { buildpack_entry_generator.buildpack_entries(buildpack_infos) }.to raise_error /Unsupported buildpack type/ + expect { buildpack_entry_generator.buildpack_entries(buildpack_infos, stack.name) }.to raise_error /Unsupported buildpack type/ end end end diff --git a/spec/unit/lib/cloud_controller/diego/buildpack/lifecycle_protocol_spec.rb b/spec/unit/lib/cloud_controller/diego/buildpack/lifecycle_protocol_spec.rb index 23d919b92cb..249e3357466 100644 --- a/spec/unit/lib/cloud_controller/diego/buildpack/lifecycle_protocol_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/buildpack/lifecycle_protocol_spec.rb @@ -41,7 +41,8 @@ module Buildpack end before do - VCAP::CloudController::Buildpack.create(name: 'ruby', key: 'ruby-buildpack-key', position: 2) + Stack.create(name: 'potato-stack') + VCAP::CloudController::Buildpack.create(name: 'ruby', stack: 'potato-stack', key: 'ruby-buildpack-key', position: 2) allow(blobstore_url_generator).to receive(:admin_buildpack_download_url).and_return('bp-download-url') end diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_spec.rb index 8a630acb1f4..2d0732faa59 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_spec.rb @@ -145,7 +145,7 @@ module VCAP::CloudController end end - context 'and the app does not have a stack' do + context 'when the app does not have a stack' do it 'uses the default value for stack' do expect(buildpack_lifecycle.staging_stack).to eq(Stack.default.name) end diff --git a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb index f8334202d4b..5cae8aa3fe2 100644 --- a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb +++ b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb @@ -3,7 +3,7 @@ module VCAP::CloudController RSpec.describe UploadBuildpack do let(:buildpack_blobstore) { double(:buildpack_blobstore).as_null_object } - let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', position: 0 }) } + let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', stack: 'cflinuxfs2', position: 0 }) } let(:upload_buildpack) { UploadBuildpack.new(buildpack_blobstore) } @@ -13,9 +13,16 @@ module VCAP::CloudController let(:sha_valid_zip) { Digester.new(algorithm: Digest::SHA256).digest_file(valid_zip) } let(:sha_valid_zip2) { Digester.new(algorithm: Digest::SHA256).digest_file(valid_zip2) } + let(:valid_zip_manifest_stack) { nil } let(:valid_zip) do zip_name = File.join(tmpdir, filename) - TestZip.create(zip_name, 1, 1024) + TestZip.create(zip_name, 1, 1024) do |zipfile| + if valid_zip_manifest_stack + zipfile.get_output_stream('manifest.yml') do |f| + f.write("---\nstack: #{valid_zip_manifest_stack}\n") + end + end + end zip_file = File.new(zip_name) Rack::Test::UploadedFile.new(zip_file) end @@ -37,6 +44,102 @@ module VCAP::CloudController end context 'and the upload to the blobstore succeeds' do + context 'stack from manifest' do + context 'manifest file is too large (>1mb)' do + let(:valid_zip_manifest_stack) { 'cflinuxfs2' } + let(:zip_with_massive_manifest) do + zip_name = File.join(tmpdir, filename) + TestZip.create(zip_name, 1, 1024) do |zipfile| + if valid_zip_manifest_stack + zipfile.get_output_stream('manifest.yml') do |f| + alphachars = [*'A'..'Z'] + megabyte_string = (0...(1024 * 1024)).map { alphachars.sample }.join + f.write("---\nstack: cflinuxfs2\nabsurdly_long_value: " + megabyte_string) + end + end + end + zip_file = File.new(zip_name) + Rack::Test::UploadedFile.new(zip_file) + end + + it 'returns an error and does not update stack' do + expect { + upload_buildpack.upload_buildpack(buildpack, zip_with_massive_manifest, filename) + }.to raise_error(CloudController::Errors::ApiError, /Buildpack zip error/) + bp = Buildpack.find(name: buildpack.name) + expect(bp).to_not be_nil + expect(bp.stack).to eq('cflinuxfs2') + end + end + + context 'same as buildpack' do + let(:valid_zip_manifest_stack) { 'cflinuxfs2' } + + it 'copies new bits to the blobstore' do + expect(buildpack_blobstore).to receive(:cp_to_blobstore).with(valid_zip, expected_sha_valid_zip) + + upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) + end + end + + context 'different from buildpack' do + let(:valid_zip_manifest_stack) { 'cflinuxfs3' } + before do + VCAP::CloudController::Stack.create(name: 'cflinuxfs3') + end + + it 'raises an error and does not update stack' do + expect { + upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) + }.to raise_error(CloudController::Errors::ApiError, /Uploaded buildpack stack \(cflinuxfs3\) does not match cflinuxfs2/) + bp = Buildpack.find(name: buildpack.name) + expect(bp).to_not be_nil + expect(bp.stack).to eq('cflinuxfs2') + end + end + + context 'stack previously unknown' do + let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', stack: nil, position: 0 }) } + context 'and the stack exists' do + let(:valid_zip_manifest_stack) { 'cflinuxfs3' } + before do + VCAP::CloudController::Stack.create(name: 'cflinuxfs3') + end + + it 'copies new bits to the blobstore and updates the stack' do + expect(buildpack_blobstore).to receive(:cp_to_blobstore).with(valid_zip, expected_sha_valid_zip) + + expect do + upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) + end.to change { buildpack.stack }.from(nil).to('cflinuxfs3') + end + + context 'buildpack with same name and stack exists' do + let(:valid_zip_manifest_stack) { 'cflinuxfs3' } + + it 'raises an error' do + VCAP::CloudController::Buildpack.create(name: buildpack.name, stack: 'cflinuxfs3') + expect { + upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) + }.to raise_error(CloudController::Errors::ApiError, /The buildpack name #{buildpack.name} is already in use for the stack #{valid_zip_manifest_stack}/) + end + end + end + + context "but the stack doesn't exist" do + let(:valid_zip_manifest_stack) { 'new-and-unknown' } + it 'raises an error' do + expect { + upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) + }.to raise_error(CloudController::Errors::ApiError, /Uploaded buildpack stack \(#{valid_zip_manifest_stack}\) does not exist/) + bp = Buildpack.find(name: buildpack.name) + expect(bp).to_not be_nil + expect(bp.filename).to be_nil + end + end + end + end + it 'updates the buildpack filename' do expect { upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) @@ -116,7 +219,7 @@ module VCAP::CloudController end context 'when the same bits are uploaded twice' do - let(:buildpack2) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'buildpack2', position: 0 }) } + let(:buildpack2) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'buildpack2', stack: 'cflinuxfs2', position: 0 }) } it 'should have different keys' do upload_buildpack.upload_buildpack(buildpack, valid_zip2, filename) diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index f0f5bce0948..8f03297326a 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -9,7 +9,32 @@ def ordered_buildpacks it { is_expected.to have_timestamp_columns } describe 'Validations' do - it { is_expected.to validate_uniqueness :name } + it { is_expected.to validate_uniqueness [:name, :stack] } + + describe 'stack' do + it 'can be changed if not set' do + buildpack = Buildpack.create(name: 'test', stack: nil) + buildpack.stack = Stack.make.name + + expect(buildpack).to be_valid + end + + it 'cannot be changed once it is set' do + buildpack = Buildpack.create(name: 'test', stack: Stack.make.name) + buildpack.stack = Stack.make.name + + expect(buildpack).not_to be_valid + expect(buildpack.errors.on(:stack)).to include(:buildpack_cant_change_stacks) + end + + it 'cannot be changed to a stack that doesn\'t exist' do + buildpack = Buildpack.create(name: 'test', stack: nil) + buildpack.stack = 'this-stack-isnt-real' + + expect(buildpack).not_to be_valid + expect(buildpack.errors.on(:stack)).to include(:buildpack_stack_does_not_exist) + end + end describe 'name' do it 'does not allow non-word non-dash characters' do @@ -30,8 +55,8 @@ def ordered_buildpacks end describe 'Serialization' do - it { is_expected.to export_attributes :name, :position, :enabled, :locked, :filename } - it { is_expected.to import_attributes :name, :position, :enabled, :locked, :filename, :key } + it { is_expected.to export_attributes :name, :stack, :position, :enabled, :locked, :filename } + it { is_expected.to import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key } it 'does not string mung(e)?' do expect(Buildpack.new(name: "my_custom_buildpack\r\n").to_json).to eq '"my_custom_buildpack\r\n"' @@ -92,7 +117,7 @@ def ordered_buildpacks end context 'and there are buildpacks with null keys' do - let!(:null_buildpack) { Buildpack.create(name: 'nil_key_custom_buildpack', position: 0) } + let!(:null_buildpack) { Buildpack.create(name: 'nil_key_custom_buildpack', stack: Stack.make.name, position: 0) } it 'only returns buildpacks with non-null keys' do expect(Buildpack.all).to include(null_buildpack) @@ -102,7 +127,7 @@ def ordered_buildpacks end context 'and there are buildpacks with empty keys' do - let!(:empty_buildpack) { Buildpack.create(name: 'nil_key_custom_buildpack', key: '', position: 0) } + let!(:empty_buildpack) { Buildpack.create(name: 'nil_key_custom_buildpack', stack: Stack.make.name, key: '', position: 0) } it 'only returns buildpacks with non-null keys' do expect(Buildpack.all).to include(empty_buildpack) @@ -126,6 +151,29 @@ def ordered_buildpacks expect(all_buildpacks).to match_array([enabled_buildpack, disabled_buildpack]) end end + + context 'with a stack' do + subject(:all_buildpacks) { Buildpack.list_admin_buildpacks('stack1') } + let!(:stack1) { Stack.make(name: 'stack1') } + let!(:stack2) { Stack.make(name: 'stack2') } + + before do + buildpack_blobstore.cp_to_blobstore(buildpack_file_1.path, 'a key') + Buildpack.make(key: 'a key', position: 2, stack: 'stack1') + + buildpack_blobstore.cp_to_blobstore(buildpack_file_2.path, 'b key') + Buildpack.make(key: 'b key', position: 1, stack: 'stack2') + + buildpack_blobstore.cp_to_blobstore(buildpack_file_3.path, 'c key') + @another_buildpack = Buildpack.make(key: 'c key', position: 3, stack: nil) + end + + it { is_expected.to have(2).items } + + it 'returns the list in position order, including buildpacks with null stack or matching stacks' do + expect(all_buildpacks.collect(&:key)).to eq(['a key', 'c key']) + end + end end describe 'staging_message' do @@ -137,7 +185,7 @@ def ordered_buildpacks describe '#update' do let!(:buildpacks) do - Array.new(4) { |i| Buildpack.create(name: "name_#{100 - i}", position: i + 1) } + Array.new(4) { |i| Buildpack.create(name: "name_#{100 - i}", stack: Stack.make.name, position: i + 1) } end it 'does not modify the frozen hash provided by Sequel' do @@ -148,8 +196,8 @@ def ordered_buildpacks end describe '#destroy' do - let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', key: 'xyz', position: 1 }) } - let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', key: 'xyz', position: 2 }) } + let!(:buildpack1) { VCAP::CloudController::Buildpack.create({ name: 'first_buildpack', stack: Stack.make.name, key: 'xyz', position: 1 }) } + let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', stack: Stack.make.name, key: 'xyz', position: 2 }) } it 'removes the specified buildpack' do expect { diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index 7ff6e7b2200..d86d514829c 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -858,6 +858,11 @@ http_code: 502 message: "Service broker returned dashboard client configuration without a dashboard URL" +290000: + name: BuildpackNameStackTaken + http_code: 422 + message: "The buildpack name %s is already in use for the stack %s" + 290001: name: BuildpackNameTaken http_code: 400 @@ -1148,3 +1153,18 @@ name: SharedServiceInstanceNotDeletableInTargetSpace http_code: 403 message: 'You cannot delete service instances that have been shared with you' + +390011: + name: BuildpackStacksDontMatch + http_code: 422 + message: 'Uploaded buildpack stack (%s) does not match %s' + +390012: + name: BuildpackStackDoesNotExist + http_code: 422 + message: 'Uploaded buildpack stack (%s) does not exist' + +390013: + name: BuildpackZipError + http_code: 422 + message: 'Buildpack zip error: %s'