From da7220b7c4e40fd4a8363e7839e8820d31557e44 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] - Add "stack" to buildpack model - At migration time, stacks.yml will be read (location from STACKS_YML env var) to determine what stack should be assigned to existing buildpacks - Buildpacks are now unique over name AND stack - Sets buildpack stack from manifest.yml in buildpack zip on creation - Validate buildpack model stack against stack in buildpack zip manifest.yml - Validate stack exists upon buildpack bits upload - Include stack name in serialized buildpack filename - Only provide buildpacks for the relevant stack to the staging container - Handle buildpack stacks appropriately in the buildpack installer NOTE: The API checkshum has changed due to adding stack as an input Signed-off-by: Dave Goddard Signed-off-by: Victoria Henry Signed-off-by: Jackson Feeny Signed-off-by: Sam Coward Signed-off-by: Dave Goddard Signed-off-by: Sam Coward --- .../runtime/buildpacks_controller.rb | 7 +- app/fetchers/buildpack_lifecycle_fetcher.rb | 11 +- app/jobs/runtime/buildpack_installer.rb | 22 +++- app/models/runtime/buildpack.rb | 13 +- app/presenters/v2/buildpack_presenter.rb | 17 +++ app/presenters/v2/presenter_provider.rb | 1 + ...0102183100_add_stack_to_buildpack_table.rb | 30 +++++ .../buildpack/buildpack_entry_generator.rb | 8 +- .../diego/buildpack/lifecycle_protocol.rb | 13 +- .../diego/lifecycles/buildpack_lifecycle.rb | 9 +- lib/cloud_controller/upload_buildpack.rb | 26 ++++ spec/api/api_version_spec.rb | 2 +- spec/api/documentation/buildpacks_api_spec.rb | 3 +- spec/request/apps_spec.rb | 7 +- spec/support/fakes/blueprints.rb | 2 + spec/support/test_zip.rb | 21 +-- .../runtime/buildpack_bits_controller_spec.rb | 117 +++++++++++++---- .../runtime/buildpacks_controller_spec.rb | 40 ++++-- .../controllers/v3/builds_controller_spec.rb | 11 +- .../jobs/runtime/buildpack_delete_spec.rb | 2 +- .../jobs/runtime/buildpack_installer_spec.rb | 124 +++++++++++++++--- .../buildpack_entry_generator_spec.rb | 33 +++-- .../buildpack/lifecycle_protocol_spec.rb | 2 +- .../cloud_controller/upload_buildpack_spec.rb | 76 ++++++++++- spec/unit/models/runtime/buildpack_spec.rb | 20 +-- .../presenters/v2/buildpack_presenter_spec.rb | 42 ++++++ vendor/errors/v2.yml | 16 ++- 27 files changed, 545 insertions(+), 130 deletions(-) create mode 100644 app/presenters/v2/buildpack_presenter.rb create mode 100644 db/migrations/20180102183100_add_stack_to_buildpack_table.rb create mode 100644 spec/unit/presenters/v2/buildpack_presenter_spec.rb diff --git a/app/controllers/runtime/buildpacks_controller.rb b/app/controllers/runtime/buildpacks_controller.rb index 88f56f4d457..ee134f33254 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: 'unknown' attribute :position, Integer, default: 0 attribute :enabled, Message::Boolean, default: true attribute :locked, Message::Boolean, default: false end - query_parameters :name + query_parameters :name, :stack def self.translate_validation_exception(e, attributes) - buildpack_errors = e.errors.on(:name) + buildpack_errors = e.errors.on([:name, :stack]) if buildpack_errors && buildpack_errors.include?(:unique) - CloudController::Errors::ApiError.new_from_details('BuildpackNameTaken', attributes['name']) + CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', attributes['name'], attributes['stack']) else CloudController::Errors::ApiError.new_from_details('BuildpackInvalid', e.errors.full_messages) end diff --git a/app/fetchers/buildpack_lifecycle_fetcher.rb b/app/fetchers/buildpack_lifecycle_fetcher.rb index a8ef6224a4c..ab0a83ee4b3 100644 --- a/app/fetchers/buildpack_lifecycle_fetcher.rb +++ b/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 } diff --git a/app/jobs/runtime/buildpack_installer.rb b/app/jobs/runtime/buildpack_installer.rb index cfac8f1c698..2f50d662ab8 100644 --- a/app/jobs/runtime/buildpack_installer.rb +++ b/app/jobs/runtime/buildpack_installer.rb @@ -14,9 +14,9 @@ def perform logger = Steno.logger('cc.background') logger.info "Installing buildpack #{name}" - buildpack = Buildpack.find(name: name) + buildpack = find_existing_buildpack if buildpack.nil? - buildpack = Buildpack.create(name: name) + buildpack = Buildpack.create(name: name, stack: 'unknown') created = true elsif buildpack.locked logger.info "Buildpack #{name} locked, not updated" @@ -34,6 +34,8 @@ def perform buildpack.update(opts) logger.info "Buildpack #{name} installed or updated" + rescue AmbiguousBuildpackException => abe + logger.error abe.message rescue => e logger.error("Buildpack #{name} failed to install or update. Error: #{e.inspect}") raise e @@ -51,6 +53,22 @@ def buildpack_uploader buildpack_blobstore = CloudController::DependencyLocator.instance.buildpack_blobstore UploadBuildpack.new(buildpack_blobstore) end + + private + + def find_existing_buildpack + stack = buildpack_uploader.extract_stack_from_buildpack(file) + return Buildpack.find(name: name, stack: stack) if stack.to_s != '' + + buildpacks = Buildpack.where(name: name) + if buildpacks.count > 1 + raise AmbiguousBuildpackException.new("Buildpack #{name} has #{buildpacks.count} stacks, not updated") + end + buildpacks.first + end + + class AmbiguousBuildpackException < RuntimeError + end end end end diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 6ab63186a41..ee800ca1579 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -2,11 +2,13 @@ module VCAP::CloudController class Buildpack < Sequel::Model plugin :list - export_attributes :name, :position, :enabled, :locked, :filename - import_attributes :name, :position, :enabled, :locked, :filename, :key + export_attributes :name, :stack, :position, :enabled, :locked, :filename + import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key - def self.list_admin_buildpacks - exclude(key: nil).exclude(key: '').order(:position).all + def self.list_admin_buildpacks(stack_name=nil) + scoped = exclude(key: nil).exclude(key: '') + scoped = scoped.where(stack: stack_name) if stack_name + scoped.order(:position).all end def self.at_last_position @@ -18,8 +20,9 @@ def self.user_visibility_filter(user) end def validate - validates_unique :name + validates_unique [:name, :stack] validates_format(/\A(\w|\-)+\z/, :name, message: 'name can only contain alphanumeric characters') + validates_presence :stack end def locked? diff --git a/app/presenters/v2/buildpack_presenter.rb b/app/presenters/v2/buildpack_presenter.rb new file mode 100644 index 00000000000..01a347c4eaf --- /dev/null +++ b/app/presenters/v2/buildpack_presenter.rb @@ -0,0 +1,17 @@ +module CloudController + module Presenters + module V2 + class BuildpackPresenter < DefaultPresenter + extend PresenterProvider + + present_for_class 'VCAP::CloudController::Buildpack' + + def entity_hash(controller, buildpack, opts, depth, parents, orphans=nil) + entity = super + entity['filename'] = "#{buildpack.filename} (#{buildpack.stack})".strip + entity + end + end + end + end +end diff --git a/app/presenters/v2/presenter_provider.rb b/app/presenters/v2/presenter_provider.rb index f0476df89e4..7d569085245 100644 --- a/app/presenters/v2/presenter_provider.rb +++ b/app/presenters/v2/presenter_provider.rb @@ -33,3 +33,4 @@ def self.presenters require_relative 'space_presenter' require_relative 'organization_presenter' require_relative 'service_plan_presenter' +require_relative 'buildpack_presenter' 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..82b918df636 --- /dev/null +++ b/db/migrations/20180102183100_add_stack_to_buildpack_table.rb @@ -0,0 +1,30 @@ +require 'yaml' + +def default_stack + stacks_yml_path = ENV.fetch('STACKS_YML', nil) + YAML.safe_load(File.read(stacks_yml_path))['default'] if stacks_yml_path && File.exist?(stacks_yml_path) +end + +Sequel.migration do + up do + alter_table(:buildpacks) do + add_column :stack, String, size: 255, null: true + drop_index :name, unique: true + add_index [:name, :stack], unique: true, name: :unique_name_and_stack + end + + self['UPDATE buildpacks SET stack = ?', default_stack || 'unknown'].update + + alter_table(:buildpacks) do + set_column_not_null :stack + end + end + + down do + alter_table(:buildpacks) do + drop_index [:name, :stack], unique: true + drop_column :stack + add_index :name, unique: true, name: :unique_name + end + end +end 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..8b76c29cb5a 100644 --- a/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb +++ b/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 diff --git a/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle.rb b/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle.rb index 33e543a38eb..576d9174fc2 100644 --- a/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle.rb +++ b/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle.rb @@ -36,7 +36,7 @@ def staging_environment_variables end def staging_stack - requested_stack || app_stack || VCAP::CloudController::Stack.default.name + requested_stack || app_stack || buildpack_stack || VCAP::CloudController::Stack.default.name end private @@ -57,6 +57,13 @@ def app_stack @package.app.buildpack_lifecycle_data.try(:stack) end + def buildpack_stack + stacks = Buildpack.where(name: buildpacks_to_use).select(:stack).uniq + if stacks.length == 1 + stacks.first + end + end + attr_reader :validator end end diff --git a/lib/cloud_controller/upload_buildpack.rb b/lib/cloud_controller/upload_buildpack.rb index 3b5865cefb2..ab3d5669497 100644 --- a/lib/cloud_controller/upload_buildpack.rb +++ b/lib/cloud_controller/upload_buildpack.rb @@ -24,6 +24,8 @@ def upload_buildpack(buildpack, bits_file_path, new_filename) old_buildpack_key = nil + new_stack = determine_new_stack(buildpack, bits_file_path) + begin Buildpack.db.transaction do buildpack.lock! @@ -32,6 +34,7 @@ def upload_buildpack(buildpack, bits_file_path, new_filename) key: new_key, filename: new_filename, sha256_checksum: sha256, + stack: new_stack, ) end rescue Sequel::Error @@ -47,8 +50,31 @@ def upload_buildpack(buildpack, bits_file_path, new_filename) true end + def extract_stack_from_buildpack(bits_file_path) + bits_file_path = bits_file_path.path if bits_file_path.respond_to?(:path) + output, _, status = Open3.capture3('unzip', '-p', bits_file_path, 'manifest.yml') + YAML.safe_load(output).dig('stack') if status.success? + end + private + def determine_new_stack(buildpack, bits_file_path) + extracted_stack = extract_stack_from_buildpack(bits_file_path) + if extracted_stack.to_s != '' && !Stack.where(name: extracted_stack).first + raise CloudController::Errors::ApiError.new_from_details('BuildpackStackDoesNotExist', extracted_stack) + end + new_stack = [extracted_stack, buildpack.stack, Stack.default.name].find { |s| s.to_s != '' && s.to_s != 'unknown' } + if buildpack.stack != 'unknown' && buildpack.stack != new_stack + raise CloudController::Errors::ApiError.new_from_details('BuildpackStacksDontMatch', new_stack, buildpack.stack) + end + + if buildpack.stack != new_stack && Buildpack.find(name: buildpack.name, stack: new_stack) + raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', buildpack.name, new_stack) + end + + new_stack + end + def new_bits?(buildpack, key) buildpack.key != key end diff --git a/spec/api/api_version_spec.rb b/spec/api/api_version_spec.rb index a7dc004fd79..9c6128464be 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 = '8d08df242e77b29ef0374b7557031985fcdfba43'.freeze + API_FOLDER_CHECKSUM = '5e3e3c64074e05882c6748b578a9a62f80694c94'.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/request/apps_spec.rb b/spec/request/apps_spec.rb index 62d6b5e1696..35ca3f62f5d 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 78d2c49052d..29a32088783 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 @@ -395,6 +396,7 @@ module VCAP::CloudController Buildpack.blueprint do name { Sham.name } + stack { Sham.stack } key { Sham.guid } position { 0 } enabled { true } 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/controllers/runtime/buildpack_bits_controller_spec.rb b/spec/unit/controllers/runtime/buildpack_bits_controller_spec.rb index 348769818ba..446ca52094c 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) @@ -29,10 +49,7 @@ module VCAP::CloudController let(:expected_sha_valid_zip) { "#{@buildpack.guid}_#{sha_valid_zip}" } before do - @file = double(:file, { - public_url: 'https://some-bucket.example.com/ab/cd/abcdefg', - key: '123-456', - }) + @file = double(:file, { public_url: 'https://some-bucket.example.com/ab/cd/abcdefg', key: '123-456' }) buildpack_blobstore = CloudController::DependencyLocator.instance.buildpack_blobstore allow(buildpack_blobstore).to receive(:files).and_return(double(:files, head: @file, create: {})) @@ -42,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: 'stack', position: 0 }) } before { CloudController::DependencyLocator.instance.register(:upload_handler, UploadHandler.new(TestConfig.config_instance)) } @@ -51,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 } @@ -75,20 +95,65 @@ module VCAP::CloudController 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 + test_buildpack.update(stack: 'unknown') + + 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 + test_buildpack.update(stack: 'unknown') + + 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 eq('unknown') + end + + it 'sets the buildpack stack to default if it is unset and NOT in buildpack manifest' do + test_buildpack.update(stack: 'unknown') + + 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 + 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 @@ -126,19 +191,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} (stack)") 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: 'unknown', 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: 'unknown', position: 0 }) + put "/v2/buildpacks/#{new_buildpack.guid}/bits", { buildpack: valid_zip_manifest } + + expect(last_response.status).to eq(409) + 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') @@ -155,13 +230,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 } @@ -177,7 +252,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 } @@ -196,20 +271,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: 'stack', 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 0cba02f8df5..6f0b46dcd1a 100644 --- a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb @@ -7,7 +7,7 @@ def ordered_buildpacks end let(:user) { make_user } - let(:req_body) { MultiJson.dump({ name: 'dynamic_test_buildpack', position: 1 }) } + let(:req_body) { MultiJson.dump({ name: 'dynamic_test_buildpack', stack: 'stack', position: 1 }) } before { set_current_user_as_admin } @@ -19,6 +19,7 @@ def ordered_buildpacks it do expect(VCAP::CloudController::BuildpacksController).to have_creatable_attributes({ name: { type: 'string', required: true }, + stack: { type: 'string', default: 'unknown' }, position: { type: 'integer', default: 0 }, enabled: { type: 'bool', default: true }, locked: { type: 'bool', default: false } @@ -28,6 +29,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 +61,30 @@ def ordered_buildpacks expect(buildpack.position).to eq(1) end + it 'defaults stack to unknown when 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 eq('unknown') + end + + it 'uses specified stack' do + expect do + post '/v2/buildpacks', MultiJson.dump({ name: 'a_buildpack', stack: 'cflinuxfs99', 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('cflinuxfs99') + 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', position: 1) + Buildpack.create(name: 'pre-existing-buildpack-2', stack: 'stack', position: 2) expect { - post '/v2/buildpacks', MultiJson.dump({ name: 'new-buildpack', position: 2 }) + post '/v2/buildpacks', MultiJson.dump({ name: 'new-buildpack', stack: 'stack', position: 2 }) }.to change { ordered_buildpacks }.from( [['pre-existing-buildpack', 1], ['pre-existing-buildpack-2', 2]] ).to( @@ -73,15 +93,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') post '/v2/buildpacks', req_body - expect(last_response.status).to eq(400) + expect(last_response.status).to eq(409) expect(decoded_response['code']).to eq(290001) 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' }) expect(last_response.status).to eq(400) expect(decoded_response['code']).to eq(290003) expect(Buildpack.count).to eq(0) @@ -97,8 +117,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', key: 'xyz', filename: 'a', position: 1 }) } + let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', stack: 'stack', key: 'xyz', filename: 'b', position: 2 }) } it 'can update the buildpack name' do set_current_user_as_admin @@ -127,7 +147,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', 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 d56e6951d07..5f7afda498b 100644 --- a/spec/unit/controllers/v3/builds_controller_spec.rb +++ b/spec/unit/controllers/v3/builds_controller_spec.rb @@ -4,6 +4,7 @@ describe '#create' do 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( @@ -25,7 +26,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 @@ -98,14 +99,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 @@ -166,7 +167,7 @@ type: 'buildpack', data: { buildpacks: [], - stack: 'cflinuxfs2' + stack: stack }, } end @@ -185,7 +186,7 @@ type: 'buildpack', data: { buildpacks: nil, - stack: 'cflinuxfs2' + stack: stack }, } 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..5b31085352b 100644 --- a/spec/unit/jobs/runtime/buildpack_installer_spec.rb +++ b/spec/unit/jobs/runtime/buildpack_installer_spec.rb @@ -20,30 +20,114 @@ 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 + 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) } + + 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 + + it 'updates an existing buildpack' do + buildpack1 = Buildpack.make(name: buildpack_name, stack: 'manifest-stack', key: 'new_key') - update_job = BuildpackInstaller.new(buildpack_name, zipfile2, { enabled: false }) - update_job.perform + 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(zipfile2)) - expect(buildpack2.key).to_not eql(buildpack1.key) + 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 'creates a new buildpack if existing buildpacks have different stacks' do + 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/diego/buildpack/buildpack_entry_generator_spec.rb b/spec/unit/lib/cloud_controller/diego/buildpack/buildpack_entry_generator_spec.rb index 97d78d45582..c925a50ffad 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 @@ -14,10 +14,13 @@ module Buildpack let(:blobstore_url_generator) { double('fake url generator') } 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', 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', key: 'ruby-buildpack-key', position: 2, sha256_checksum: 'checksum') + end + let!(:ruby_buildpack_other_stack) do + VCAP::CloudController::Buildpack.create(name: 'ruby', stack: 'stack2', key: 'ruby-buildpack-stack2-key', position: 3, sha256_checksum: 'checksum') end before do @@ -40,8 +43,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')).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 +56,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')).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 +66,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')).to eq([ { name: 'custom', key: 'http://example.com/my_buildpack_url', url: 'http://example.com/my_buildpack_url', skip_detect: true } ]) end @@ -74,7 +77,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')).to eq([ { name: 'java', key: 'java-buildpack-key', url: admin_buildpack_download_url, skip_detect: true, sha256: 'checksum' } ]) end @@ -85,7 +88,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') }.to raise_error /Unsupported buildpack type/ end end end @@ -93,10 +96,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')).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 +116,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') }.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..a5ddffed0a2 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,7 @@ module Buildpack end before do - VCAP::CloudController::Buildpack.create(name: 'ruby', key: 'ruby-buildpack-key', position: 2) + 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/upload_buildpack_spec.rb b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb index f8334202d4b..dbbbcaa02eb 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,69 @@ module VCAP::CloudController end context 'and the upload to the blobstore succeeds' do + context 'stack from manifest' do + 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) + 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: 'unknown', position: 0 }) } + context 'known' 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('unknown').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) + end + end + end + + context 'unknown' 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) + 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 +186,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..f1352f3617c 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -9,12 +9,12 @@ 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 'name' do it 'does not allow non-word non-dash characters' do ['git://github.com', '$abc', 'foobar!'].each do |name| - buildpack = Buildpack.new(name: name) + buildpack = Buildpack.new(name: name, stack: 'unknown') expect(buildpack).not_to be_valid expect(buildpack.errors.on(:name)).to be_present end @@ -22,7 +22,7 @@ def ordered_buildpacks it 'allows word and dash characters' do ['name', 'name-with-dash', '-name-'].each do |name| - buildpack = Buildpack.new(name: name) + buildpack = Buildpack.new(name: name, stack: 'unknown') expect(buildpack).to be_valid end end @@ -30,8 +30,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 +92,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', position: 0) } it 'only returns buildpacks with non-null keys' do expect(Buildpack.all).to include(null_buildpack) @@ -102,7 +102,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', key: '', position: 0) } it 'only returns buildpacks with non-null keys' do expect(Buildpack.all).to include(empty_buildpack) @@ -137,7 +137,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', position: i + 1) } end it 'does not modify the frozen hash provided by Sequel' do @@ -148,8 +148,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', key: 'xyz', position: 1 }) } + let!(:buildpack2) { VCAP::CloudController::Buildpack.create({ name: 'second_buildpack', stack: 'stack', key: 'xyz', position: 2 }) } it 'removes the specified buildpack' do expect { diff --git a/spec/unit/presenters/v2/buildpack_presenter_spec.rb b/spec/unit/presenters/v2/buildpack_presenter_spec.rb new file mode 100644 index 00000000000..13013d5cc37 --- /dev/null +++ b/spec/unit/presenters/v2/buildpack_presenter_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' + +module CloudController::Presenters::V2 + RSpec.describe BuildpackPresenter do + let(:buildpack_presenter) { BuildpackPresenter.new } + let(:controller) { 'controller' } + let(:opts) { {} } + let(:depth) { 0 } + let(:parents) { 'parents' } + let(:orphans) { 'orphans' } + let(:relations_presenter) { instance_double(RelationsPresenter, to_hash: relations_hash) } + let(:relations_hash) { {} } + before do + allow(RelationsPresenter).to receive(:new).and_return(relations_presenter) + end + + describe '#entity_hash' do + let(:buildpack) { VCAP::CloudController::Buildpack.make } + + it 'returns the space entity and associated urls' do + expected_entity_hash = { + 'name' => buildpack.name, + 'stack' => buildpack.stack, + 'enabled' => buildpack.enabled, + 'locked' => buildpack.locked, + 'position' => buildpack.position, + 'filename' => "#{buildpack.filename} (#{buildpack.stack})", + } + + actual_entity_hash = buildpack_presenter.entity_hash(controller, buildpack, opts, depth, parents, orphans) + + expect(actual_entity_hash).to be_a_response_like(expected_entity_hash) + end + + it 'formats correctly when no filename is present' do + buildpack.filename = '' + actual_entity_hash = buildpack_presenter.entity_hash(controller, buildpack, opts, depth, parents, orphans) + expect(actual_entity_hash['filename']).to eq("(#{buildpack.stack})") + end + end + end +end diff --git a/vendor/errors/v2.yml b/vendor/errors/v2.yml index df6a87cc311..6f3fbe7bb26 100644 --- a/vendor/errors/v2.yml +++ b/vendor/errors/v2.yml @@ -839,9 +839,9 @@ message: "Service broker returned dashboard client configuration without a dashboard URL" 290001: - name: BuildpackNameTaken - http_code: 400 - message: "The buildpack name is already in use: %s" + name: BuildpackNameStackTaken + http_code: 409 + message: "The buildpack name %s is already in use for the stack %s" 290002: name: BuildpackBitsUploadInvalid @@ -1153,3 +1153,13 @@ name: SharedServiceInstanceNotDeleteableInTargetSpace 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'