From d3cb1a277b3753f90423c80b05163868d9fef2d1 Mon Sep 17 00:00:00 2001 From: Nicolas Bender Date: Wed, 10 Jul 2024 16:30:39 +0200 Subject: [PATCH 01/20] Add lifecycle column to the buildpacks table Co-authored-by: Pavel Busko --- app/actions/buildpack_create.rb | 1 + app/messages/buildpack_create_message.rb | 8 +++++++- app/presenters/v3/buildpack_presenter.rb | 1 + db/migrations/20240710152700_cnb_system_buildpacks.rb | 11 +++++++++++ 4 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 db/migrations/20240710152700_cnb_system_buildpacks.rb diff --git a/app/actions/buildpack_create.rb b/app/actions/buildpack_create.rb index fd8cc1a8d6f..1dee200dd7f 100644 --- a/app/actions/buildpack_create.rb +++ b/app/actions/buildpack_create.rb @@ -14,6 +14,7 @@ def create(message) buildpack = Buildpack.create( name: message.name, stack: message.stack, + lifecycle: (message.lifecycle.nil? ? VCAP::CloudController::Config.config.get(:default_app_lifecycle) : message.lifecycle), enabled: (message.enabled.nil? ? DEFAULT_ENABLED : message.enabled), locked: (message.locked.nil? ? DEFAULT_LOCKED : message.locked) ) diff --git a/app/messages/buildpack_create_message.rb b/app/messages/buildpack_create_message.rb index b935ef2075d..9838c57ae33 100644 --- a/app/messages/buildpack_create_message.rb +++ b/app/messages/buildpack_create_message.rb @@ -1,12 +1,13 @@ require 'messages/metadata_base_message' require 'messages/validators' +require 'cloud_controller/diego/lifecycles/lifecycles' module VCAP::CloudController class BuildpackCreateMessage < MetadataBaseMessage MAX_BUILDPACK_NAME_LENGTH = 250 MAX_STACK_LENGTH = 250 - register_allowed_keys %i[name stack position enabled locked] + register_allowed_keys %i[name stack position enabled locked lifecycle] validates_with NoAdditionalKeysValidator validates :name, @@ -32,5 +33,10 @@ class BuildpackCreateMessage < MetadataBaseMessage validates :locked, allow_nil: true, boolean: true + + validates :lifecycle, + string: true, + allow_nil: true, + inclusion: { in: [VCAP::CloudController::Lifecycles::BUILDPACK, VCAP::CloudController::Lifecycles::CNB], message: 'must be either "buildpack" or "cnb"' } end end diff --git a/app/presenters/v3/buildpack_presenter.rb b/app/presenters/v3/buildpack_presenter.rb index bd084815b5b..ec32547b700 100644 --- a/app/presenters/v3/buildpack_presenter.rb +++ b/app/presenters/v3/buildpack_presenter.rb @@ -13,6 +13,7 @@ def to_hash name: buildpack.name, stack: buildpack.stack, state: buildpack.state, + lifecycle: buildpack.lifecycle, filename: buildpack.filename, position: buildpack.position, enabled: buildpack.enabled, diff --git a/db/migrations/20240710152700_cnb_system_buildpacks.rb b/db/migrations/20240710152700_cnb_system_buildpacks.rb new file mode 100644 index 00000000000..de803d63c5c --- /dev/null +++ b/db/migrations/20240710152700_cnb_system_buildpacks.rb @@ -0,0 +1,11 @@ +require 'cloud_controller/diego/lifecycles/lifecycles' + +Sequel.migration do + up do + add_column :buildpacks, :lifecycle, String, size: 16, if_not_exists: true, default: VCAP::CloudController::Lifecycles::BUILDPACK + end + + down do + drop_column :buildpacks, :lifecycle, if_exists: true + end +end From df16e8d01799f274d5922ac205e7f22af696ac16 Mon Sep 17 00:00:00 2001 From: Ralf Pannemans Date: Fri, 12 Jul 2024 17:24:33 +0200 Subject: [PATCH 02/20] WIP: Restructured buildpack model Co-authored-by: Nicolas Bender --- app/models/runtime/buildpack.rb | 20 +++++--------------- app/models/runtime/classic_buildpack.rb | 20 ++++++++++++++++++++ app/models/runtime/cnb_buildpack.rb | 20 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 15 deletions(-) create mode 100644 app/models/runtime/classic_buildpack.rb create mode 100644 app/models/runtime/cnb_buildpack.rb diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 2e99a2e366b..e02f0726120 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -1,6 +1,11 @@ module VCAP::CloudController class Buildpack < Sequel::Model plugin :list + plugin :single_table_inheritance, :lifecycle, + model_map: { + VCAP::CloudController::Lifecycles::BUILDPACK => "VCAP::CloudController::ClassicBuildpack", + VCAP::CloudController::Lifecycles::CNB => "VCAP::CloudController::CNBBuildpack" + } export_attributes :name, :stack, :position, :enabled, :locked, :filename import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key @@ -16,21 +21,6 @@ class Buildpack < Sequel::Model add_association_dependencies labels: :destroy add_association_dependencies annotations: :destroy - def self.list_admin_buildpacks(stack_name=nil) - scoped = exclude(key: nil).exclude(key: '') - if stack_name.present? - scoped = scoped.filter(Sequel.or([ - [:stack, stack_name], - [:stack, nil] - ])) - end - scoped.order(:position).all - end - - def self.at_last_position - where(position: max(:position)).first - end - def self.user_visibility_filter(_user) full_dataset_filter end diff --git a/app/models/runtime/classic_buildpack.rb b/app/models/runtime/classic_buildpack.rb new file mode 100644 index 00000000000..9171199b226 --- /dev/null +++ b/app/models/runtime/classic_buildpack.rb @@ -0,0 +1,20 @@ +module VCAP::CloudController + class ClassicBuildpack < Buildpack + + def self.list_admin_buildpacks(stack_name=nil) + scoped = exclude(key: nil).exclude(key: '') + if stack_name.present? + scoped = scoped.filter(Sequel.or([ + [:stack, stack_name], + [:stack, nil] + ])) + end + scoped.order(:position).all + end + + def self.at_last_position + where(position: max(:position)).first + end + + end +end diff --git a/app/models/runtime/cnb_buildpack.rb b/app/models/runtime/cnb_buildpack.rb new file mode 100644 index 00000000000..a275b0db1ec --- /dev/null +++ b/app/models/runtime/cnb_buildpack.rb @@ -0,0 +1,20 @@ +module VCAP::CloudController + class CNBBuildpack < Buildpack + + def self.list_admin_buildpacks(stack_name=nil) + scoped = exclude(key: nil).exclude(key: '') + if stack_name.present? + scoped = scoped.filter(Sequel.or([ + [:stack, stack_name], + [:stack, nil] + ])) + end + scoped.order(:position).all + end + + def self.at_last_position + where(position: max(:position)).first + end + + end +end From ebdeff7128b0788786451e6a34d0bd02550b0640 Mon Sep 17 00:00:00 2001 From: Ralf Pannemans Date: Mon, 15 Jul 2024 16:20:06 +0200 Subject: [PATCH 03/20] WIP: Fix scoped buildpack order Co-authored-by: Nicolas Bender --- app/models/runtime/buildpack.rb | 23 ++++++++++++++------ app/models/runtime/classic_buildpack.rb | 20 ----------------- app/models/runtime/cnb_buildpack.rb | 20 ----------------- spec/unit/models/runtime/buildpack_spec.rb | 25 ++++++++++++++++++++++ 4 files changed, 42 insertions(+), 46 deletions(-) delete mode 100644 app/models/runtime/classic_buildpack.rb delete mode 100644 app/models/runtime/cnb_buildpack.rb diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index e02f0726120..a33238cb7cd 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -1,11 +1,6 @@ module VCAP::CloudController class Buildpack < Sequel::Model - plugin :list - plugin :single_table_inheritance, :lifecycle, - model_map: { - VCAP::CloudController::Lifecycles::BUILDPACK => "VCAP::CloudController::ClassicBuildpack", - VCAP::CloudController::Lifecycles::CNB => "VCAP::CloudController::CNBBuildpack" - } + plugin :list, scope: :lifecycle export_attributes :name, :stack, :position, :enabled, :locked, :filename import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key @@ -25,6 +20,22 @@ def self.user_visibility_filter(_user) full_dataset_filter end + def self.list_admin_buildpacks(stack_name=nil, lifecycle='buildpack') + scoped = exclude(key: nil).exclude(key: '') + scoped = scoped.filter(:lifecycle => lifecycle) + if stack_name.present? + scoped = scoped.filter(Sequel.or([ + [:stack, stack_name], + [:stack, nil] + ])) + end + scoped.order(:position).all + end + + def self.at_last_position + where(position: max(:position)).first + end + def validate validates_unique %i[name stack] validates_format(/\A(\w|-)+\z/, :name, message: 'can only contain alphanumeric characters') diff --git a/app/models/runtime/classic_buildpack.rb b/app/models/runtime/classic_buildpack.rb deleted file mode 100644 index 9171199b226..00000000000 --- a/app/models/runtime/classic_buildpack.rb +++ /dev/null @@ -1,20 +0,0 @@ -module VCAP::CloudController - class ClassicBuildpack < Buildpack - - def self.list_admin_buildpacks(stack_name=nil) - scoped = exclude(key: nil).exclude(key: '') - if stack_name.present? - scoped = scoped.filter(Sequel.or([ - [:stack, stack_name], - [:stack, nil] - ])) - end - scoped.order(:position).all - end - - def self.at_last_position - where(position: max(:position)).first - end - - end -end diff --git a/app/models/runtime/cnb_buildpack.rb b/app/models/runtime/cnb_buildpack.rb deleted file mode 100644 index a275b0db1ec..00000000000 --- a/app/models/runtime/cnb_buildpack.rb +++ /dev/null @@ -1,20 +0,0 @@ -module VCAP::CloudController - class CNBBuildpack < Buildpack - - def self.list_admin_buildpacks(stack_name=nil) - scoped = exclude(key: nil).exclude(key: '') - if stack_name.present? - scoped = scoped.filter(Sequel.or([ - [:stack, stack_name], - [:stack, nil] - ])) - end - scoped.order(:position).all - end - - def self.at_last_position - where(position: max(:position)).first - end - - end -end diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index 794c7b3a9ab..de9454a5ee1 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -91,6 +91,7 @@ def ordered_buildpacks let(:buildpack_file_1) { Tempfile.new('admin buildpack 1') } let(:buildpack_file_2) { Tempfile.new('admin buildpack 2') } let(:buildpack_file_3) { Tempfile.new('admin buildpack 3') } + let(:buildpack_file_cnb) { Tempfile.new('admin buildpack cnb') } let(:buildpack_blobstore) { CloudController::DependencyLocator.instance.buildpack_blobstore } @@ -103,6 +104,7 @@ def ordered_buildpacks Timecop.return end + subject(:cnb_buildpacks) { Buildpack.list_admin_buildpacks(nil, 'cnb') } subject(:all_buildpacks) { Buildpack.list_admin_buildpacks } context 'with prioritized buildpacks' do @@ -110,6 +112,9 @@ def ordered_buildpacks buildpack_blobstore.cp_to_blobstore(buildpack_file_1.path, 'a key') Buildpack.make(key: 'a key', position: 2) + buildpack_blobstore.cp_to_blobstore(buildpack_file_cnb.path, 'cnb key') + @cnb_buildpack = Buildpack.make(key: 'cnb key', position: 1, lifecycle: 'cnb') + buildpack_blobstore.cp_to_blobstore(buildpack_file_2.path, 'b key') Buildpack.make(key: 'b key', position: 1) @@ -131,6 +136,10 @@ def ordered_buildpacks expect(all_buildpacks).to have(2).items end + it 'returns the list of cnb buildpacks' do + expect(cnb_buildpacks.collect(&:key)).to eq(['cnb key']) + end + it 'randomly orders any buildpacks with the same position (for now we did not want to make clever logic of shifting stuff around: up to the user to get it all correct)' do @another_buildpack.position = 1 @another_buildpack.save @@ -138,6 +147,13 @@ def ordered_buildpacks expect(all_buildpacks[2].key).to eq('a key') end + it 'does not reorder cnb buildpacks when classical buildpacks change position)' do + @cnb_buildpack.position = 1 + @cnb_buildpack.save + + expect(cnb_buildpacks[0].key).to eq('cnb key') + end + context 'and there are buildpacks with null keys' do let!(:null_buildpack) { Buildpack.create(name: 'nil_key_custom_buildpack', stack: Stack.make.name, position: 0) } @@ -175,6 +191,7 @@ def ordered_buildpacks end context 'with a stack' do + subject(:cnb_buildpacks) { Buildpack.list_admin_buildpacks('stack2', 'cnb') } subject(:all_buildpacks) { Buildpack.list_admin_buildpacks('stack1') } let!(:stack1) { Stack.make(name: 'stack1') } let!(:stack2) { Stack.make(name: 'stack2') } @@ -186,6 +203,9 @@ def ordered_buildpacks 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_cnb.path, 'cnb key') + Buildpack.make(key: 'cnb key', position: 1, stack: 'stack2', lifecycle: 'cnb') + buildpack_blobstore.cp_to_blobstore(buildpack_file_3.path, 'c key') @another_buildpack = Buildpack.make(key: 'c key', position: 3, stack: nil) end @@ -195,6 +215,11 @@ def ordered_buildpacks 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 + + it 'returns the list of cnb buildpacks' do + expect(cnb_buildpacks.collect(&:key)).to eq(['cnb key']) + end + end end From 5b630a7426e1b2ca4455358efc291397665ebb64 Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Tue, 16 Jul 2024 16:42:09 +0200 Subject: [PATCH 04/20] WIP: Fix passing lifecycle Co-authored-by: Nicolas Bender --- app/actions/build_create.rb | 1 - app/fetchers/buildpack_lifecycle_fetcher.rb | 10 +-- app/messages/app_manifest_message.rb | 10 +-- app/messages/buildpack_upload_message.rb | 16 +++-- app/models/runtime/buildpack.rb | 4 +- .../runtime/cnb_lifecycle_data_model.rb | 47 ++++++++++++-- .../diego/buildpack/staging_action_builder.rb | 61 +------------------ .../diego/cnb/staging_action_builder.rb | 2 +- .../diego/docker/staging_action_builder.rb | 4 ++ .../lifecycles/app_buildpack_lifecycle.rb | 2 +- .../diego/lifecycles/app_cnb_lifecycle.rb | 23 ++++--- .../diego/staging_action_builder.rb | 52 +++++++++++++++- 12 files changed, 135 insertions(+), 97 deletions(-) diff --git a/app/actions/build_create.rb b/app/actions/build_create.rb index d870e59441d..80c6bd79fa4 100644 --- a/app/actions/build_create.rb +++ b/app/actions/build_create.rb @@ -95,7 +95,6 @@ def create_and_stage(package:, lifecycle:, metadata: nil, start_after_staging: f def requested_buildpacks_disabled!(lifecycle) return if lifecycle.type == Lifecycles::DOCKER - return if lifecycle.type == Lifecycles::CNB admin_buildpack_records = lifecycle.buildpack_infos.map(&:buildpack_record).compact disabled_buildpacks = admin_buildpack_records.reject(&:enabled) diff --git a/app/fetchers/buildpack_lifecycle_fetcher.rb b/app/fetchers/buildpack_lifecycle_fetcher.rb index 72ca45f43da..e7e9ed24b82 100644 --- a/app/fetchers/buildpack_lifecycle_fetcher.rb +++ b/app/fetchers/buildpack_lifecycle_fetcher.rb @@ -1,17 +1,19 @@ +require 'cloud_controller/diego/lifecycles/lifecycles' + module VCAP::CloudController class BuildpackLifecycleFetcher class << self - def fetch(buildpack_names, stack_name) + def fetch(buildpack_names, stack_name, lifecycle = VCAP::CloudController::Lifecycles::BUILDPACK) { stack: Stack.find(name: stack_name), - buildpack_infos: ordered_buildpacks(buildpack_names, stack_name) + buildpack_infos: ordered_buildpacks(buildpack_names, stack_name, lifecycle) } end private - def ordered_buildpacks(buildpack_names, stack_name) - buildpacks_with_stacks, buildpacks_without_stacks = Buildpack.list_admin_buildpacks(stack_name).partition(&:stack) + def ordered_buildpacks(buildpack_names, stack_name, lifecycle) + buildpacks_with_stacks, buildpacks_without_stacks = Buildpack.list_admin_buildpacks(stack_name, lifecycle).partition(&:stack) buildpack_names.map do |buildpack_name| buildpack_record = buildpacks_with_stacks.find { |b| b.name == buildpack_name } || buildpacks_without_stacks.find { |b| b.name == buildpack_name } diff --git a/app/messages/app_manifest_message.rb b/app/messages/app_manifest_message.rb index a2fa25dc55e..b0bd061d883 100644 --- a/app/messages/app_manifest_message.rb +++ b/app/messages/app_manifest_message.rb @@ -72,7 +72,6 @@ def self.underscore_keys(hash) validate :validate_buildpack_and_buildpacks_combination! validate :validate_docker_enabled! validate :validate_cnb_enabled! - validate :validate_cnb_buildpacks! validate :validate_docker_buildpacks_combination! validate :validate_service_bindings_message!, if: ->(record) { record.requested?(:services) } validate :validate_env_update_message!, if: ->(record) { record.requested?(:env) } @@ -291,7 +290,7 @@ def cnb_lifecycle_data requested_buildpacks = @buildpacks elsif requested?(:buildpack) requested_buildpacks = [] - requested_buildpacks.push(@buildpack) + requested_buildpacks.push(@buildpack) unless should_autodetect?(@buildpack) end { @@ -485,13 +484,6 @@ def validate_cnb_enabled! errors.add(:base, e.message) end - def validate_cnb_buildpacks! - return unless @lifecycle == 'cnb' - return if requested?(:lifecycle) && (requested?(:buildpack) || requested?(:buildpacks)) - - errors.add(:base, 'Buildpack(s) must be specified when using Cloud Native Buildpacks') - end - def validate_docker_buildpacks_combination! return unless requested?(:docker) && (requested?(:buildpack) || requested?(:buildpacks)) diff --git a/app/messages/buildpack_upload_message.rb b/app/messages/buildpack_upload_message.rb index e6e58f3f50a..b1acf1bc7b9 100644 --- a/app/messages/buildpack_upload_message.rb +++ b/app/messages/buildpack_upload_message.rb @@ -1,16 +1,18 @@ require 'messages/base_message' module VCAP::CloudController + GZIP_MIME = Regexp.new("\x1F\x8B\x08".force_encoding("binary")) + ZIP_MIME = Regexp.new("PK\x03\x04".force_encoding("binary")) + class BuildpackUploadMessage < BaseMessage class MissingFilePathError < StandardError; end - register_allowed_keys %i[bits_path bits_name upload_start_time] validates_with NoAdditionalKeysValidator validate :nginx_fields validate :bits_path_in_tmpdir - validate :is_zip + validate :is_archive validate :is_not_empty validate :missing_file_path @@ -43,10 +45,16 @@ def tmpdir VCAP::CloudController::Config.config.get(:directories, :tmpdir) end - def is_zip + def is_archive return unless bits_name + return unless bits_path + + case IO.read(bits_path, 4) + when /^#{VCAP::CloudController::GZIP_MIME}/, /^#{VCAP::CloudController::ZIP_MIME}/ + else + errors.add(:base, "#{bits_name} is not a zip or gzip archive") + end - errors.add(:base, "#{bits_name} is not a zip") unless File.extname(bits_name) == '.zip' end def missing_file_path diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index a33238cb7cd..634f861fc87 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -1,3 +1,5 @@ +require 'cloud_controller/diego/lifecycles/lifecycles' + module VCAP::CloudController class Buildpack < Sequel::Model plugin :list, scope: :lifecycle @@ -20,7 +22,7 @@ def self.user_visibility_filter(_user) full_dataset_filter end - def self.list_admin_buildpacks(stack_name=nil, lifecycle='buildpack') + def self.list_admin_buildpacks(stack_name=nil, lifecycle=VCAP::CloudController::Lifecycles::BUILDPACK) scoped = exclude(key: nil).exclude(key: '') scoped = scoped.filter(:lifecycle => lifecycle) if stack_name.present? diff --git a/app/models/runtime/cnb_lifecycle_data_model.rb b/app/models/runtime/cnb_lifecycle_data_model.rb index 4b53328e195..cb2523af33d 100644 --- a/app/models/runtime/cnb_lifecycle_data_model.rb +++ b/app/models/runtime/cnb_lifecycle_data_model.rb @@ -44,7 +44,7 @@ def buildpacks def buildpack_models if buildpack_lifecycle_buildpacks.present? buildpack_lifecycle_buildpacks.map do |buildpack| - CustomBuildpack.new(buildpack.name) + Buildpack.find(name: buildpack.name) || CustomBuildpack.new(buildpack.name) end else [] @@ -64,11 +64,7 @@ def first_custom_buildpack_url end def using_custom_buildpack? - true - end - - def attributes_from_buildpack(buildpack_name) - { buildpack_url: buildpack_name, admin_buildpack_name: nil } + buildpack_lifecycle_buildpacks.any?(&:custom?) end def to_hash @@ -96,5 +92,44 @@ def credentials def credentials=(creds) self.registry_credentials_json = Oj.dump(creds) end + + private + + def attributes_from_buildpack_name(buildpack_name) + if UriUtils.is_buildpack_uri?(buildpack_name) + { buildpack_url: buildpack_name, admin_buildpack_name: nil } + else + { buildpack_url: nil, admin_buildpack_name: buildpack_name } + end + end + + def attributes_from_buildpack_key(key) + admin_buildpack = Buildpack.find(key:) + if admin_buildpack + { buildpack_url: nil, admin_buildpack_name: admin_buildpack.name } + elsif UriUtils.is_buildpack_uri?(key) + { buildpack_url: key, admin_buildpack_name: nil } + else + {} # Will fail a validity check downstream + end + end + + def attributes_from_buildpack_hash(buildpack) + { + buildpack_name: buildpack[:name], + version: buildpack[:version] + }.merge(buildpack[:key] ? attributes_from_buildpack_key(buildpack[:key]) : attributes_from_buildpack_name(buildpack[:name])) + end + + def attributes_from_buildpack(buildpack) + if buildpack.is_a?(String) + attributes_from_buildpack_name buildpack + elsif buildpack.is_a?(Hash) + attributes_from_buildpack_hash buildpack + else + # Don't set anything -- this will fail later on a validity check + {} + end + end end end diff --git a/lib/cloud_controller/diego/buildpack/staging_action_builder.rb b/lib/cloud_controller/diego/buildpack/staging_action_builder.rb index a0fe002a977..f00d67491ba 100644 --- a/lib/cloud_controller/diego/buildpack/staging_action_builder.rb +++ b/lib/cloud_controller/diego/buildpack/staging_action_builder.rb @@ -8,58 +8,7 @@ module Diego module Buildpack class StagingActionBuilder < VCAP::CloudController::Diego::StagingActionBuilder def initialize(config, staging_details, lifecycle_data) - super(config, staging_details, lifecycle_data, 'buildpack', '/tmp/app', '/tmp/output-cache') - end - - def additional_image_layers - lifecycle_data[:buildpacks]. - reject { |buildpack| buildpack[:name] == 'custom' }. - map do |buildpack| - layer = { - name: buildpack[:name], - url: buildpack[:url], - destination_path: buildpack_path(buildpack[:key]), - layer_type: ::Diego::Bbs::Models::ImageLayer::Type::SHARED, - media_type: ::Diego::Bbs::Models::ImageLayer::MediaType::ZIP - } - if buildpack[:sha256] - layer[:digest_algorithm] = ::Diego::Bbs::Models::ImageLayer::DigestAlgorithm::SHA256 - layer[:digest_value] = buildpack[:sha256] - end - - ::Diego::Bbs::Models::ImageLayer.new(layer.compact) - end - end - - def cached_dependencies - return nil if @config.get(:diego, :enable_declarative_asset_downloads) - - dependencies = [ - ::Diego::Bbs::Models::CachedDependency.new( - from: LifecycleBundleUriGenerator.uri(config.get(:diego, :lifecycle_bundles)[lifecycle_bundle_key]), - to: '/tmp/lifecycle', - cache_key: "buildpack-#{lifecycle_stack}-lifecycle" - ) - ] - - others = lifecycle_data[:buildpacks].map do |buildpack| - next if buildpack[:name] == 'custom' - - buildpack_dependency = { - name: buildpack[:name], - from: buildpack[:url], - to: buildpack_path(buildpack[:key]), - cache_key: buildpack[:key] - } - if buildpack[:sha256] - buildpack_dependency[:checksum_algorithm] = 'sha256' - buildpack_dependency[:checksum_value] = buildpack[:sha256] - end - - ::Diego::Bbs::Models::CachedDependency.new(buildpack_dependency.compact) - end.compact - - dependencies.concat(others) + super(config, staging_details, lifecycle_data, 'buildpack', '/tmp/app', '/tmp/output-cache', ::Diego::Bbs::Models::ImageLayer::MediaType::ZIP) end def task_environment_variables @@ -96,14 +45,6 @@ def platform_options_env arr end - - def buildpack_path(buildpack_key) - if config.get(:staging, :legacy_md5_buildpack_paths_enabled) - "/tmp/buildpacks/#{OpenSSL::Digest::MD5.hexdigest(buildpack_key)}" - else - "/tmp/buildpacks/#{Digest::XXH64.hexdigest(buildpack_key)}" - end - end end end end diff --git a/lib/cloud_controller/diego/cnb/staging_action_builder.rb b/lib/cloud_controller/diego/cnb/staging_action_builder.rb index 28f6fb2f6a5..663d4f23546 100644 --- a/lib/cloud_controller/diego/cnb/staging_action_builder.rb +++ b/lib/cloud_controller/diego/cnb/staging_action_builder.rb @@ -8,7 +8,7 @@ module Diego module CNB class StagingActionBuilder < VCAP::CloudController::Diego::StagingActionBuilder def initialize(config, staging_details, lifecycle_data) - super(config, staging_details, lifecycle_data, 'cnb', '/home/vcap/workspace', '/tmp/cache-output.tgz') + super(config, staging_details, lifecycle_data, 'cnb', '/home/vcap/workspace', '/tmp/cache-output.tgz', ::Diego::Bbs::Models::ImageLayer::MediaType::TGZ) end def cached_dependencies diff --git a/lib/cloud_controller/diego/docker/staging_action_builder.rb b/lib/cloud_controller/diego/docker/staging_action_builder.rb index d01fa05b070..c4c259899b2 100644 --- a/lib/cloud_controller/diego/docker/staging_action_builder.rb +++ b/lib/cloud_controller/diego/docker/staging_action_builder.rb @@ -67,6 +67,10 @@ def cached_dependencies ] end + def additional_image_layers + [] + end + def stack "preloaded:#{config.get(:diego, :docker_staging_stack)}" end diff --git a/lib/cloud_controller/diego/lifecycles/app_buildpack_lifecycle.rb b/lib/cloud_controller/diego/lifecycles/app_buildpack_lifecycle.rb index aa9d28e1e54..419c9fc9c6c 100644 --- a/lib/cloud_controller/diego/lifecycles/app_buildpack_lifecycle.rb +++ b/lib/cloud_controller/diego/lifecycles/app_buildpack_lifecycle.rb @@ -8,7 +8,7 @@ class AppBuildpackLifecycle < AppBaseLifecycle def initialize(message) @message = message - db_result = BuildpackLifecycleFetcher.fetch(buildpacks, stack) + db_result = BuildpackLifecycleFetcher.fetch(buildpacks, stack, type) @validator = BuildpackLifecycleDataValidator.new({ buildpack_infos: db_result[:buildpack_infos], stack: db_result[:stack] diff --git a/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle.rb b/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle.rb index 139e9c4131a..a4eae0f65dd 100644 --- a/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle.rb +++ b/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle.rb @@ -1,11 +1,22 @@ +require 'cloud_controller/diego/lifecycles/buildpack_info' +require 'cloud_controller/diego/lifecycles/buildpack_lifecycle_data_validator' require 'cloud_controller/diego/lifecycles/app_base_lifecycle' +require 'fetchers/buildpack_lifecycle_fetcher' module VCAP::CloudController class AppCNBLifecycle < AppBaseLifecycle def initialize(message) @message = message + + db_result = BuildpackLifecycleFetcher.fetch(buildpacks, stack, type) + @validator = BuildpackLifecycleDataValidator.new({ + buildpack_infos: db_result[:buildpack_infos], + stack: db_result[:stack] + }) end + delegate :valid?, :errors, to: :validator + def create_lifecycle_data_model(app) CNBLifecycleDataModel.create( buildpacks:, @@ -15,14 +26,6 @@ def create_lifecycle_data_model(app) ) end - def valid? - message.is_a?(AppUpdateMessage) || !buildpacks.empty? - end - - def errors - [] - end - def update_lifecycle_data_credentials(app) return unless message.buildpack_data.requested?(:credentials) @@ -36,5 +39,9 @@ def type def credentials message.buildpack_data.credentials end + + private + + attr_reader :validator end end diff --git a/lib/cloud_controller/diego/staging_action_builder.rb b/lib/cloud_controller/diego/staging_action_builder.rb index d1dbdff9522..3cca3aca34b 100644 --- a/lib/cloud_controller/diego/staging_action_builder.rb +++ b/lib/cloud_controller/diego/staging_action_builder.rb @@ -10,13 +10,14 @@ class StagingActionBuilder attr_reader :config, :lifecycle_data, :staging_details - def initialize(config, staging_details, lifecycle_data, prefix, app_destination_path, cache_source) + def initialize(config, staging_details, lifecycle_data, prefix, app_destination_path, cache_source, bp_media_type) @config = config @staging_details = staging_details @lifecycle_data = lifecycle_data @prefix = prefix @app_destination_path = app_destination_path @cache_source = cache_source + @bp_media_type = bp_media_type end def action @@ -35,8 +36,55 @@ def action serial(actions) end + def cached_dependencies + return nil if @config.get(:diego, :enable_declarative_asset_downloads) + + dependencies = [ + ::Diego::Bbs::Models::CachedDependency.new( + from: LifecycleBundleUriGenerator.uri(config.get(:diego, :lifecycle_bundles)[lifecycle_bundle_key]), + to: '/tmp/lifecycle', + cache_key: "#{@prefix}-#{lifecycle_stack}-lifecycle" + ) + ] + + others = lifecycle_data[:buildpacks].map do |buildpack| + next if buildpack[:name] == 'custom' + + buildpack_dependency = { + name: buildpack[:name], + from: buildpack[:url], + to: buildpack_path(buildpack[:key]), + cache_key: buildpack[:key] + } + if buildpack[:sha256] + buildpack_dependency[:checksum_algorithm] = 'sha256' + buildpack_dependency[:checksum_value] = buildpack[:sha256] + end + + ::Diego::Bbs::Models::CachedDependency.new(buildpack_dependency.compact) + end.compact + + dependencies.concat(others) + end + def additional_image_layers - [] + lifecycle_data[:buildpacks]. + reject { |buildpack| buildpack[:name] == 'custom' }. + map do |buildpack| + layer = { + name: buildpack[:name], + url: buildpack[:url], + destination_path: buildpack_path(buildpack[:key]), + layer_type: ::Diego::Bbs::Models::ImageLayer::Type::SHARED, + media_type: @bp_media_type + } + if buildpack[:sha256] + layer[:digest_algorithm] = ::Diego::Bbs::Models::ImageLayer::DigestAlgorithm::SHA256 + layer[:digest_value] = buildpack[:sha256] + end + + ::Diego::Bbs::Models::ImageLayer.new(layer.compact) + end end def image_layers From cfa0f87a5dc0ae53e290a494d9ab4ddf27308cc2 Mon Sep 17 00:00:00 2001 From: Johannes Dillmann Date: Mon, 22 Jul 2024 13:57:52 +0200 Subject: [PATCH 05/20] add support for system cnbs Co-authored-by: Pavel Busko --- app/messages/app_manifest_message.rb | 29 +++++++------------ .../diego/buildpack/lifecycle_protocol.rb | 4 +++ .../diego/buildpack_entry_generator.rb | 5 ++-- .../diego/cnb/lifecycle_protocol.rb | 4 +++ .../diego/cnb/staging_action_builder.rb | 15 ++-------- .../diego/lifecycle_protocol.rb | 2 +- lib/cloud_controller/upload_buildpack.rb | 3 ++ .../messages/app_manifest_message_spec.rb | 21 ++------------ 8 files changed, 31 insertions(+), 52 deletions(-) diff --git a/app/messages/app_manifest_message.rb b/app/messages/app_manifest_message.rb index b0bd061d883..6a8550e4d7d 100644 --- a/app/messages/app_manifest_message.rb +++ b/app/messages/app_manifest_message.rb @@ -284,15 +284,6 @@ def docker_lifecycle_data end def cnb_lifecycle_data - return unless requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack) - - if requested?(:buildpacks) - requested_buildpacks = @buildpacks - elsif requested?(:buildpack) - requested_buildpacks = [] - requested_buildpacks.push(@buildpack) unless should_autodetect?(@buildpack) - end - { type: Lifecycles::CNB, data: { @@ -304,16 +295,8 @@ def cnb_lifecycle_data end def buildpacks_lifecycle_data - return unless requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack) - - if requested?(:buildpacks) - requested_buildpacks = @buildpacks - elsif requested?(:buildpack) - requested_buildpacks = [] - requested_buildpacks.push(@buildpack) unless should_autodetect?(@buildpack) - end - { + type: Lifecycles::BUILDPACK, data: { buildpacks: requested_buildpacks, stack: @stack @@ -493,5 +476,15 @@ def validate_docker_buildpacks_combination! def add_process_error!(error_message, type) errors.add(:base, %(Process "#{type}": #{error_message})) end + + def requested_buildpacks + return nil unless requested?(:buildpacks) || requested?(:buildpack) + return @buildpacks if requested?(:buildpacks) + + buildpacks = [] + buildpacks.push(@buildpack) if requested?(:buildpack) && !should_autodetect?(@buildpack) + + buildpacks + end end end diff --git a/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb b/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb index 8122ac7c03e..4305a62581d 100644 --- a/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb +++ b/lib/cloud_controller/diego/buildpack/lifecycle_protocol.rb @@ -23,6 +23,10 @@ def desired_lrp_builder(config, process) def new_lifecycle_data(_) LifecycleData.new end + + def type + VCAP::CloudController::Lifecycles::BUILDPACK + end end end end diff --git a/lib/cloud_controller/diego/buildpack_entry_generator.rb b/lib/cloud_controller/diego/buildpack_entry_generator.rb index a4a85be256e..ebd66c3923b 100644 --- a/lib/cloud_controller/diego/buildpack_entry_generator.rb +++ b/lib/cloud_controller/diego/buildpack_entry_generator.rb @@ -1,8 +1,9 @@ module VCAP::CloudController module Diego class BuildpackEntryGenerator - def initialize(blobstore_url_generator) + def initialize(blobstore_url_generator, type) @blobstore_url_generator = blobstore_url_generator + @type = type end def buildpack_entries(buildpack_infos, stack_name) @@ -26,7 +27,7 @@ def custom_buildpack_entry(buildpack) end def default_admin_buildpacks(stack_name) - VCAP::CloudController::Buildpack.list_admin_buildpacks(stack_name). + VCAP::CloudController::Buildpack.list_admin_buildpacks(stack_name, @type). select(&:enabled). collect { |buildpack| admin_buildpack_entry(buildpack) } end diff --git a/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb b/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb index 7f9507987c3..bc65ea521a4 100644 --- a/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb +++ b/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb @@ -28,6 +28,10 @@ def new_lifecycle_data(staging_details) lifecycle_data end + + def type + VCAP::CloudController::Lifecycles::CNB + end end end end diff --git a/lib/cloud_controller/diego/cnb/staging_action_builder.rb b/lib/cloud_controller/diego/cnb/staging_action_builder.rb index 663d4f23546..675ea28c350 100644 --- a/lib/cloud_controller/diego/cnb/staging_action_builder.rb +++ b/lib/cloud_controller/diego/cnb/staging_action_builder.rb @@ -11,18 +11,6 @@ def initialize(config, staging_details, lifecycle_data) super(config, staging_details, lifecycle_data, 'cnb', '/home/vcap/workspace', '/tmp/cache-output.tgz', ::Diego::Bbs::Models::ImageLayer::MediaType::TGZ) end - def cached_dependencies - return nil if @config.get(:diego, :enable_declarative_asset_downloads) - - [ - ::Diego::Bbs::Models::CachedDependency.new( - from: LifecycleBundleUriGenerator.uri(config.get(:diego, :lifecycle_bundles)[lifecycle_bundle_key]), - to: '/tmp/lifecycle', - cache_key: "#{@prefix}-#{lifecycle_stack}-lifecycle" - ) - ] - end - def task_environment_variables env = [ ::Diego::Bbs::Models::EnvironmentVariable.new(name: 'CNB_USER_ID', value: '2000'), @@ -43,7 +31,8 @@ def stage_action ] lifecycle_data[:buildpacks].each do |buildpack| - args.push('--buildpack', buildpack[:url]) + args.push('--buildpack', buildpack[:url]) if buildpack[:name] == 'custom' + args.push('--system-buildpack', buildpack[:key]) unless buildpack[:name] == 'custom' end env_vars = BbsEnvironmentBuilder.build(staging_details.environment_variables) diff --git a/lib/cloud_controller/diego/lifecycle_protocol.rb b/lib/cloud_controller/diego/lifecycle_protocol.rb index c36cd94bd0f..e2f0f2b7f7e 100644 --- a/lib/cloud_controller/diego/lifecycle_protocol.rb +++ b/lib/cloud_controller/diego/lifecycle_protocol.rb @@ -19,7 +19,7 @@ class InvalidDownloadUri < StandardError; end def initialize(blobstore_url_generator=::CloudController::DependencyLocator.instance.blobstore_url_generator, droplet_url_generator=::CloudController::DependencyLocator.instance.droplet_url_generator) @blobstore_url_generator = blobstore_url_generator - @buildpack_entry_generator = BuildpackEntryGenerator.new(@blobstore_url_generator) + @buildpack_entry_generator = BuildpackEntryGenerator.new(@blobstore_url_generator, type) @droplet_url_generator = droplet_url_generator end diff --git a/lib/cloud_controller/upload_buildpack.rb b/lib/cloud_controller/upload_buildpack.rb index cda53101a6e..2ccb2b9f86d 100644 --- a/lib/cloud_controller/upload_buildpack.rb +++ b/lib/cloud_controller/upload_buildpack.rb @@ -1,4 +1,5 @@ require 'vcap/digester' +require 'cloud_controller/diego/lifecycles/lifecycles' module VCAP::CloudController class UploadBuildpack @@ -69,6 +70,8 @@ def determine_new_stack(buildpack, bits_file_path) extracted_stack = Buildpacks::StackNameExtractor.extract_from_file(bits_file_path) [extracted_stack, buildpack.stack].find(&:present?) rescue CloudController::Errors::BuildpackError => e + return buildpack.stack if buildpack.lifecycle == Lifecycles::CNB + raise CloudController::Errors::ApiError.new_from_details('BuildpackZipError', e.message) end diff --git a/spec/unit/messages/app_manifest_message_spec.rb b/spec/unit/messages/app_manifest_message_spec.rb index 67ce16b5d4f..74a5d59ab3d 100644 --- a/spec/unit/messages/app_manifest_message_spec.rb +++ b/spec/unit/messages/app_manifest_message_spec.rb @@ -1114,22 +1114,6 @@ module VCAP::CloudController expect(message.errors.full_messages).to match_array(error_messages) end end - - context 'when cnb: true and no buildpacks provided' do - before do - FeatureFlag.make(name: 'diego_cnb', enabled: true, error_message: nil) - end - - let(:params_from_yaml) { { name: 'eugene', lifecycle: 'cnb' } } - - it 'is not valid' do - message = AppManifestMessage.create_from_yml(params_from_yaml) - - expect(message).not_to be_valid - expect(message.errors).to have(1).items - expect(message.errors.full_messages).to include('Buildpack(s) must be specified when using Cloud Native Buildpacks') - end - end end describe '.create_from_yml' do @@ -2009,10 +1993,11 @@ module VCAP::CloudController context 'when no lifecycle data is requested in the manifest' do let(:parsed_yaml) { {} } - it 'does not forward missing attributes to the AppUpdateMessage' do + it 'defaults to the buildpack lifecycle' do message = AppManifestMessage.create_from_yml(parsed_yaml) - expect(message.app_update_message.requested?(:lifecycle)).to be false + expect(message.app_update_message.requested?(:lifecycle)).to be true + expect(message.app_update_message.lifecycle_type).to eq('buildpack') end end From 8826d902d7c8b1be178fdbcec30c912478bb4108 Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Mon, 22 Jul 2024 16:51:57 +0200 Subject: [PATCH 06/20] Add missing tests Co-authored-by: Johannes Dillmann --- app/fetchers/buildpack_lifecycle_fetcher.rb | 2 +- app/messages/buildpack_upload_message.rb | 13 +++-- app/models/runtime/buildpack.rb | 6 +-- .../runtime/cnb_lifecycle_data_model.rb | 4 +- .../diego/lifecycles/buildpack_info.rb | 2 +- spec/support/test_tgz.rb | 18 +++++++ spec/unit/actions/buildpack_create_spec.rb | 17 ++++++- .../diego/buildpack_entry_generator_spec.rb | 2 +- .../diego/cnb/staging_action_builder_spec.rb | 50 ++++++++++++++++++- .../lifecycles/app_cnb_lifecycle_spec.rb | 13 +++-- .../cloud_controller/upload_buildpack_spec.rb | 20 ++++++++ .../messages/buildpack_create_message_spec.rb | 11 ++++ spec/unit/models/runtime/buildpack_spec.rb | 6 +-- .../runtime/cnb_lifecycle_data_model_spec.rb | 2 +- 14 files changed, 140 insertions(+), 26 deletions(-) create mode 100644 spec/support/test_tgz.rb diff --git a/app/fetchers/buildpack_lifecycle_fetcher.rb b/app/fetchers/buildpack_lifecycle_fetcher.rb index e7e9ed24b82..58813d59114 100644 --- a/app/fetchers/buildpack_lifecycle_fetcher.rb +++ b/app/fetchers/buildpack_lifecycle_fetcher.rb @@ -3,7 +3,7 @@ module VCAP::CloudController class BuildpackLifecycleFetcher class << self - def fetch(buildpack_names, stack_name, lifecycle = VCAP::CloudController::Lifecycles::BUILDPACK) + def fetch(buildpack_names, stack_name, lifecycle=VCAP::CloudController::Lifecycles::BUILDPACK) { stack: Stack.find(name: stack_name), buildpack_infos: ordered_buildpacks(buildpack_names, stack_name, lifecycle) diff --git a/app/messages/buildpack_upload_message.rb b/app/messages/buildpack_upload_message.rb index b1acf1bc7b9..f0ebe0b1da0 100644 --- a/app/messages/buildpack_upload_message.rb +++ b/app/messages/buildpack_upload_message.rb @@ -1,8 +1,8 @@ require 'messages/base_message' module VCAP::CloudController - GZIP_MIME = Regexp.new("\x1F\x8B\x08".force_encoding("binary")) - ZIP_MIME = Regexp.new("PK\x03\x04".force_encoding("binary")) + GZIP_MIME = Regexp.new("\x1F\x8B\x08".force_encoding('binary')) + ZIP_MIME = Regexp.new("PK\x03\x04".force_encoding('binary')) class BuildpackUploadMessage < BaseMessage class MissingFilePathError < StandardError; end @@ -49,12 +49,11 @@ def is_archive return unless bits_name return unless bits_path - case IO.read(bits_path, 4) - when /^#{VCAP::CloudController::GZIP_MIME}/, /^#{VCAP::CloudController::ZIP_MIME}/ - else - errors.add(:base, "#{bits_name} is not a zip or gzip archive") - end + mime_bits = File.read(bits_path, 4) + return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/ || mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/ + + errors.add(:base, "#{bits_name} is not a zip or gzip archive") end def missing_file_path diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 634f861fc87..ee3378e5f29 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -4,8 +4,8 @@ module VCAP::CloudController class Buildpack < Sequel::Model plugin :list, scope: :lifecycle - export_attributes :name, :stack, :position, :enabled, :locked, :filename - import_attributes :name, :stack, :position, :enabled, :locked, :filename, :key + export_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle + import_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle, :key PACKAGE_STATES = [ CREATED_STATE = 'AWAITING_UPLOAD'.freeze, @@ -24,7 +24,7 @@ def self.user_visibility_filter(_user) def self.list_admin_buildpacks(stack_name=nil, lifecycle=VCAP::CloudController::Lifecycles::BUILDPACK) scoped = exclude(key: nil).exclude(key: '') - scoped = scoped.filter(:lifecycle => lifecycle) + scoped = scoped.filter(lifecycle:) if stack_name.present? scoped = scoped.filter(Sequel.or([ [:stack, stack_name], diff --git a/app/models/runtime/cnb_lifecycle_data_model.rb b/app/models/runtime/cnb_lifecycle_data_model.rb index cb2523af33d..63bd6a32142 100644 --- a/app/models/runtime/cnb_lifecycle_data_model.rb +++ b/app/models/runtime/cnb_lifecycle_data_model.rb @@ -96,7 +96,7 @@ def credentials=(creds) private def attributes_from_buildpack_name(buildpack_name) - if UriUtils.is_buildpack_uri?(buildpack_name) + if UriUtils.is_cnb_buildpack_uri?(buildpack_name) { buildpack_url: buildpack_name, admin_buildpack_name: nil } else { buildpack_url: nil, admin_buildpack_name: buildpack_name } @@ -107,7 +107,7 @@ def attributes_from_buildpack_key(key) admin_buildpack = Buildpack.find(key:) if admin_buildpack { buildpack_url: nil, admin_buildpack_name: admin_buildpack.name } - elsif UriUtils.is_buildpack_uri?(key) + elsif UriUtils.is_cnb_buildpack_uri?(key) { buildpack_url: key, admin_buildpack_name: nil } else {} # Will fail a validity check downstream diff --git a/lib/cloud_controller/diego/lifecycles/buildpack_info.rb b/lib/cloud_controller/diego/lifecycles/buildpack_info.rb index 0dbd3194317..0cae5333d22 100644 --- a/lib/cloud_controller/diego/lifecycles/buildpack_info.rb +++ b/lib/cloud_controller/diego/lifecycles/buildpack_info.rb @@ -7,7 +7,7 @@ class BuildpackInfo def initialize(buildpack_name_or_url, buildpack_record) @buildpack = buildpack_name_or_url @buildpack_record = buildpack_record - @buildpack_url = buildpack_name_or_url if UriUtils.is_buildpack_uri?(buildpack_name_or_url) + @buildpack_url = buildpack_name_or_url if UriUtils.is_buildpack_uri?(buildpack_name_or_url) || UriUtils.is_cnb_buildpack_uri?(buildpack_name_or_url) end def buildpack_exists_in_db? diff --git a/spec/support/test_tgz.rb b/spec/support/test_tgz.rb new file mode 100644 index 00000000000..15d1f44d738 --- /dev/null +++ b/spec/support/test_tgz.rb @@ -0,0 +1,18 @@ +require 'zlib' +require 'rubygems/package' + +module TestTgz + def self.create(tgz_name, file_count, file_size=1024, &) + File.open(tgz_name, 'wb') do |file| + Zlib::GzipWriter.wrap(file) do |gzip| + Gem::Package::TarWriter.new(gzip) do |tar| + file_count.times do |i| + tar.add_file_simple("ziptest_#{i}", 0o644, file_size) do |f| + f.write('A' * file_size) + end + end + end + end + end + end +end diff --git a/spec/unit/actions/buildpack_create_spec.rb b/spec/unit/actions/buildpack_create_spec.rb index 3a0c3dbf2ac..1bcc2d7e4bc 100644 --- a/spec/unit/actions/buildpack_create_spec.rb +++ b/spec/unit/actions/buildpack_create_spec.rb @@ -19,7 +19,8 @@ module VCAP::CloudController name: 'the-name', stack: 'the-stack', enabled: false, - locked: true + locked: true, + lifecycle: Lifecycles::BUILDPACK ) buildpack = BuildpackCreate.new.create(message) @@ -28,6 +29,7 @@ module VCAP::CloudController expect(buildpack.position).to eq(1) expect(buildpack.enabled).to be(false) expect(buildpack.locked).to be(true) + expect(buildpack.lifecycle).to eq(Lifecycles::BUILDPACK) end end @@ -114,6 +116,19 @@ module VCAP::CloudController end end + context 'when lifecycle is provided' do + it 'creates a buildpack with locked set to true' do + message = BuildpackCreateMessage.new( + name: 'the-name', + stack: 'the-stack', + lifecycle: Lifecycles::CNB + ) + buildpack = BuildpackCreate.new.create(message) + + expect(buildpack.lifecycle).to eq(Lifecycles::CNB) + end + end + context 'when a model validation fails' do it 'raises an error' do errors = Sequel::Model::Errors.new diff --git a/spec/unit/lib/cloud_controller/diego/buildpack_entry_generator_spec.rb b/spec/unit/lib/cloud_controller/diego/buildpack_entry_generator_spec.rb index 22d4b513782..fa869e91e55 100644 --- a/spec/unit/lib/cloud_controller/diego/buildpack_entry_generator_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/buildpack_entry_generator_spec.rb @@ -4,7 +4,7 @@ module VCAP::CloudController module Diego RSpec.describe BuildpackEntryGenerator do - subject(:buildpack_entry_generator) { BuildpackEntryGenerator.new(blobstore_url_generator) } + subject(:buildpack_entry_generator) { BuildpackEntryGenerator.new(blobstore_url_generator, Lifecycles::BUILDPACK) } let(:admin_buildpack_download_url) { 'http://admin-buildpack.example.com' } let(:app_package_download_url) { 'http://app-package.example.com' } diff --git a/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb b/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb index 3da080341f4..70688d809e3 100644 --- a/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb @@ -131,8 +131,8 @@ module CNB let(:buildpacks) do [ - { url: 'gcr.io/paketo-buildpacks/node-start', skip_detect: false }, - { url: 'gcr.io/paketo-buildpacks/node-engine', skip_detect: false } + { name: 'custom', url: 'gcr.io/paketo-buildpacks/node-start', skip_detect: false }, + { name: 'custom', url: 'gcr.io/paketo-buildpacks/node-engine', skip_detect: false } ] end @@ -240,6 +240,52 @@ module CNB end end end + + context('when system-buildpaks are used') do + let(:buildpacks) do + [ + { name: 'node-cnb', key: 'node-key', skip_detect: false }, + { name: 'java-cnb', key: 'java-key', skip_detect: false } + ] + end + + let(:run_staging_action) do + ::Diego::Bbs::Models::RunAction.new( + path: '/tmp/lifecycle/builder', + user: 'vcap', + args: ['--cache-dir', '/tmp/cache', '--cache-output', '/tmp/cache-output.tgz', '--system-buildpack', 'node-key', '--system-buildpack', + 'java-key', '--pass-env-var', 'FOO', '--pass-env-var', 'BAR'], + env: bbs_env + ) + end + + it 'returns the buildpack staging action without download actions' do + result = builder.action + + serial_action = result.serial_action + actions = serial_action.actions + + expect(actions[1].run_action).to eq(run_staging_action) + expect(builder.cached_dependencies).to include(::Diego::Bbs::Models::CachedDependency.new( + name: 'node-cnb', + from: '', + to: '/tmp/buildpacks/54548f35489d6234', + cache_key: 'node-cnb', + log_source: '', + checksum_algorithm: '', + checksum_value: '' + )) + expect(builder.cached_dependencies).to include(::Diego::Bbs::Models::CachedDependency.new( + name: 'java-cnb', + from: '', + to: '/tmp/buildpacks/c56b3bfdba7aa4dd', + cache_key: 'java-cnb', + log_source: '', + checksum_algorithm: '', + checksum_value: '' + )) + end + end end describe '#cached_dependencies' do diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb index 47fa8d320ca..6367395270c 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb @@ -43,7 +43,7 @@ module VCAP::CloudController let(:lifecycle_request_data) { { buildpacks: ['docker://nodejs', 'http://buildpack.com', 'http://other.com'] } } before do - Buildpack.make(name: 'custom-bp') + Buildpack.make(name: 'custom-bp', lifecycle: 'cnb') end it 'uses all of the buildpacks' do @@ -96,8 +96,8 @@ module VCAP::CloudController end describe '#validation' do - context 'with no buildpacks' do - let(:lifecycle_request_data) { {} } + context 'with unknown admin buildpack' do + let(:lifecycle_request_data) { { buildpacks: %w[foo] } } it 'invalid' do expect(lifecycle.valid?).to be(false) @@ -113,7 +113,12 @@ module VCAP::CloudController end context 'with buildpacks' do - let(:lifecycle_request_data) { { buildpacks: %w[foo bar] } } + before do + Buildpack.make(name: 'foo', lifecycle: 'cnb') + Buildpack.make(name: 'bar', lifecycle: 'cnb') + end + + let(:lifecycle_request_data) { { buildpacks: ['foo', 'bar', 'docker://nodejs:latest'] } } it 'valid' do expect(lifecycle.valid?).to be(true) diff --git a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb index 6a0a1cbf214..4810594bdf5 100644 --- a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb +++ b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb @@ -9,9 +9,11 @@ module VCAP::CloudController let(:tmpdir) { Dir.mktmpdir } let(:filename) { 'file.zip' } + let(:tgz_filename) { 'file.tgz' } let(:sha_valid_zip) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_zip) } let(:sha_valid_zip2) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_zip2) } + let(:sha_valid_tgz) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_tgz) } let(:valid_zip_manifest_stack) { nil } let(:valid_zip) do @@ -34,6 +36,13 @@ module VCAP::CloudController Rack::Test::UploadedFile.new(zip_file) end + let(:valid_tgz) do + tgz_name = File.join(tmpdir, tgz_filename) + TestTgz.create(tgz_name, 3, 1024) + tgz_file = File.new(tgz_name) + Rack::Test::UploadedFile.new(tgz_file) + end + let(:staging_timeout) { TestConfig.config_instance.get(:staging, :timeout_in_seconds) } let(:expected_sha_valid_zip) { "#{buildpack.guid}_#{sha_valid_zip}" } @@ -142,6 +151,17 @@ module VCAP::CloudController end end + context 'lifecycle: cnb' do + let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_cnb_buildpack', stack: 'cider', position: 0, lifecycle: 'cnb' }) } + let(:expected_sha_valid_tgz) { "#{buildpack.guid}_#{sha_valid_tgz}" } + + it 'uploads' do + expect(buildpack_blobstore).to receive(:cp_to_blobstore).with(valid_tgz, expected_sha_valid_tgz) + + upload_buildpack.upload_buildpack(buildpack, valid_tgz, tgz_filename) + end + end + it 'updates the buildpack filename' do expect do upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) diff --git a/spec/unit/messages/buildpack_create_message_spec.rb b/spec/unit/messages/buildpack_create_message_spec.rb index 6ba6501d35a..36f766a63fa 100644 --- a/spec/unit/messages/buildpack_create_message_spec.rb +++ b/spec/unit/messages/buildpack_create_message_spec.rb @@ -156,6 +156,17 @@ module VCAP::CloudController it { is_expected.to be_valid } end end + + describe 'lifecycle' do + context 'when the lifecycle is invalid' do + let(:params) { { name: 'cnb-test', enabled: true, lifecycle: 'foo' } } + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:lifecycle]).to include('must be either "buildpack" or "cnb"') + end + end + end end end end diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index de9454a5ee1..9e66f2d0351 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -104,7 +104,8 @@ def ordered_buildpacks Timecop.return end - subject(:cnb_buildpacks) { Buildpack.list_admin_buildpacks(nil, 'cnb') } + let(:cnb_buildpacks) { Buildpack.list_admin_buildpacks(nil, 'cnb') } + subject(:all_buildpacks) { Buildpack.list_admin_buildpacks } context 'with prioritized buildpacks' do @@ -191,7 +192,7 @@ def ordered_buildpacks end context 'with a stack' do - subject(:cnb_buildpacks) { Buildpack.list_admin_buildpacks('stack2', 'cnb') } + let(:cnb_buildpacks) { Buildpack.list_admin_buildpacks('stack2', 'cnb') } subject(:all_buildpacks) { Buildpack.list_admin_buildpacks('stack1') } let!(:stack1) { Stack.make(name: 'stack1') } let!(:stack2) { Stack.make(name: 'stack2') } @@ -219,7 +220,6 @@ def ordered_buildpacks it 'returns the list of cnb buildpacks' do expect(cnb_buildpacks.collect(&:key)).to eq(['cnb key']) end - end end diff --git a/spec/unit/models/runtime/cnb_lifecycle_data_model_spec.rb b/spec/unit/models/runtime/cnb_lifecycle_data_model_spec.rb index 8fea07fdf29..c7e5dd8ad23 100644 --- a/spec/unit/models/runtime/cnb_lifecycle_data_model_spec.rb +++ b/spec/unit/models/runtime/cnb_lifecycle_data_model_spec.rb @@ -199,7 +199,7 @@ module VCAP::CloudController expect(lifecycle_data.valid?).to be(false) expect(lifecycle_data.errors.full_messages.size).to eq(1) - expect(lifecycle_data.errors.full_messages.first).to include('Specified invalid buildpack URL: "invalid_buildpack_name"') + expect(lifecycle_data.errors.full_messages.first).to include('Specified unknown buildpack name: "invalid_buildpack_name"') end it 'is valid' do From 88b18d74677c599c8f5c76f3be8cf21cdbe45e37 Mon Sep 17 00:00:00 2001 From: Ralf Pannemans Date: Tue, 23 Jul 2024 13:54:48 +0200 Subject: [PATCH 07/20] set --auto-detect flag when no buildpacks were requested Co-authored-by: Pavel Busko --- lib/cloud_controller/diego/cnb/lifecycle_data.rb | 8 +++++--- .../diego/cnb/lifecycle_protocol.rb | 1 + .../diego/cnb/staging_action_builder.rb | 3 ++- .../diego/lifecycles/lifecycle_base.rb | 2 +- .../diego/cnb/lifecycle_data_spec.rb | 6 ++++-- .../diego/cnb/lifecycle_protocol_spec.rb | 13 ++++++++++--- .../diego/cnb/staging_action_builder_spec.rb | 16 ++++++++++------ .../diego/lifecycles/cnb_lifecycle_spec.rb | 9 +++++++-- 8 files changed, 40 insertions(+), 18 deletions(-) diff --git a/lib/cloud_controller/diego/cnb/lifecycle_data.rb b/lib/cloud_controller/diego/cnb/lifecycle_data.rb index c238c863322..63ff1fd642c 100644 --- a/lib/cloud_controller/diego/cnb/lifecycle_data.rb +++ b/lib/cloud_controller/diego/cnb/lifecycle_data.rb @@ -4,7 +4,7 @@ module CNB class LifecycleData attr_accessor :app_bits_download_uri, :build_artifacts_cache_download_uri, :build_artifacts_cache_upload_uri, :buildpacks, :app_bits_checksum, :droplet_upload_uri, :stack, :buildpack_cache_checksum, - :credentials + :credentials, :auto_detect def message message = { @@ -13,7 +13,8 @@ def message droplet_upload_uri:, buildpacks:, stack:, - app_bits_checksum: + app_bits_checksum:, + auto_detect: } message[:build_artifacts_cache_download_uri] = build_artifacts_cache_download_uri if build_artifacts_cache_download_uri message[:buildpack_cache_checksum] = buildpack_cache_checksum if buildpack_cache_checksum @@ -36,7 +37,8 @@ def schema buildpacks: Array, stack: String, app_bits_checksum: Hash, - optional(:credentials) => String + optional(:credentials) => String, + auto_detect: bool } end end diff --git a/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb b/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb index bc65ea521a4..ae095673b8b 100644 --- a/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb +++ b/lib/cloud_controller/diego/cnb/lifecycle_protocol.rb @@ -25,6 +25,7 @@ def desired_lrp_builder(config, process) def new_lifecycle_data(staging_details) lifecycle_data = LifecycleData.new lifecycle_data.credentials = staging_details.lifecycle.credentials + lifecycle_data.auto_detect = staging_details.lifecycle.buildpack_infos.empty? lifecycle_data end diff --git a/lib/cloud_controller/diego/cnb/staging_action_builder.rb b/lib/cloud_controller/diego/cnb/staging_action_builder.rb index 675ea28c350..439c57d7fd8 100644 --- a/lib/cloud_controller/diego/cnb/staging_action_builder.rb +++ b/lib/cloud_controller/diego/cnb/staging_action_builder.rb @@ -30,9 +30,10 @@ def stage_action '--cache-output', '/tmp/cache-output.tgz' ] + args.push('--auto-detect') if lifecycle_data[:auto_detect] lifecycle_data[:buildpacks].each do |buildpack| args.push('--buildpack', buildpack[:url]) if buildpack[:name] == 'custom' - args.push('--system-buildpack', buildpack[:key]) unless buildpack[:name] == 'custom' + args.push('--buildpack', buildpack[:key]) unless buildpack[:name] == 'custom' end env_vars = BbsEnvironmentBuilder.build(staging_details.environment_variables) diff --git a/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb b/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb index 4a691418c1d..671d0bd3d92 100644 --- a/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb +++ b/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb @@ -9,7 +9,7 @@ def initialize(package, staging_message) @staging_message = staging_message @package = package - db_result = BuildpackLifecycleFetcher.fetch(buildpacks_to_use, staging_stack) + db_result = BuildpackLifecycleFetcher.fetch(buildpacks_to_use, staging_stack, type) @buildpack_infos = db_result[:buildpack_infos] @validator = BuildpackLifecycleDataValidator.new({ buildpack_infos: buildpack_infos, stack: db_result[:stack] }) end diff --git a/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_data_spec.rb b/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_data_spec.rb index ebb24b9384d..d584a338a8a 100644 --- a/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_data_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_data_spec.rb @@ -16,6 +16,7 @@ module CNB data.buildpack_cache_checksum = 'bp-cache-checksum' data.app_bits_checksum = { type: 'sha256', value: 'package-checksum' } data.credentials = '{"registry":{"username":"password"}}' + data.auto_detect = false data end @@ -29,7 +30,8 @@ module CNB stack: 'stack', buildpack_cache_checksum: 'bp-cache-checksum', app_bits_checksum: { type: 'sha256', value: 'package-checksum' }, - credentials: '{"registry":{"username":"password"}}' + credentials: '{"registry":{"username":"password"}}', + auto_detect: false } end @@ -98,7 +100,7 @@ module CNB expect do data.message end.to raise_error( - Membrane::SchemaValidationError, /{ #{key} => Expected instance of (String|Array|Hash), given an instance of NilClass }/ + Membrane::SchemaValidationError, /{ #{key} => Expected instance of (String|Array|Hash|true or false), given .*}/ ) end end diff --git a/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_protocol_spec.rb b/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_protocol_spec.rb index 2fcd24b6cc9..6f8cac99972 100644 --- a/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_protocol_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/cnb/lifecycle_protocol_spec.rb @@ -77,6 +77,16 @@ module CNB end end + context 'when buildpack_infos is empty' do + let(:buildpack_infos) { [] } + + it 'sets auto_detect: true' do + lifecycle_data = lifecycle_protocol.lifecycle_data(staging_details) + + expect(lifecycle_data[:auto_detect]).to be true + end + end + context 'when the generated message has invalid data' do let(:buildpack_infos) { [] } @@ -208,9 +218,6 @@ module CNB end it 'creates a diego DesiredLrpBuilder' do - puts 'hello' - puts 'world!' - expect(VCAP::CloudController::Diego::CNB::DesiredLrpBuilder).to receive(:new).with( config, builder_opts diff --git a/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb b/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb index 70688d809e3..3ceb1548cfe 100644 --- a/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/cnb/staging_action_builder_spec.rb @@ -241,7 +241,7 @@ module CNB end end - context('when system-buildpaks are used') do + context('when system-buildpacks are used') do let(:buildpacks) do [ { name: 'node-cnb', key: 'node-key', skip_detect: false }, @@ -249,11 +249,15 @@ module CNB ] end + before do + lifecycle_data[:auto_detect] = true + end + let(:run_staging_action) do ::Diego::Bbs::Models::RunAction.new( path: '/tmp/lifecycle/builder', user: 'vcap', - args: ['--cache-dir', '/tmp/cache', '--cache-output', '/tmp/cache-output.tgz', '--system-buildpack', 'node-key', '--system-buildpack', + args: ['--cache-dir', '/tmp/cache', '--cache-output', '/tmp/cache-output.tgz', '--auto-detect', '--buildpack', 'node-key', '--buildpack', 'java-key', '--pass-env-var', 'FOO', '--pass-env-var', 'BAR'], env: bbs_env ) @@ -269,8 +273,8 @@ module CNB expect(builder.cached_dependencies).to include(::Diego::Bbs::Models::CachedDependency.new( name: 'node-cnb', from: '', - to: '/tmp/buildpacks/54548f35489d6234', - cache_key: 'node-cnb', + to: '/tmp/buildpacks/0dcf6bb539d77cbc', + cache_key: 'node-key', log_source: '', checksum_algorithm: '', checksum_value: '' @@ -278,8 +282,8 @@ module CNB expect(builder.cached_dependencies).to include(::Diego::Bbs::Models::CachedDependency.new( name: 'java-cnb', from: '', - to: '/tmp/buildpacks/c56b3bfdba7aa4dd', - cache_key: 'java-cnb', + to: '/tmp/buildpacks/be0ef1aa1092a6db', + cache_key: 'java-key', log_source: '', checksum_algorithm: '', checksum_value: '' diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb index 1ad626ab78b..0aec41e34b1 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb @@ -16,10 +16,15 @@ module VCAP::CloudController context 'when the user specifies buildpacks' do let(:request_data) do { - buildpacks: %w[docker://cool-buildpack docker://rad-buildpack] + buildpacks: %w[docker://nodejs cool-buildpack] } end + before do + Buildpack.make(name: 'cool-buildpack', lifecycle: 'cnb') + Buildpack.make(name: 'rad-buildpack') + end + it 'uses the buildpacks from the user' do build = BuildModel.make(:cnb) @@ -29,7 +34,7 @@ module VCAP::CloudController data_model = VCAP::CloudController::CNBLifecycleDataModel.last - expect(data_model.buildpacks).to eq(%w[docker://cool-buildpack docker://rad-buildpack]) + expect(data_model.buildpacks).to eq(%w[docker://nodejs cool-buildpack]) expect(data_model.build).to eq(build) end end From 08ab65ac7fc38dcace82ced5f2d084dd73e2776a Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Wed, 24 Jul 2024 14:19:29 +0200 Subject: [PATCH 08/20] Fix buildpack install and tests Co-authored-by: Johannes Dillmann --- app/jobs/runtime/buildpack_installer_factory.rb | 3 +-- app/jobs/runtime/create_buildpack_installer.rb | 2 +- lib/cloud_controller/install_buildpacks.rb | 14 ++++++++++---- spec/request/buildpacks_spec.rb | 7 +++++++ 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/jobs/runtime/buildpack_installer_factory.rb b/app/jobs/runtime/buildpack_installer_factory.rb index 8565e5cf702..2dfbc6f74e9 100644 --- a/app/jobs/runtime/buildpack_installer_factory.rb +++ b/app/jobs/runtime/buildpack_installer_factory.rb @@ -37,8 +37,7 @@ def plan(buildpack_name, manifest_fields) planned_jobs = [] - found_buildpacks = Buildpack.where(name: buildpack_name).all - + found_buildpacks = Buildpack.where(name: buildpack_name, lifecycle: manifest_fields[0][:lifecycle]).all ensure_no_attempt_to_upgrade_a_stackless_locked_buildpack(buildpack_name, found_buildpacks, manifest_fields) manifest_fields.each do |buildpack_fields| diff --git a/app/jobs/runtime/create_buildpack_installer.rb b/app/jobs/runtime/create_buildpack_installer.rb index aefd135544d..5448d49eef4 100644 --- a/app/jobs/runtime/create_buildpack_installer.rb +++ b/app/jobs/runtime/create_buildpack_installer.rb @@ -10,7 +10,7 @@ def perform buildpacks_lock = Locking[name: 'buildpacks'] buildpacks_lock.db.transaction do buildpacks_lock.lock! - buildpack = Buildpack.create(name: name, stack: stack_name) + buildpack = Buildpack.create(name: name, stack: stack_name, lifecycle: options[:lifecycle]) end begin buildpack_uploader.upload_buildpack(buildpack, file, File.basename(file)) diff --git a/lib/cloud_controller/install_buildpacks.rb b/lib/cloud_controller/install_buildpacks.rb index 7986ea157a2..89f8584d6f5 100644 --- a/lib/cloud_controller/install_buildpacks.rb +++ b/lib/cloud_controller/install_buildpacks.rb @@ -18,6 +18,8 @@ def install(buildpacks) buildpacks.each do |bpack| buildpack_opts = bpack.deep_symbolize_keys + buildpack_opts[:lifecycle] = Lifecycles::BUILDPACK if buildpack_opts[:lifecycle].nil? + buildpack_name = buildpack_opts.delete(:name) if buildpack_name.nil? logger.error "A name must be specified for the buildpack_opts: #{buildpack_opts}" @@ -40,13 +42,17 @@ def install(buildpacks) next end - detected_stack = VCAP::CloudController::Buildpacks::StackNameExtractor.extract_from_file(buildpack_file) + detected_stack = VCAP::CloudController::Buildpacks::StackNameExtractor.extract_from_file(buildpack_file) if buildpack_opts[:lifecycle] == Lifecycles::BUILDPACK + detected_stack = buildpack_opts[:stack] if buildpack_opts[:lifecycle] == Lifecycles::CNB + factory_options << { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack } end - buildpacks_by_name = factory_options.group_by { |options| options[:name] } - buildpacks_by_name.each do |name, buildpack_options| - buildpack_install_jobs << job_factory.plan(name, buildpack_options) + buildpacks_by_lifecycle = factory_options.group_by { |options| options[:options][:lifecycle] } + buildpacks_by_lifecycle.each_value do |options| + options.group_by { |opts| opts[:name] }.each do |name, buildpack_options| + buildpack_install_jobs << job_factory.plan(name, buildpack_options) + end end buildpack_install_jobs.flatten! diff --git a/spec/request/buildpacks_spec.rb b/spec/request/buildpacks_spec.rb index 78a0d859bd7..91bb82960ad 100644 --- a/spec/request/buildpacks_spec.rb +++ b/spec/request/buildpacks_spec.rb @@ -118,6 +118,7 @@ 'resources' => [ { 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'name' => buildpack1.name, @@ -140,6 +141,7 @@ }, { 'guid' => buildpack3.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'name' => buildpack3.name, @@ -185,6 +187,7 @@ 'resources' => [ { 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'name' => buildpack1.name, @@ -230,6 +233,7 @@ 'resources' => [ { 'guid' => buildpack3.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'name' => buildpack3.name, @@ -252,6 +256,7 @@ }, { 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'name' => buildpack1.name, @@ -360,6 +365,7 @@ 'enabled' => params[:enabled], 'locked' => params[:locked], 'guid' => buildpack.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'metadata' => { @@ -477,6 +483,7 @@ 'enabled' => buildpack.enabled, 'locked' => buildpack.locked, 'guid' => buildpack.guid, + 'lifecycle' => 'buildpack', 'created_at' => iso8601, 'updated_at' => iso8601, 'metadata' => { 'labels' => {}, 'annotations' => {} }, From 03ccd80b488101ee86b06c74e1ab2a1bee616dda Mon Sep 17 00:00:00 2001 From: Johannes Dillmann Date: Wed, 24 Jul 2024 15:33:03 +0200 Subject: [PATCH 09/20] Fix tests Co-authored-by: Pavel Busko --- .../runtime/buildpacks_controller.rb | 3 +- lib/cloud_controller/install_buildpacks.rb | 32 +++++++++++-------- .../runtime/buildpacks_controller_spec.rb | 3 +- .../v3/buildpacks_controller_spec.rb | 6 ++-- spec/unit/jobs/deserialization_spec.rb | 1 + .../install_buildpacks_spec.rb | 11 ++++--- .../messages/buildpack_upload_message_spec.rb | 20 ++++++++++-- 7 files changed, 51 insertions(+), 25 deletions(-) diff --git a/app/controllers/runtime/buildpacks_controller.rb b/app/controllers/runtime/buildpacks_controller.rb index f13ae080c44..936c077e900 100644 --- a/app/controllers/runtime/buildpacks_controller.rb +++ b/app/controllers/runtime/buildpacks_controller.rb @@ -7,12 +7,13 @@ def self.dependencies define_attributes do attribute :name, String attribute :stack, String, default: nil + attribute :lifecycle, String, default: Lifecycles::BUILDPACK, exclude_in: :update attribute :position, Integer, default: 0 attribute :enabled, Message::Boolean, default: true attribute :locked, Message::Boolean, default: false end - query_parameters :name, :stack + query_parameters :name, :stack, :lifecycle def self.translate_validation_exception(e, attributes) buildpack_errors = e.errors.on(%i[name stack]) diff --git a/lib/cloud_controller/install_buildpacks.rb b/lib/cloud_controller/install_buildpacks.rb index 89f8584d6f5..1dbd8cb8c50 100644 --- a/lib/cloud_controller/install_buildpacks.rb +++ b/lib/cloud_controller/install_buildpacks.rb @@ -12,8 +12,6 @@ def install(buildpacks) CloudController::DependencyLocator.instance.buildpack_blobstore.ensure_bucket_exists job_factory = VCAP::CloudController::Jobs::Runtime::BuildpackInstallerFactory.new - buildpack_install_jobs = [] - factory_options = [] buildpacks.each do |bpack| buildpack_opts = bpack.deep_symbolize_keys @@ -41,19 +39,10 @@ def install(buildpacks) logger.error "File not found: #{buildpack_file}, for the buildpack_opts: #{bpack}" next end - - detected_stack = VCAP::CloudController::Buildpacks::StackNameExtractor.extract_from_file(buildpack_file) if buildpack_opts[:lifecycle] == Lifecycles::BUILDPACK - detected_stack = buildpack_opts[:stack] if buildpack_opts[:lifecycle] == Lifecycles::CNB - - factory_options << { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack } + factory_options << { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack(buildpack_file, buildpack_opts) } end - buildpacks_by_lifecycle = factory_options.group_by { |options| options[:options][:lifecycle] } - buildpacks_by_lifecycle.each_value do |options| - options.group_by { |opts| opts[:name] }.each do |name, buildpack_options| - buildpack_install_jobs << job_factory.plan(name, buildpack_options) - end - end + buildpack_install_jobs = generate_install_jobs(factory_options, job_factory) buildpack_install_jobs.flatten! run_canary(buildpack_install_jobs) @@ -66,6 +55,23 @@ def logger private + def generate_install_jobs(factory_options, job_factory) + buildpack_install_jobs = [] + buildpacks_by_lifecycle = factory_options.group_by { |options| options[:options][:lifecycle] } + buildpacks_by_lifecycle.each_value do |options| + options.group_by { |opts| opts[:name] }.each do |name, buildpack_options| + buildpack_install_jobs << job_factory.plan(name, buildpack_options) + end + end + buildpack_install_jobs + end + + def detected_stack(file, opts) + return opts[:stack] if opts[:lifecycle] == Lifecycles::CNB + + VCAP::CloudController::Buildpacks::StackNameExtractor.extract_from_file(file) + end + def buildpack_zip(package, zipfile) return zipfile if zipfile diff --git a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb index 80c819a77b7..748fd8450d0 100644 --- a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb @@ -25,7 +25,8 @@ def ordered_buildpacks stack: { type: 'string' }, position: { type: 'integer', default: 0 }, enabled: { type: 'bool', default: true }, - locked: { type: 'bool', default: false } + locked: { type: 'bool', default: false }, + lifecycle: { type: 'string', default: 'buildpack' } }) end diff --git a/spec/unit/controllers/v3/buildpacks_controller_spec.rb b/spec/unit/controllers/v3/buildpacks_controller_spec.rb index bb6e280eaf3..3cdd0001bf6 100644 --- a/spec/unit/controllers/v3/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/v3/buildpacks_controller_spec.rb @@ -512,8 +512,10 @@ let(:buildpack_bits_name) { 'buildpack.zip' } before do - allow(File).to receive(:stat).and_return(stat_double) - # allow(VCAP::CloudController::BuildpackUpload).to receive(:new).and_return(uploader) + allow(File).to receive_messages( + stat: stat_double, + read: "PK\x03\x04".force_encoding('binary') + ) end describe 'permissions' do diff --git a/spec/unit/jobs/deserialization_spec.rb b/spec/unit/jobs/deserialization_spec.rb index c8d15dedc47..31b350a3e41 100644 --- a/spec/unit/jobs/deserialization_spec.rb +++ b/spec/unit/jobs/deserialization_spec.rb @@ -178,6 +178,7 @@ module Jobs - :lifecycle extra_keys: [] lifecycle: + :type: buildpack :data: :buildpacks: - ruby diff --git a/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb b/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb index 7cb68107abe..e3b7d9f8320 100644 --- a/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb +++ b/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb @@ -46,13 +46,13 @@ module VCAP::CloudController let(:buildpack2_file) { 'abuildpack2.zip' } let(:buildpack1a_fields) do - { name: 'buildpack1', file: buildpack1a_file, stack: 'cflinuxfs11', options: {} } + { name: 'buildpack1', file: buildpack1a_file, stack: 'cflinuxfs11', options: { lifecycle: Lifecycles::BUILDPACK } } end let(:buildpack1b_fields) do - { name: 'buildpack1', file: buildpack1b_file, stack: 'cflinuxfs12', options: {} } + { name: 'buildpack1', file: buildpack1b_file, stack: 'cflinuxfs12', options: { lifecycle: Lifecycles::BUILDPACK } } end let(:buildpack2_fields) do - { name: 'buildpack2', file: buildpack2_file, stack: nil, options: {} } + { name: 'buildpack2', file: buildpack2_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } } end before do @@ -153,7 +153,7 @@ module VCAP::CloudController # call install # verify that job_factory.plan was called with the right file expect(File).to receive(:file?).with('another.zip').and_return(true) - expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: {} }]) + expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } }]) installer.install(TestConfig.config_instance.get(:install_buildpacks)) end @@ -167,7 +167,7 @@ module VCAP::CloudController it 'succeeds when no package is specified' do TestConfig.config[:install_buildpacks][0].delete('package') expect(File).to receive(:file?).with('another.zip').and_return(true) - expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: {} }]) + expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } }]) installer.install(TestConfig.config_instance.get(:install_buildpacks)) end @@ -225,6 +225,7 @@ module VCAP::CloudController stack: nil, options: { enabled: true, + lifecycle: Lifecycles::BUILDPACK, locked: false, position: 5 } }]) diff --git a/spec/unit/messages/buildpack_upload_message_spec.rb b/spec/unit/messages/buildpack_upload_message_spec.rb index 16505280363..7c295b783ac 100644 --- a/spec/unit/messages/buildpack_upload_message_spec.rb +++ b/spec/unit/messages/buildpack_upload_message_spec.rb @@ -9,7 +9,10 @@ module VCAP::CloudController let(:stat_double) { instance_double(File::Stat, size: 2) } before do - allow(File).to receive(:stat).and_return(stat_double) + allow(File).to receive_messages( + stat: stat_double, + read: "PK\x03\x04".force_encoding('binary') + ) end context 'when the param is set' do @@ -93,12 +96,23 @@ module VCAP::CloudController end context 'when the file is not a zip' do - let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.abcd' } } it 'is not valid' do + allow(File).to receive(:read).and_return("PX\x03\x04".force_encoding('binary')) upload_message = BuildpackUploadMessage.new(opts) expect(upload_message).not_to be_valid - expect(upload_message.errors.full_messages[0]).to include('buildpack.tgz is not a zip') + expect(upload_message.errors.full_messages[0]).to include('buildpack.abcd is not a zip or gzip') + end + end + + context 'when the file is a tgz' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } + + it 'is not valid' do + allow(File).to receive(:read).and_return("\x1F\x8B\x08".force_encoding('binary')) + upload_message = BuildpackUploadMessage.new(opts) + expect(upload_message).to be_valid end end From a661da67fba127d69995d888cd8cfdecf3faa957 Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Wed, 24 Jul 2024 16:17:53 +0200 Subject: [PATCH 10/20] ensure the default lifecycle is set for buildpack model Co-authored-by: Johannes Dillmann --- app/actions/buildpack_update.rb | 5 ++++- app/jobs/runtime/buildpack_installer_factory.rb | 2 +- app/models/runtime/buildpack.rb | 14 +++++++++++++- spec/unit/actions/app_apply_manifest_spec.rb | 1 + spec/unit/actions/buildpack_update_spec.rb | 4 ++-- .../runtime/buildpack_installer_factory_spec.rb | 2 +- .../diego/lifecycles/buildpack_lifecycle_spec.rb | 2 +- .../diego/lifecycles/cnb_lifecycle_spec.rb | 11 +++++++---- spec/unit/models/runtime/buildpack_spec.rb | 16 +++++++++++++--- 9 files changed, 43 insertions(+), 14 deletions(-) diff --git a/app/actions/buildpack_update.rb b/app/actions/buildpack_update.rb index 791dd281703..b0b9b8674c6 100644 --- a/app/actions/buildpack_update.rb +++ b/app/actions/buildpack_update.rb @@ -27,7 +27,10 @@ def validation_error!(error, message) error!(%(Stack '#{message.stack}' does not exist)) if error.errors.on(:stack)&.include?(:buildpack_stack_does_not_exist) error!(%(Buildpack stack cannot be changed)) if error.errors.on(:stack)&.include?(:buildpack_cant_change_stacks) error!(%(Buildpack with name '#{error.model.name}' and an unassigned stack already exists)) if error.errors.on(:stack)&.include?(:unique) - error!(%(Buildpack with name '#{error.model.name}' and stack '#{error.model.stack}' already exists)) if error.errors.on(%i[name stack])&.include?(:unique) + + if error.errors.on(%i[name stack lifecycle])&.include?(:unique) + error!(%(Buildpack with name '#{error.model.name}', stack '#{error.model.stack}' and lifecycle '#{error.model.lifecycle}' already exists)) + end error!(error.message) end diff --git a/app/jobs/runtime/buildpack_installer_factory.rb b/app/jobs/runtime/buildpack_installer_factory.rb index 2dfbc6f74e9..fd03d1ef91b 100644 --- a/app/jobs/runtime/buildpack_installer_factory.rb +++ b/app/jobs/runtime/buildpack_installer_factory.rb @@ -37,7 +37,7 @@ def plan(buildpack_name, manifest_fields) planned_jobs = [] - found_buildpacks = Buildpack.where(name: buildpack_name, lifecycle: manifest_fields[0][:lifecycle]).all + found_buildpacks = Buildpack.where(name: buildpack_name, lifecycle: manifest_fields[0][:options][:lifecycle]).all ensure_no_attempt_to_upgrade_a_stackless_locked_buildpack(buildpack_name, found_buildpacks, manifest_fields) manifest_fields.each do |buildpack_fields| diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index ee3378e5f29..483dd97a7b0 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -3,6 +3,7 @@ module VCAP::CloudController class Buildpack < Sequel::Model plugin :list, scope: :lifecycle + plugin :after_initialize export_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle import_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle, :key @@ -12,6 +13,10 @@ class Buildpack < Sequel::Model READY_STATE = 'READY'.freeze ].map(&:freeze).freeze + def after_initialize + self.lifecycle ||= Lifecycles::BUILDPACK + end + one_to_many :labels, class: 'VCAP::CloudController::BuildpackLabelModel', key: :resource_guid, primary_key: :guid one_to_many :annotations, class: 'VCAP::CloudController::BuildpackAnnotationModel', key: :resource_guid, primary_key: :guid @@ -39,12 +44,13 @@ def self.at_last_position end def validate - validates_unique %i[name stack] + validates_unique %i[name stack lifecycle] validates_format(/\A(\w|-)+\z/, :name, message: 'can only contain alphanumeric characters') validate_stack_existence validate_stack_change validate_multiple_nil_stacks + validate_lifecycle_change end def locked? @@ -87,6 +93,12 @@ def validate_stack_change errors.add(:stack, :buildpack_cant_change_stacks) if column_changes.key?(:stack) end + def validate_lifecycle_change + return if initial_value(:lifecycle).nil? + + errors.add(:lifecycle, :buildpack_cant_change_lifecycle) if column_changes.key?(:lifecycle) + end + def validate_stack_existence return unless stack diff --git a/spec/unit/actions/app_apply_manifest_spec.rb b/spec/unit/actions/app_apply_manifest_spec.rb index 8e9d43e6bd7..f0761d7cfab 100644 --- a/spec/unit/actions/app_apply_manifest_spec.rb +++ b/spec/unit/actions/app_apply_manifest_spec.rb @@ -119,6 +119,7 @@ module VCAP::CloudController end describe 'using cnb type' do + let(:message) { AppManifestMessage.create_from_yml({ name: 'blah', buildpack: buildpack.name, lifecycle: 'cnb' }) } let(:app) { AppModel.make(:cnb) } it 'calls AppUpdate with the correct arguments' do diff --git a/spec/unit/actions/buildpack_update_spec.rb b/spec/unit/actions/buildpack_update_spec.rb index 0c6f4ef94aa..5958e3bcfda 100644 --- a/spec/unit/actions/buildpack_update_spec.rb +++ b/spec/unit/actions/buildpack_update_spec.rb @@ -120,13 +120,13 @@ module VCAP::CloudController end end - it 'raises a human-friendly error when name and stack conflict' do + it 'raises a human-friendly error when name, stack and lifecycle conflict' do expect(buildpack1.stack).to eq buildpack2.stack message = BuildpackUpdateMessage.new(name: buildpack1.name) expect do BuildpackUpdate.new.update(buildpack2, message) - end.to raise_error(BuildpackUpdate::Error, "Buildpack with name '#{buildpack1.name}' and stack '#{buildpack1.stack}' already exists") + end.to raise_error(BuildpackUpdate::Error, "Buildpack with name '#{buildpack1.name}', stack '#{buildpack1.stack}' and lifecycle '#{buildpack1.lifecycle}' already exists") end it 're-raises when there is an unknown error' do diff --git a/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb b/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb index 481eaca0171..144b640f442 100644 --- a/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb +++ b/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb @@ -6,7 +6,7 @@ module Jobs::Runtime describe '#plan' do let(:name) { 'the-buildpack' } let(:file) { 'the-file' } - let(:opts) { { enabled: true, locked: false, position: 1 } } + let(:opts) { { enabled: true, locked: false, position: 1, lifecycle: 'buildpack' } } let(:factory) { BuildpackInstallerFactory.new } let(:jobs) { factory.plan(name, buildpack_fields) } 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 464890abf19..44a4e27508d 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 @@ -169,7 +169,7 @@ module VCAP::CloudController it 'returns the expected value' do expect(buildpack_lifecycle.buildpack_infos).to eq(stubbed_data[:buildpack_infos]) - expect(BuildpackLifecycleFetcher).to have_received(:fetch).with(%w[cool-buildpack rad-buildpack], Stack.default.name) + expect(BuildpackLifecycleFetcher).to have_received(:fetch).with(%w[cool-buildpack rad-buildpack], Stack.default.name, 'buildpack') end end diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb index 0aec41e34b1..96dcac5ef86 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/cnb_lifecycle_spec.rb @@ -171,15 +171,18 @@ module VCAP::CloudController let(:stubbed_data) { { stack: app.lifecycle_data.stack, buildpack_infos: [instance_double(BuildpackInfo)] } } let(:request_data) do { - buildpacks: %w[docker://cool-buildpack docker://rad-buildpack] + buildpacks: %w[cool-buildpack docker://rad-buildpack] } end + before do + allow(BuildpackLifecycleFetcher).to receive(:fetch).and_return(stubbed_data) + end + it 'returns the expected value' do - expect(cnb_lifecycle.buildpack_infos).to have(2).items + expect(cnb_lifecycle.buildpack_infos).to eq(stubbed_data[:buildpack_infos]) - expect(cnb_lifecycle.buildpack_infos[0].buildpack_url).to eq('docker://cool-buildpack') - expect(cnb_lifecycle.buildpack_infos[1].buildpack_url).to eq('docker://rad-buildpack') + expect(BuildpackLifecycleFetcher).to have_received(:fetch).with(%w[cool-buildpack docker://rad-buildpack], app.lifecycle_data.stack, 'cnb') end end end diff --git a/spec/unit/models/runtime/buildpack_spec.rb b/spec/unit/models/runtime/buildpack_spec.rb index 9e66f2d0351..200c8645a13 100644 --- a/spec/unit/models/runtime/buildpack_spec.rb +++ b/spec/unit/models/runtime/buildpack_spec.rb @@ -9,7 +9,7 @@ def ordered_buildpacks it { is_expected.to have_timestamp_columns } describe 'Validations' do - it { is_expected.to validate_uniqueness %i[name stack] } + it { is_expected.to validate_uniqueness %i[name stack lifecycle] } describe 'stack' do let(:stack) { Stack.make(name: 'happy') } @@ -74,11 +74,21 @@ def ordered_buildpacks end end end + + describe 'lifecycle' do + it 'cannot be changed once it is set' do + buildpack = Buildpack.create(name: 'test', lifecycle: 'cnb') + buildpack.lifecycle = 'buildpack' + + expect(buildpack).not_to be_valid + expect(buildpack.errors.on(:lifecycle)).to include(:buildpack_cant_change_lifecycle) + end + end end describe 'Serialization' do - 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 { is_expected.to export_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle } + it { is_expected.to import_attributes :name, :stack, :position, :enabled, :locked, :filename, :lifecycle, :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"' From 6942f9c8b1e271b6cbecf5827d8f1ff745a6b5ed Mon Sep 17 00:00:00 2001 From: Ralf Pannemans Date: Thu, 25 Jul 2024 10:40:55 +0200 Subject: [PATCH 11/20] fix buildpack uniqness check and manifest message Co-authored-by: Pavel Busko --- app/actions/buildpack_create.rb | 4 +++- app/controllers/runtime/buildpacks_controller.rb | 4 ++-- app/messages/app_manifest_message.rb | 2 +- app/models/runtime/buildpack.rb | 2 +- errors/v2.yml | 4 ++-- lib/cloud_controller/upload_buildpack.rb | 6 ++++-- spec/unit/actions/buildpack_create_spec.rb | 2 +- spec/unit/lib/cloud_controller/upload_buildpack_spec.rb | 3 ++- spec/unit/messages/app_manifest_message_spec.rb | 5 ++--- 9 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/actions/buildpack_create.rb b/app/actions/buildpack_create.rb index 1dee200dd7f..ffdf3d59aee 100644 --- a/app/actions/buildpack_create.rb +++ b/app/actions/buildpack_create.rb @@ -29,7 +29,9 @@ def create(message) def validation_error!(error, create_message) error!(%(Stack '#{create_message.stack}' does not exist)) if error.errors.on(:stack)&.include?(:buildpack_stack_does_not_exist) - error!(%(Buildpack with name '#{error.model.name}' and stack '#{error.model.stack}' already exists)) if error.errors.on(%i[name stack])&.include?(:unique) + if error.errors.on(%i[name stack lifecycle])&.include?(:unique) + error!(%(Buildpack with name '#{error.model.name}', stack '#{error.model.stack}' and lifecycle '#{error.model.lifecycle}' already exists)) + end error!(%(Buildpack with name '#{error.model.name}' and an unassigned stack already exists)) if error.errors.on(:stack)&.include?(:unique) error!(error.message) diff --git a/app/controllers/runtime/buildpacks_controller.rb b/app/controllers/runtime/buildpacks_controller.rb index 936c077e900..51af5098010 100644 --- a/app/controllers/runtime/buildpacks_controller.rb +++ b/app/controllers/runtime/buildpacks_controller.rb @@ -16,9 +16,9 @@ def self.dependencies query_parameters :name, :stack, :lifecycle def self.translate_validation_exception(e, attributes) - buildpack_errors = e.errors.on(%i[name stack]) + buildpack_errors = e.errors.on(%i[name stack lifecycle]) if buildpack_errors && buildpack_errors.include?(:unique) - CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', attributes['name'], attributes['stack']) + CloudController::Errors::ApiError.new_from_details('BuildpackNameStackLifecycleTaken', attributes['name'], attributes['stack'], attributes['lifecycle']) else CloudController::Errors::ApiError.new_from_details('BuildpackInvalid', e.errors.full_messages) end diff --git a/app/messages/app_manifest_message.rb b/app/messages/app_manifest_message.rb index 6a8550e4d7d..ee4757fd13f 100644 --- a/app/messages/app_manifest_message.rb +++ b/app/messages/app_manifest_message.rb @@ -127,7 +127,7 @@ def app_lifecycle_hash cnb_lifecycle_data elsif requested?(:docker) docker_lifecycle_data - else + elsif requested?(:buildpacks) || requested?(:buildpack) || requested?(:stack) buildpacks_lifecycle_data end diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 483dd97a7b0..0b725cf091a 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -84,7 +84,7 @@ def state def validate_multiple_nil_stacks return unless stack.nil? - errors.add(:stack, :unique) if Buildpack.exclude(guid:).where(name: name, stack: nil).present? + errors.add(:stack, :unique) if Buildpack.exclude(guid:).where(name: name, stack: nil, lifecycle: lifecycle).present? end def validate_stack_change diff --git a/errors/v2.yml b/errors/v2.yml index e57c6b48dac..cf4bf28bea2 100644 --- a/errors/v2.yml +++ b/errors/v2.yml @@ -964,9 +964,9 @@ message: "Encountered an error while attempting to sync cloud controller with the service broker's catalog: %s" 290000: - name: BuildpackNameStackTaken + name: BuildpackNameStackLifecycleTaken http_code: 422 - message: "The buildpack name %s is already in use for the stack %s" + message: "The buildpack name %s is already in use for the stack %s and the lifecycle %s" 290001: name: BuildpackNameTaken diff --git a/lib/cloud_controller/upload_buildpack.rb b/lib/cloud_controller/upload_buildpack.rb index 2ccb2b9f86d..d435e5ef519 100644 --- a/lib/cloud_controller/upload_buildpack.rb +++ b/lib/cloud_controller/upload_buildpack.rb @@ -56,8 +56,10 @@ def upload_buildpack(buildpack, bits_file_path, new_filename) private def raise_translated_api_error(buildpack) - raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackTaken', buildpack.name, buildpack.stack) if buildpack.errors.on(%i[name stack]).try(:include?, - :unique) + if buildpack.errors.on(%i[name stack lifecycle]).try(:include?, :unique) + raise CloudController::Errors::ApiError.new_from_details('BuildpackNameStackLifecycleTaken', buildpack.name, buildpack.stack, buildpack.lifecycle) + end + raise CloudController::Errors::ApiError.new_from_details('BuildpackStacksDontMatch', buildpack.stack, buildpack.initial_value(:stack)) if buildpack.errors.on(:stack).try( :include?, :buildpack_cant_change_stacks ) diff --git a/spec/unit/actions/buildpack_create_spec.rb b/spec/unit/actions/buildpack_create_spec.rb index 1bcc2d7e4bc..c9f01d6471c 100644 --- a/spec/unit/actions/buildpack_create_spec.rb +++ b/spec/unit/actions/buildpack_create_spec.rb @@ -178,7 +178,7 @@ module VCAP::CloudController message = BuildpackCreateMessage.new(name: name, stack: 'the-stack') expect do BuildpackCreate.new.create(message) - end.to raise_error(BuildpackCreate::Error, "Buildpack with name 'the-name' and stack 'the-stack' already exists") + end.to raise_error(BuildpackCreate::Error, "Buildpack with name 'the-name', stack 'the-stack' and lifecycle 'buildpack' already exists") end end end diff --git a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb index 4810594bdf5..67d93a1ea4c 100644 --- a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb +++ b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb @@ -131,7 +131,8 @@ module VCAP::CloudController VCAP::CloudController::Buildpack.create(name: buildpack.name, stack: valid_zip_manifest_stack) expect do upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) - end.to raise_error(CloudController::Errors::ApiError, /The buildpack name #{buildpack.name} is already in use for the stack #{valid_zip_manifest_stack}/) + end.to raise_error(CloudController::Errors::ApiError, + /The buildpack name #{buildpack.name} is already in use for the stack #{valid_zip_manifest_stack} and the lifecycle #{buildpack.lifecycle}/) end end end diff --git a/spec/unit/messages/app_manifest_message_spec.rb b/spec/unit/messages/app_manifest_message_spec.rb index 74a5d59ab3d..3680e2de7bc 100644 --- a/spec/unit/messages/app_manifest_message_spec.rb +++ b/spec/unit/messages/app_manifest_message_spec.rb @@ -1993,11 +1993,10 @@ module VCAP::CloudController context 'when no lifecycle data is requested in the manifest' do let(:parsed_yaml) { {} } - it 'defaults to the buildpack lifecycle' do + it 'does not forward missing attributes to the AppUpdateMessage' do message = AppManifestMessage.create_from_yml(parsed_yaml) - expect(message.app_update_message.requested?(:lifecycle)).to be true - expect(message.app_update_message.lifecycle_type).to eq('buildpack') + expect(message.app_update_message.requested?(:lifecycle)).to be false end end From 8bfacc3a991f510211aabe27deae3f48a0a58f65 Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Thu, 25 Jul 2024 13:58:55 +0200 Subject: [PATCH 12/20] allow filtering buildpack list by lifecycle Co-authored-by: Ralf Pannemans --- app/fetchers/buildpack_list_fetcher.rb | 2 ++ app/messages/buildpacks_list_message.rb | 5 ++++ .../v3/buildpacks_controller_spec.rb | 7 ++++++ .../fetchers/buildpack_list_fetcher_spec.rb | 23 +++++++++++++++++++ .../messages/buildpacks_list_message_spec.rb | 15 ++++++++++-- 5 files changed, 50 insertions(+), 2 deletions(-) diff --git a/app/fetchers/buildpack_list_fetcher.rb b/app/fetchers/buildpack_list_fetcher.rb index c312432df17..40a717d668c 100644 --- a/app/fetchers/buildpack_list_fetcher.rb +++ b/app/fetchers/buildpack_list_fetcher.rb @@ -1,5 +1,6 @@ require 'cloud_controller/paging/sequel_paginator' require 'cloud_controller/paging/paginated_result' +require 'cloud_controller/diego/lifecycles/lifecycles' require 'fetchers/null_filter_query_generator' require 'fetchers/base_list_fetcher' @@ -18,6 +19,7 @@ def filter(message, dataset) dataset = dataset.where(name: message.names) if message.requested?(:names) dataset = NullFilterQueryGenerator.add_filter(dataset, :stack, message.stacks) if message.requested?(:stacks) + dataset = dataset.where(lifecycle: message.requested?(:lifecycle) ? message.lifecycle : Lifecycles::BUILDPACK) if message.requested?(:label_selector) dataset = LabelSelectorQueryGenerator.add_selector_queries( diff --git a/app/messages/buildpacks_list_message.rb b/app/messages/buildpacks_list_message.rb index 24234055310..b0ba8cb5a64 100644 --- a/app/messages/buildpacks_list_message.rb +++ b/app/messages/buildpacks_list_message.rb @@ -5,12 +5,17 @@ class BuildpacksListMessage < MetadataListMessage register_allowed_keys %i[ stacks names + lifecycle page per_page ] validates :names, array: true, allow_nil: true validates :stacks, array: true, allow_nil: true + validates :lifecycle, + string: true, + allow_nil: true, + inclusion: { in: [VCAP::CloudController::Lifecycles::BUILDPACK, VCAP::CloudController::Lifecycles::CNB], message: 'must be either "buildpack" or "cnb"' } validates_with NoAdditionalParamsValidator diff --git a/spec/unit/controllers/v3/buildpacks_controller_spec.rb b/spec/unit/controllers/v3/buildpacks_controller_spec.rb index 3cdd0001bf6..f99d423e528 100644 --- a/spec/unit/controllers/v3/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/v3/buildpacks_controller_spec.rb @@ -54,6 +54,7 @@ let!(:buildpack1) { VCAP::CloudController::Buildpack.make(stack: stack1.name) } let!(:buildpack2) { VCAP::CloudController::Buildpack.make(stack: stack2.name) } + let!(:buildpack3) { VCAP::CloudController::Buildpack.make(stack: stack1.name, lifecycle: 'cnb') } before do set_current_user(user) @@ -66,6 +67,12 @@ expect(parsed_body['resources'].second['guid']).to eq(buildpack2.guid) end + it 'renders a lifecycle filtered list of buildpacks' do + get :index, params: { lifecycle: 'cnb' } + + expect(parsed_body['resources'].first['guid']).to eq(buildpack3.guid) + end + it 'renders a name filtered list of buildpacks' do get :index, params: { names: buildpack2.name } diff --git a/spec/unit/fetchers/buildpack_list_fetcher_spec.rb b/spec/unit/fetchers/buildpack_list_fetcher_spec.rb index df60a1470f6..9f95ea95cf1 100644 --- a/spec/unit/fetchers/buildpack_list_fetcher_spec.rb +++ b/spec/unit/fetchers/buildpack_list_fetcher_spec.rb @@ -14,6 +14,9 @@ module VCAP::CloudController let!(:buildpack2) { Buildpack.make(stack: stack2.name) } let!(:buildpack3) { Buildpack.make(stack: stack3.name) } let!(:buildpack4) { Buildpack.make(stack: stack1.name) } + let!(:buildpack5) { Buildpack.make(stack: stack1.name, lifecycle: 'cnb') } + let!(:buildpack6) { Buildpack.make(stack: stack2.name, lifecycle: 'cnb') } + let!(:buildpack7) { Buildpack.make(stack: nil, lifecycle: 'cnb') } let!(:buildpack_without_stack) { Buildpack.make(stack: nil) } let(:message) { BuildpacksListMessage.from_params(filters) } @@ -73,6 +76,26 @@ module VCAP::CloudController expect(subject).to contain_exactly(buildpack2, buildpack_without_stack) end end + + context 'when filtering by lifecycle' do + let(:filters) do + { 'lifecycle' => 'cnb' } + end + + it 'returns all buildpacks with the cnb lifecycle' do + expect(subject).to contain_exactly(buildpack5, buildpack6, buildpack7) + end + end + + context 'when filtering by lifecycle and stack' do + let(:filters) do + { 'lifecycle' => 'cnb', 'stacks' => stack1.name } + end + + it 'returns all buildpacks with the cnb lifecycle' do + expect(subject).to contain_exactly(buildpack5) + end + end end end end diff --git a/spec/unit/messages/buildpacks_list_message_spec.rb b/spec/unit/messages/buildpacks_list_message_spec.rb index 974641ecf4b..605c4fbac5e 100644 --- a/spec/unit/messages/buildpacks_list_message_spec.rb +++ b/spec/unit/messages/buildpacks_list_message_spec.rb @@ -8,6 +8,7 @@ module VCAP::CloudController { 'names' => 'name1,name2', 'stacks' => 'stack1,stack2', + 'lifecycle' => 'buildpack', 'label_selector' => 'foo=bar', 'page' => 1, 'per_page' => 5 @@ -21,6 +22,7 @@ module VCAP::CloudController expect(message.stacks).to eq(%w[stack1 stack2]) expect(message.names).to eq(%w[name1 name2]) + expect(message.lifecycle).to eq('buildpack') expect(message.label_selector).to eq('foo=bar') expect(message.requirements.first.key).to eq('foo') expect(message.page).to eq(1) @@ -32,6 +34,7 @@ module VCAP::CloudController expect(message).to be_requested(:stacks) expect(message).to be_requested(:names) + expect(message).to be_requested(:lifecycle) expect(message).to be_requested(:label_selector) expect(message).to be_requested(:page) expect(message).to be_requested(:per_page) @@ -43,6 +46,7 @@ module VCAP::CloudController { names: %w[name1 name2], stacks: %w[stack1 stack2], + lifecycle: 'buildpack', label_selector: 'foo=bar', page: 1, per_page: 5 @@ -50,7 +54,7 @@ module VCAP::CloudController end it 'excludes the pagination keys' do - expected_params = %i[names stacks label_selector] + expected_params = %i[names stacks label_selector lifecycle] expect(BuildpacksListMessage.from_params(opts).to_param_hash.keys).to match_array(expected_params) end end @@ -61,7 +65,8 @@ module VCAP::CloudController BuildpacksListMessage.from_params({ names: [], stacks: [], - label_selector: '' + label_selector: '', + lifecycle: 'buildpack' }) end.not_to raise_error end @@ -101,6 +106,12 @@ module VCAP::CloudController and_call_original message.valid? end + + it 'validates lifecycle' do + message = BuildpacksListMessage.from_params lifecycle: 'foo' + expect(message).not_to be_valid + expect(message.errors[:lifecycle].length).to eq 1 + end end end end From 82c9895a6a9beafed82c4e42b4001cff67abe322 Mon Sep 17 00:00:00 2001 From: Ralf Pannemans Date: Thu, 25 Jul 2024 14:56:07 +0200 Subject: [PATCH 13/20] read default lifecycle from config Co-authored-by: Pavel Busko --- app/actions/buildpack_create.rb | 2 +- app/controllers/runtime/buildpacks_controller.rb | 2 +- app/fetchers/buildpack_lifecycle_fetcher.rb | 2 +- app/fetchers/buildpack_list_fetcher.rb | 2 +- app/models/runtime/buildpack.rb | 4 ++-- spec/unit/controllers/runtime/buildpacks_controller_spec.rb | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/actions/buildpack_create.rb b/app/actions/buildpack_create.rb index ffdf3d59aee..387b3344fc0 100644 --- a/app/actions/buildpack_create.rb +++ b/app/actions/buildpack_create.rb @@ -14,7 +14,7 @@ def create(message) buildpack = Buildpack.create( name: message.name, stack: message.stack, - lifecycle: (message.lifecycle.nil? ? VCAP::CloudController::Config.config.get(:default_app_lifecycle) : message.lifecycle), + lifecycle: (message.lifecycle.nil? ? Config.config.get(:default_app_lifecycle) : message.lifecycle), enabled: (message.enabled.nil? ? DEFAULT_ENABLED : message.enabled), locked: (message.locked.nil? ? DEFAULT_LOCKED : message.locked) ) diff --git a/app/controllers/runtime/buildpacks_controller.rb b/app/controllers/runtime/buildpacks_controller.rb index 51af5098010..461cb3b669a 100644 --- a/app/controllers/runtime/buildpacks_controller.rb +++ b/app/controllers/runtime/buildpacks_controller.rb @@ -7,7 +7,7 @@ def self.dependencies define_attributes do attribute :name, String attribute :stack, String, default: nil - attribute :lifecycle, String, default: Lifecycles::BUILDPACK, exclude_in: :update + attribute :lifecycle, String, default: nil, exclude_in: :update attribute :position, Integer, default: 0 attribute :enabled, Message::Boolean, default: true attribute :locked, Message::Boolean, default: false diff --git a/app/fetchers/buildpack_lifecycle_fetcher.rb b/app/fetchers/buildpack_lifecycle_fetcher.rb index 58813d59114..f5d399a64e2 100644 --- a/app/fetchers/buildpack_lifecycle_fetcher.rb +++ b/app/fetchers/buildpack_lifecycle_fetcher.rb @@ -3,7 +3,7 @@ module VCAP::CloudController class BuildpackLifecycleFetcher class << self - def fetch(buildpack_names, stack_name, lifecycle=VCAP::CloudController::Lifecycles::BUILDPACK) + def fetch(buildpack_names, stack_name, lifecycle=Config.config.get(:default_app_lifecycle)) { stack: Stack.find(name: stack_name), buildpack_infos: ordered_buildpacks(buildpack_names, stack_name, lifecycle) diff --git a/app/fetchers/buildpack_list_fetcher.rb b/app/fetchers/buildpack_list_fetcher.rb index 40a717d668c..1ae20e8052c 100644 --- a/app/fetchers/buildpack_list_fetcher.rb +++ b/app/fetchers/buildpack_list_fetcher.rb @@ -19,7 +19,7 @@ def filter(message, dataset) dataset = dataset.where(name: message.names) if message.requested?(:names) dataset = NullFilterQueryGenerator.add_filter(dataset, :stack, message.stacks) if message.requested?(:stacks) - dataset = dataset.where(lifecycle: message.requested?(:lifecycle) ? message.lifecycle : Lifecycles::BUILDPACK) + dataset = dataset.where(lifecycle: message.requested?(:lifecycle) ? message.lifecycle : Config.config.get(:default_app_lifecycle)) if message.requested?(:label_selector) dataset = LabelSelectorQueryGenerator.add_selector_queries( diff --git a/app/models/runtime/buildpack.rb b/app/models/runtime/buildpack.rb index 0b725cf091a..9dec15560c6 100644 --- a/app/models/runtime/buildpack.rb +++ b/app/models/runtime/buildpack.rb @@ -14,7 +14,7 @@ class Buildpack < Sequel::Model ].map(&:freeze).freeze def after_initialize - self.lifecycle ||= Lifecycles::BUILDPACK + self.lifecycle ||= Config.config.get(:default_app_lifecycle) end one_to_many :labels, class: 'VCAP::CloudController::BuildpackLabelModel', key: :resource_guid, primary_key: :guid @@ -27,7 +27,7 @@ def self.user_visibility_filter(_user) full_dataset_filter end - def self.list_admin_buildpacks(stack_name=nil, lifecycle=VCAP::CloudController::Lifecycles::BUILDPACK) + def self.list_admin_buildpacks(stack_name=nil, lifecycle=Config.config.get(:default_app_lifecycle)) scoped = exclude(key: nil).exclude(key: '') scoped = scoped.filter(lifecycle:) if stack_name.present? diff --git a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb index 748fd8450d0..0eecd522bdd 100644 --- a/spec/unit/controllers/runtime/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/runtime/buildpacks_controller_spec.rb @@ -26,7 +26,7 @@ def ordered_buildpacks position: { type: 'integer', default: 0 }, enabled: { type: 'bool', default: true }, locked: { type: 'bool', default: false }, - lifecycle: { type: 'string', default: 'buildpack' } + lifecycle: { type: 'string' } }) end From 264c3a8298f1ac94b256669c5ee1a3dcfe9a1ea8 Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Thu, 25 Jul 2024 15:10:20 +0200 Subject: [PATCH 14/20] restrict default_app_lifecycle on schema level Co-authored-by: Ralf Pannemans --- .../config_schemas/base/api_schema.rb | 2 +- .../config_schemas/base/worker_schema.rb | 3 +- spec/request/buildpacks_spec.rb | 48 +++++++++++++++++++ .../lifecycles/app_lifecycle_provider_spec.rb | 10 ++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/lib/cloud_controller/config_schemas/base/api_schema.rb b/lib/cloud_controller/config_schemas/base/api_schema.rb index 93f5cfc541d..97875bc829b 100644 --- a/lib/cloud_controller/config_schemas/base/api_schema.rb +++ b/lib/cloud_controller/config_schemas/base/api_schema.rb @@ -378,7 +378,7 @@ class ApiSchema < VCAP::Config internal_route_vip_range: String, - default_app_lifecycle: String, + default_app_lifecycle: enum('buildpack', 'cnb'), custom_metric_tag_prefix_list: Array, optional(:cc_service_key_client_name) => String, diff --git a/lib/cloud_controller/config_schemas/base/worker_schema.rb b/lib/cloud_controller/config_schemas/base/worker_schema.rb index 84726503417..46de1630e03 100644 --- a/lib/cloud_controller/config_schemas/base/worker_schema.rb +++ b/lib/cloud_controller/config_schemas/base/worker_schema.rb @@ -191,7 +191,8 @@ class WorkerSchema < VCAP::Config max_labels_per_resource: Integer, max_annotations_per_resource: Integer, internal_route_vip_range: String, - custom_metric_tag_prefix_list: Array + custom_metric_tag_prefix_list: Array, + default_app_lifecycle: enum('buildpack', 'cnb') } end # rubocop:enable Metrics/BlockLength diff --git a/spec/request/buildpacks_spec.rb b/spec/request/buildpacks_spec.rb index 91bb82960ad..991d70e6e46 100644 --- a/spec/request/buildpacks_spec.rb +++ b/spec/request/buildpacks_spec.rb @@ -28,6 +28,7 @@ order_by: 'updated_at', names: 'foo', stacks: 'cf', + lifecycle: 'buildpack', label_selector: 'foo,bar', guids: 'foo,bar', created_ats: "#{Time.now.utc.iso8601},#{Time.now.utc.iso8601}", @@ -95,6 +96,7 @@ let!(:buildpack1) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 1) } let!(:buildpack2) { VCAP::CloudController::Buildpack.make(stack: stack2.name, position: 3) } let!(:buildpack3) { VCAP::CloudController::Buildpack.make(stack: stack3.name, position: 2) } + let!(:buildpack4) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 1, lifecycle: 'cnb') } it 'returns a paginated list of buildpacks, sorted by position' do get '/v3/buildpacks?page=1&per_page=2', nil, headers @@ -213,6 +215,52 @@ ) end + it 'returns a list of buildpacks filtered by lifecycle' do + get '/v3/buildpacks?lifecycle=cnb', nil, headers + + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 1, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=cnb&page=1&per_page=50" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=cnb&page=1&per_page=50" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => buildpack4.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack4.name, + 'state' => buildpack4.state, + 'filename' => buildpack4.filename, + 'stack' => buildpack4.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}/upload", + 'method' => 'POST' + } + } + } + ] + } + ) + end + it 'orders by position' do get "/v3/buildpacks?names=#{buildpack1.name},#{buildpack3.name}&order_by=-position", nil, headers diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/app_lifecycle_provider_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/app_lifecycle_provider_spec.rb index e795f9033a8..31b809e0c1b 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/app_lifecycle_provider_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/app_lifecycle_provider_spec.rb @@ -37,6 +37,16 @@ module VCAP::CloudController expect(AppLifecycleProvider.provide_for_create(message)).to be_a(AppBuildpackLifecycle) end end + + context 'default_app_lifecycle is set to cnb' do + before do + TestConfig.override(default_app_lifecycle: 'cnb') + end + + it 'returns a AppCNBLifecycle' do + expect(AppLifecycleProvider.provide_for_create(message)).to be_a(AppCNBLifecycle) + end + end end end From cc3ac62b72d776a782d78297a967c20aa4f1bdc2 Mon Sep 17 00:00:00 2001 From: Johannes Dillmann Date: Thu, 31 Oct 2024 10:01:54 +0100 Subject: [PATCH 15/20] Update migration Co-authored-by: Nicolas Bender --- ...stem_buildpacks.rb => 20241031095500_cnb_system_buildpacks.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename db/migrations/{20240710152700_cnb_system_buildpacks.rb => 20241031095500_cnb_system_buildpacks.rb} (100%) diff --git a/db/migrations/20240710152700_cnb_system_buildpacks.rb b/db/migrations/20241031095500_cnb_system_buildpacks.rb similarity index 100% rename from db/migrations/20240710152700_cnb_system_buildpacks.rb rename to db/migrations/20241031095500_cnb_system_buildpacks.rb From 739dda745b903d9906c640aadd4ed3adc6d0ee10 Mon Sep 17 00:00:00 2001 From: Nicolas Bender Date: Thu, 24 Apr 2025 11:04:30 +0200 Subject: [PATCH 16/20] fix cnb update message validation test Co-authored-by: Pavel Busko --- .../diego/lifecycles/app_cnb_lifecycle_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb b/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb index 6367395270c..27e28307c53 100644 --- a/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/lifecycles/app_cnb_lifecycle_spec.rb @@ -102,14 +102,6 @@ module VCAP::CloudController it 'invalid' do expect(lifecycle.valid?).to be(false) end - - context 'during an update' do - let(:message) { VCAP::CloudController::AppUpdateMessage.new(request) } - - it 'valid' do - expect(lifecycle.valid?).to be(true) - end - end end context 'with buildpacks' do @@ -123,6 +115,14 @@ module VCAP::CloudController it 'valid' do expect(lifecycle.valid?).to be(true) end + + context 'during an update' do + let(:message) { VCAP::CloudController::AppUpdateMessage.new(request) } + + it 'valid' do + expect(lifecycle.valid?).to be(true) + end + end end end end From d3b75b62ec1e57cdbc81998e5a8218af8accdc3e Mon Sep 17 00:00:00 2001 From: Pavel Busko Date: Thu, 24 Apr 2025 11:30:25 +0200 Subject: [PATCH 17/20] rename migration Co-authored-by: Nicolas Bender Co-authored-by: Pavel Busko --- ...stem_buildpacks.rb => 20250424095500_cnb_system_buildpacks.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename db/migrations/{20241031095500_cnb_system_buildpacks.rb => 20250424095500_cnb_system_buildpacks.rb} (100%) diff --git a/db/migrations/20241031095500_cnb_system_buildpacks.rb b/db/migrations/20250424095500_cnb_system_buildpacks.rb similarity index 100% rename from db/migrations/20241031095500_cnb_system_buildpacks.rb rename to db/migrations/20250424095500_cnb_system_buildpacks.rb From e2d2b3f5351c6e2838deda107ce57f300c15c8f6 Mon Sep 17 00:00:00 2001 From: Tom Kennedy Date: Thu, 24 Apr 2025 10:32:00 -0400 Subject: [PATCH 18/20] Add support for uploading buildpacks packaged as cnb (tar) files - This is a follow on to https://github.com/cloudfoundry/cloud_controller_ng/pull/3898 which only supports uploading CNBs as zip or tgz whereas they are released in cnb format Signed-off-by: Tom Kennedy --- app/messages/buildpack_upload_message.rb | 6 ++++- spec/support/test_cnb.rb | 16 ++++++++++++++ .../cloud_controller/upload_buildpack_spec.rb | 22 ++++++++++++++++++- .../messages/buildpack_upload_message_spec.rb | 15 +++++++++++-- 4 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 spec/support/test_cnb.rb diff --git a/app/messages/buildpack_upload_message.rb b/app/messages/buildpack_upload_message.rb index f0ebe0b1da0..6e10fe5dc80 100644 --- a/app/messages/buildpack_upload_message.rb +++ b/app/messages/buildpack_upload_message.rb @@ -3,6 +3,7 @@ module VCAP::CloudController GZIP_MIME = Regexp.new("\x1F\x8B\x08".force_encoding('binary')) ZIP_MIME = Regexp.new("PK\x03\x04".force_encoding('binary')) + CNB_MIME = Regexp.new("\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary')) class BuildpackUploadMessage < BaseMessage class MissingFilePathError < StandardError; end @@ -53,7 +54,10 @@ def is_archive return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/ || mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/ - errors.add(:base, "#{bits_name} is not a zip or gzip archive") + mime_bits_at_offset = File.read(bits_path, 8, 257) + return if mime_bits_at_offset =~ /^#{VCAP::CloudController::CNB_MIME}/ + + errors.add(:base, "#{bits_name} is not a zip or gzip archive or cnb file") end def missing_file_path diff --git a/spec/support/test_cnb.rb b/spec/support/test_cnb.rb new file mode 100644 index 00000000000..ba6935e7101 --- /dev/null +++ b/spec/support/test_cnb.rb @@ -0,0 +1,16 @@ +require 'zlib' +require 'rubygems/package' + +module TestCnb + def self.create(name, file_count, file_size = 1024, &) + File.open(name, 'wb') do |file| + Gem::Package::TarWriter.new(file) do |tar| + file_count.times do |i| + tar.add_file_simple("test_#{i}", 0o644, file_size) do |f| + f.write('A' * file_size) + end + end + end + end + end +end diff --git a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb index 67d93a1ea4c..507f56f9abd 100644 --- a/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb +++ b/spec/unit/lib/cloud_controller/upload_buildpack_spec.rb @@ -10,10 +10,12 @@ module VCAP::CloudController let(:tmpdir) { Dir.mktmpdir } let(:filename) { 'file.zip' } let(:tgz_filename) { 'file.tgz' } + let(:cnb_filename) { 'file.cnb' } let(:sha_valid_zip) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_zip) } let(:sha_valid_zip2) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_zip2) } let(:sha_valid_tgz) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_tgz) } + let(:sha_valid_cnb) { Digester.new(algorithm: OpenSSL::Digest::SHA256).digest_file(valid_cnb) } let(:valid_zip_manifest_stack) { nil } let(:valid_zip) do @@ -43,6 +45,13 @@ module VCAP::CloudController Rack::Test::UploadedFile.new(tgz_file) end + let(:valid_cnb) do + cnb_name = File.join(tmpdir, cnb_filename) + TestCnb.create(cnb_name, 3, 1024) + cnb_file = File.new(cnb_name) + Rack::Test::UploadedFile.new(cnb_file) + end + let(:staging_timeout) { TestConfig.config_instance.get(:staging, :timeout_in_seconds) } let(:expected_sha_valid_zip) { "#{buildpack.guid}_#{sha_valid_zip}" } @@ -152,7 +161,7 @@ module VCAP::CloudController end end - context 'lifecycle: cnb' do + context 'lifecycle: gzip cnb' do let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_cnb_buildpack', stack: 'cider', position: 0, lifecycle: 'cnb' }) } let(:expected_sha_valid_tgz) { "#{buildpack.guid}_#{sha_valid_tgz}" } @@ -163,6 +172,17 @@ module VCAP::CloudController end end + context 'lifecycle: cnb' do + let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_cnb_buildpack', stack: 'cider', position: 0, lifecycle: 'cnb' }) } + let(:expected_sha_valid_cnb) { "#{buildpack.guid}_#{sha_valid_cnb}" } + + it 'uploads' do + expect(buildpack_blobstore).to receive(:cp_to_blobstore).with(valid_cnb, expected_sha_valid_cnb) + + upload_buildpack.upload_buildpack(buildpack, valid_cnb, cnb_filename) + end + end + it 'updates the buildpack filename' do expect do upload_buildpack.upload_buildpack(buildpack, valid_zip, filename) diff --git a/spec/unit/messages/buildpack_upload_message_spec.rb b/spec/unit/messages/buildpack_upload_message_spec.rb index 7c295b783ac..9c84814bc67 100644 --- a/spec/unit/messages/buildpack_upload_message_spec.rb +++ b/spec/unit/messages/buildpack_upload_message_spec.rb @@ -102,20 +102,31 @@ module VCAP::CloudController allow(File).to receive(:read).and_return("PX\x03\x04".force_encoding('binary')) upload_message = BuildpackUploadMessage.new(opts) expect(upload_message).not_to be_valid - expect(upload_message.errors.full_messages[0]).to include('buildpack.abcd is not a zip or gzip') + expect(upload_message.errors.full_messages[0]).to include('buildpack.abcd is not a zip or gzip archive or cnb file') end end context 'when the file is a tgz' do let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } - it 'is not valid' do + it 'is valid' do allow(File).to receive(:read).and_return("\x1F\x8B\x08".force_encoding('binary')) upload_message = BuildpackUploadMessage.new(opts) expect(upload_message).to be_valid end end + context 'when the file is a cnb/tar' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.cnb' } } + + it 'is valid' do + values = ["\x0".force_encoding('binary'), "\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary')] + allow(File).to receive(:read).and_return(*values) + upload_message = BuildpackUploadMessage.new(opts) + expect(upload_message).to be_valid + end + end + context 'when the file is empty' do let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.zip' } } let(:stat_double) { instance_double(File::Stat, size: 0) } From 07231d183e67ba7a4404942101c8e2660d224c5f Mon Sep 17 00:00:00 2001 From: Seth Boyles Date: Thu, 24 Apr 2025 13:52:18 -0700 Subject: [PATCH 19/20] Validate buildpack filetype based on lifecycle --- app/controllers/v3/buildpacks_controller.rb | 2 +- app/messages/buildpack_upload_message.rb | 25 +++- spec/support/test_cnb.rb | 2 +- spec/unit/actions/buildpack_upload_spec.rb | 2 +- .../messages/buildpack_upload_message_spec.rb | 113 +++++++++++++----- 5 files changed, 107 insertions(+), 37 deletions(-) diff --git a/app/controllers/v3/buildpacks_controller.rb b/app/controllers/v3/buildpacks_controller.rb index 28386823917..2916970fa59 100644 --- a/app/controllers/v3/buildpacks_controller.rb +++ b/app/controllers/v3/buildpacks_controller.rb @@ -77,7 +77,7 @@ def upload unauthorized! unless permission_queryer.can_write_globally? - message = BuildpackUploadMessage.create_from_params(hashed_params[:body]) + message = BuildpackUploadMessage.create_from_params(hashed_params[:body], buildpack.lifecycle) combine_messages(message.errors.full_messages) unless message.valid? unprocessable!('Buildpack is locked') if buildpack.locked diff --git a/app/messages/buildpack_upload_message.rb b/app/messages/buildpack_upload_message.rb index 6e10fe5dc80..0742ea88102 100644 --- a/app/messages/buildpack_upload_message.rb +++ b/app/messages/buildpack_upload_message.rb @@ -17,8 +17,15 @@ class MissingFilePathError < StandardError; end validate :is_not_empty validate :missing_file_path - def self.create_from_params(params) - BuildpackUploadMessage.new(params.dup.symbolize_keys) + attr_reader :lifecycle + + def initialize(params, lifecycle) + @lifecycle = lifecycle + super(params) + end + + def self.create_from_params(params, lifecycle) + BuildpackUploadMessage.new(params.dup.symbolize_keys, lifecycle) end def nginx_fields @@ -52,12 +59,18 @@ def is_archive mime_bits = File.read(bits_path, 4) - return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/ || mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/ + if lifecycle == VCAP::CloudController::Lifecycles::BUILDPACK + return if mime_bits =~ /^#{VCAP::CloudController::ZIP_MIME}/ + + errors.add(:base, "#{bits_name} is not a zip file. Buildpacks of lifecycle \"#{lifecycle}\" must be valid zip files.") + elsif lifecycle == VCAP::CloudController::Lifecycles::CNB + return if mime_bits =~ /^#{VCAP::CloudController::GZIP_MIME}/ - mime_bits_at_offset = File.read(bits_path, 8, 257) - return if mime_bits_at_offset =~ /^#{VCAP::CloudController::CNB_MIME}/ + mime_bits_at_offset = File.read(bits_path, 8, 257) + return if mime_bits_at_offset =~ /^#{VCAP::CloudController::CNB_MIME}/ - errors.add(:base, "#{bits_name} is not a zip or gzip archive or cnb file") + errors.add(:base, "#{bits_name} is not a gzip archive or cnb file. Buildpacks of lifecycle \"#{lifecycle}\" must be valid gzip archives or cnb files.") + end end def missing_file_path diff --git a/spec/support/test_cnb.rb b/spec/support/test_cnb.rb index ba6935e7101..b17a5c35321 100644 --- a/spec/support/test_cnb.rb +++ b/spec/support/test_cnb.rb @@ -2,7 +2,7 @@ require 'rubygems/package' module TestCnb - def self.create(name, file_count, file_size = 1024, &) + def self.create(name, file_count, file_size=1024, &) File.open(name, 'wb') do |file| Gem::Package::TarWriter.new(file) do |tar| file_count.times do |i| diff --git a/spec/unit/actions/buildpack_upload_spec.rb b/spec/unit/actions/buildpack_upload_spec.rb index cf3c5f4aa8a..642e56e1428 100644 --- a/spec/unit/actions/buildpack_upload_spec.rb +++ b/spec/unit/actions/buildpack_upload_spec.rb @@ -7,7 +7,7 @@ module VCAP::CloudController describe '#upload_async' do let!(:buildpack) { VCAP::CloudController::Buildpack.create_from_hash({ name: 'upload_binary_buildpack', stack: nil, position: 0 }) } - let(:message) { BuildpackUploadMessage.new({ 'bits_path' => '/tmp/path', 'bits_name' => 'buildpack.zip' }) } + let(:message) { BuildpackUploadMessage.new({ 'bits_path' => '/tmp/path', 'bits_name' => 'buildpack.zip' }, VCAP::CloudController::Lifecycles::BUILDPACK) } let(:config) { Config.new({ name: 'local', index: '1' }) } before do diff --git a/spec/unit/messages/buildpack_upload_message_spec.rb b/spec/unit/messages/buildpack_upload_message_spec.rb index 9c84814bc67..97e689e4b94 100644 --- a/spec/unit/messages/buildpack_upload_message_spec.rb +++ b/spec/unit/messages/buildpack_upload_message_spec.rb @@ -5,6 +5,8 @@ module VCAP::CloudController RSpec.describe BuildpackUploadMessage do before { TestConfig.override(directories: { tmpdir: '/tmp/' }) } + let(:lifecycle) { VCAP::CloudController::Lifecycles::BUILDPACK } + describe 'validations' do let(:stat_double) { instance_double(File::Stat, size: 2) } @@ -19,7 +21,7 @@ module VCAP::CloudController let(:opts) { { '' => '', bits_path: '/tmp/foobar', bits_name: 'buildpack.zip' } } it 'is invalid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid expect(upload_message.errors[:base]).to include('Uploaded bits are not a valid buildpack file') end @@ -29,7 +31,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '/tmp/foobar', bits_name: 'buildpack.zip' } } it 'is valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).to be_valid end end @@ -38,7 +40,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '../tmp/mango/pear', bits_name: 'buildpack.zip' } } it 'is valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).to be_valid end end @@ -47,7 +49,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '../tmp-not!/mango/pear' } } it 'is not valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid expect(upload_message.errors[:bits_path]).to include('is invalid') end @@ -57,7 +59,7 @@ module VCAP::CloudController let(:opts) { {} } it 'is not valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid expect(upload_message.errors[:base]).to include('A buildpack zip file must be uploaded as \'bits\'') end @@ -67,7 +69,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '/tmp/bar', unexpected: 'foo' } } it 'is not valid' do - message = BuildpackUploadMessage.new(opts) + message = BuildpackUploadMessage.new(opts, lifecycle) expect(message).not_to be_valid expect(message.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") @@ -78,7 +80,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '/secret/file', bits_name: 'buildpack.zip' } } it 'is not valid' do - message = BuildpackUploadMessage.new(opts) + message = BuildpackUploadMessage.new(opts, lifecycle) expect(message).not_to be_valid expect(message.errors.full_messages[0]).to include('Bits path is invalid') @@ -89,7 +91,7 @@ module VCAP::CloudController let(:opts) { { bits_path: '/tmp/bar' } } it 'is not valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid expect(upload_message.errors[:base]).to include('A buildpack zip file must be uploaded as \'bits\'') end @@ -100,30 +102,85 @@ module VCAP::CloudController it 'is not valid' do allow(File).to receive(:read).and_return("PX\x03\x04".force_encoding('binary')) - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid - expect(upload_message.errors.full_messages[0]).to include('buildpack.abcd is not a zip or gzip archive or cnb file') + expect(upload_message.errors.full_messages[0]).to include('buildpack.abcd is not a zip file. Buildpacks of lifecycle "buildpack" must be valid zip files.') end end - context 'when the file is a tgz' do - let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } + context 'buildpacks' do + let(:lifecycle) { VCAP::CloudController::Lifecycles::BUILDPACK } - it 'is valid' do - allow(File).to receive(:read).and_return("\x1F\x8B\x08".force_encoding('binary')) - upload_message = BuildpackUploadMessage.new(opts) - expect(upload_message).to be_valid + context 'when the file is a zip' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.zip' } } + + it 'is valid' do + allow(File).to receive(:read).and_return("PK\x03\x04".force_encoding('binary')) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).to be_valid + end end - end - context 'when the file is a cnb/tar' do - let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.cnb' } } + context 'when the file is a tgz' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } - it 'is valid' do - values = ["\x0".force_encoding('binary'), "\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary')] - allow(File).to receive(:read).and_return(*values) - upload_message = BuildpackUploadMessage.new(opts) - expect(upload_message).to be_valid + it 'is not valid' do + allow(File).to receive(:read).and_return("\x1F\x8B\x08".force_encoding('binary')) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).not_to be_valid + expect(upload_message.errors.full_messages[0]).to include('buildpack.tgz is not a zip file. Buildpacks of lifecycle "buildpack" must be valid zip files.') + end + end + + context 'when the file is a cnb/tar' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.cnb' } } + + it 'is not valid' do + values = ["\x0".force_encoding('binary'), "\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary')] + allow(File).to receive(:read).and_return(*values) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).not_to be_valid + expect(upload_message.errors.full_messages[0]).to include('buildpack.cnb is not a zip file. Buildpacks of lifecycle "buildpack" must be valid zip files.') + end + end + end + + context 'cloud native buildpacks (cnb)' do + let(:lifecycle) { VCAP::CloudController::Lifecycles::CNB } + + context 'buildpacks' do + context 'when the file is a zip' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.zip' } } + + it 'is valid' do + allow(File).to receive(:read).and_return("PK\x03\x04".force_encoding('binary')) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).not_to be_valid + expect(upload_message.errors.full_messages[0]).to + include('buildpack.zip is not a gzip archive or cnb file. Buildpacks of lifecycle "cnb" must be valid gzip archives or cnb files.') + end + end + + context 'when the file is a tgz' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.tgz' } } + + it 'is valid' do + allow(File).to receive(:read).and_return("\x1F\x8B\x08".force_encoding('binary')) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).to be_valid + end + end + + context 'when the file is a cnb/tar' do + let(:opts) { { bits_path: '/tmp/bar', bits_name: 'buildpack.cnb' } } + + it 'is valid' do + values = ["\x0".force_encoding('binary'), "\x75\x73\x74\x61\x72\x00\x30\x30".force_encoding('binary')] + allow(File).to receive(:read).and_return(*values) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) + expect(upload_message).to be_valid + end + end end end @@ -132,7 +189,7 @@ module VCAP::CloudController let(:stat_double) { instance_double(File::Stat, size: 0) } it 'is not valid' do - upload_message = BuildpackUploadMessage.new(opts) + upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid expect(upload_message.errors.full_messages[0]).to include('buildpack.zip cannot be empty') end @@ -140,7 +197,7 @@ module VCAP::CloudController end describe '#bits_path=' do - subject(:upload_message) { BuildpackUploadMessage.new(bits_path: 'not-nil') } + subject(:upload_message) { BuildpackUploadMessage.new({ bits_path: 'not-nil' }, lifecycle) } context 'when the bits_path is relative' do it 'makes it absolute (within the tmpdir)' do @@ -162,7 +219,7 @@ module VCAP::CloudController let(:params) { { 'bits_path' => '/tmp/foobar', 'bits_name' => 'buildpack.zip' } } it 'returns the correct BuildpackUploadMessage' do - message = BuildpackUploadMessage.create_from_params(params) + message = BuildpackUploadMessage.create_from_params(params, lifecycle) expect(message).to be_a(BuildpackUploadMessage) expect(message.bits_path).to eq('/tmp/foobar') @@ -170,7 +227,7 @@ module VCAP::CloudController end it 'converts requested keys to symbols' do - message = BuildpackUploadMessage.create_from_params(params) + message = BuildpackUploadMessage.create_from_params(params, lifecycle) expect(message).to be_requested(:bits_path) expect(message).to be_requested(:bits_name) From cbefbae59d31363d025507d7c44f197185977808 Mon Sep 17 00:00:00 2001 From: Seth Boyles Date: Tue, 29 Apr 2025 14:16:55 -0700 Subject: [PATCH 20/20] Remove default buildpacks filter and change default sort to 'lifecycle' * also add missing docs --- app/fetchers/buildpack_list_fetcher.rb | 3 +- app/messages/buildpacks_list_message.rb | 5 +- .../includes/api_resources/_buildpacks.erb | 2 + .../resources/buildpacks/_create.md.erb | 1 + .../resources/buildpacks/_list.md.erb | 3 +- .../resources/buildpacks/_object.md.erb | 3 +- .../paging/pagination_options.rb | 8 +- .../paging/sequel_paginator.rb | 2 + spec/request/buildpacks_spec.rb | 389 +++++++++++++++++- .../v3/buildpacks_controller_spec.rb | 18 +- .../fetchers/buildpack_list_fetcher_spec.rb | 6 +- .../paging/pagination_options_spec.rb | 28 ++ .../paging/sequel_paginator_spec.rb | 44 ++ .../messages/buildpack_upload_message_spec.rb | 4 +- 14 files changed, 490 insertions(+), 26 deletions(-) diff --git a/app/fetchers/buildpack_list_fetcher.rb b/app/fetchers/buildpack_list_fetcher.rb index 1ae20e8052c..1c3713b269c 100644 --- a/app/fetchers/buildpack_list_fetcher.rb +++ b/app/fetchers/buildpack_list_fetcher.rb @@ -19,7 +19,8 @@ def filter(message, dataset) dataset = dataset.where(name: message.names) if message.requested?(:names) dataset = NullFilterQueryGenerator.add_filter(dataset, :stack, message.stacks) if message.requested?(:stacks) - dataset = dataset.where(lifecycle: message.requested?(:lifecycle) ? message.lifecycle : Config.config.get(:default_app_lifecycle)) + + dataset = dataset.where(lifecycle: message.lifecycle) if message.requested?(:lifecycle) if message.requested?(:label_selector) dataset = LabelSelectorQueryGenerator.add_selector_queries( diff --git a/app/messages/buildpacks_list_message.rb b/app/messages/buildpacks_list_message.rb index b0ba8cb5a64..293ec500248 100644 --- a/app/messages/buildpacks_list_message.rb +++ b/app/messages/buildpacks_list_message.rb @@ -21,7 +21,8 @@ class BuildpacksListMessage < MetadataListMessage def initialize(params={}) super - pagination_options.default_order_by = :position + pagination_options.default_order_by = :lifecycle + pagination_options.secondary_default_order_by = :position end def self.from_params(params) @@ -33,7 +34,7 @@ def to_param_hash end def valid_order_by_values - super << :position + super + %i[position lifecycle] end end diff --git a/docs/v3/source/includes/api_resources/_buildpacks.erb b/docs/v3/source/includes/api_resources/_buildpacks.erb index c2586026f10..2caea659bf6 100644 --- a/docs/v3/source/includes/api_resources/_buildpacks.erb +++ b/docs/v3/source/includes/api_resources/_buildpacks.erb @@ -8,6 +8,7 @@ "filename": null, "stack": "windows64", "position": 42, + "lifecycle": "buildpack", "enabled": true, "locked": false, "metadata": { @@ -52,6 +53,7 @@ "filename": null, "stack": "my-stack", "position": 1, + "lifecycle": "cnb", "enabled": true, "locked": false, "metadata": { diff --git a/docs/v3/source/includes/resources/buildpacks/_create.md.erb b/docs/v3/source/includes/resources/buildpacks/_create.md.erb index 00e818e08e3..dbe4eb8907e 100644 --- a/docs/v3/source/includes/resources/buildpacks/_create.md.erb +++ b/docs/v3/source/includes/resources/buildpacks/_create.md.erb @@ -44,6 +44,7 @@ Name | Type | Description ----------- | -------- | ----------------------------------------------------------------------------- | ------- **stack** | _string_ | The name of the stack that the buildpack will use | null **position** | _integer_ | The order in which the buildpacks are checked during buildpack auto-detection | 1 +**lifecycle**| _string_ | The version of buildpack the buildpack will use. `buildpack` indicates [Classic Buildpacks](https://docs.cloudfoundry.org/buildpacks/classic.html). `cnb` indicates [Cloud Native Buildpacks](https://docs.cloudfoundry.org/buildpacks/cnb/) | buildpack **enabled** | _boolean_ | Whether or not the buildpack will be used for staging | true **locked** | _boolean_ | Whether or not the buildpack is locked to prevent updating the bits | false **metadata.labels** | [_label object_](#labels) | Labels applied to the buildpack diff --git a/docs/v3/source/includes/resources/buildpacks/_list.md.erb b/docs/v3/source/includes/resources/buildpacks/_list.md.erb index bfad835b683..3fe885d0478 100644 --- a/docs/v3/source/includes/resources/buildpacks/_list.md.erb +++ b/docs/v3/source/includes/resources/buildpacks/_list.md.erb @@ -34,7 +34,8 @@ Name | Type | Description **per_page** | _integer_ | Number of results per page;
valid values are 1 through 5000 **names** | _list of strings_ | Comma-delimited list of buildpack names to filter by **stacks**| _list of strings_ | Comma-delimited list of stack names to filter by -**order_by** | _string_ | Value to sort by; defaults to ascending. Prepend with `-` to sort descending.
Valid values are `created_at`, `updated_at`, and `position` +**lifecycle**| _string_ | Type of buildpack. Valid values are `buildpack` and `cnb` +**order_by** | _string_ | Value to sort by; defaults to ascending. Prepend with `-` to sort descending.
Valid values are `created_at`, `updated_at`, `lifecycle`, and `position` **label_selector** | _string_ | A query string containing a list of [label selector](#labels-and-selectors) requirements **created_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators) **updated_ats** | _[timestamp](#timestamps)_ | Timestamp to filter by. When filtering on equality, several comma-delimited timestamps may be passed. Also supports filtering with [relational operators](#relational-operators) diff --git a/docs/v3/source/includes/resources/buildpacks/_object.md.erb b/docs/v3/source/includes/resources/buildpacks/_object.md.erb index 462f3b4a778..aa8918501f7 100644 --- a/docs/v3/source/includes/resources/buildpacks/_object.md.erb +++ b/docs/v3/source/includes/resources/buildpacks/_object.md.erb @@ -15,7 +15,8 @@ Name | Type | Description **updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated **name** | _string_ | The name of the buildpack; to be used by app buildpack field (only alphanumeric characters) **state** | _string_ | The state of the buildpack; valid states are: `AWAITING_UPLOAD`, `READY` -**stack** | _string_ | The name of the stack that the buildpack will use +**stack** | _string_ | The name of the stack that the buildpack uses +**lifecycle** | _string_ | The version of buildpacks the buildpack uses. `buildpack` indicates [Classic Buildpacks](https://docs.cloudfoundry.org/buildpacks/classic.html). `cnb` indicates [Cloud Native Buildpacks](https://docs.cloudfoundry.org/buildpacks/cnb/) **filename** | _string_ | The filename of the buildpack **position** | _integer_ | The order in which the buildpacks are checked during buildpack auto-detection **enabled** | _boolean_ | Whether or not the buildpack can be used for staging diff --git a/lib/cloud_controller/paging/pagination_options.rb b/lib/cloud_controller/paging/pagination_options.rb index 0555db155dc..d07c7124e43 100644 --- a/lib/cloud_controller/paging/pagination_options.rb +++ b/lib/cloud_controller/paging/pagination_options.rb @@ -12,7 +12,7 @@ class PaginationOptions DIRECTION_DEFAULT = 'asc'.freeze VALID_DIRECTIONS = %w[asc desc].freeze - attr_writer :order_by, :order_direction, :default_order_by + attr_writer :order_by, :order_direction, :default_order_by, :secondary_default_order_by attr_accessor :page, :per_page def initialize(params) @@ -38,6 +38,12 @@ def order_direction @order_direction || DIRECTION_DEFAULT end + def secondary_order_by + return if @order_by && @order_by != default_order_by + + @secondary_default_order_by + end + def keys %i[page per_page order_by order_direction] end diff --git a/lib/cloud_controller/paging/sequel_paginator.rb b/lib/cloud_controller/paging/sequel_paginator.rb index 46d8acf933c..c104b0de08b 100644 --- a/lib/cloud_controller/paging/sequel_paginator.rb +++ b/lib/cloud_controller/paging/sequel_paginator.rb @@ -13,6 +13,8 @@ def get_page(dataset, pagination_options) order_type = Sequel.send(order_direction, Sequel.qualify(table_name, order_by)) dataset = dataset.order(order_type) + secondary_order_by = pagination_options.secondary_order_by + dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, secondary_order_by))) if secondary_order_by dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :guid))) if order_by != 'id' && has_guid_column distinct_opt = dataset.opts[:distinct] diff --git a/spec/request/buildpacks_spec.rb b/spec/request/buildpacks_spec.rb index 991d70e6e46..a11a3850b71 100644 --- a/spec/request/buildpacks_spec.rb +++ b/spec/request/buildpacks_spec.rb @@ -93,24 +93,26 @@ let!(:stack2) { VCAP::CloudController::Stack.make } let!(:stack3) { VCAP::CloudController::Stack.make } + let!(:buildpack4) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 2, lifecycle: 'cnb') } + let!(:buildpack5) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 1, lifecycle: 'cnb') } + let!(:buildpack1) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 1) } let!(:buildpack2) { VCAP::CloudController::Buildpack.make(stack: stack2.name, position: 3) } let!(:buildpack3) { VCAP::CloudController::Buildpack.make(stack: stack3.name, position: 2) } - let!(:buildpack4) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 1, lifecycle: 'cnb') } - it 'returns a paginated list of buildpacks, sorted by position' do + it 'returns a paginated list of buildpacks, sorted by lifecycle and position' do get '/v3/buildpacks?page=1&per_page=2', nil, headers expect(parsed_response).to be_a_response_like( { 'pagination' => { - 'total_results' => 3, - 'total_pages' => 2, + 'total_results' => 5, + 'total_pages' => 3, 'first' => { 'href' => "#{link_prefix}/v3/buildpacks?page=1&per_page=2" }, 'last' => { - 'href' => "#{link_prefix}/v3/buildpacks?page=2&per_page=2" + 'href' => "#{link_prefix}/v3/buildpacks?page=3&per_page=2" }, 'next' => { 'href' => "#{link_prefix}/v3/buildpacks?page=2&per_page=2" @@ -169,6 +171,144 @@ ) end + it 'with no filters, returns a list of buildpacks, sorted by lifecycle and position' do + get '/v3/buildpacks', nil, headers + + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 5, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/buildpacks?page=1&per_page=50" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/buildpacks?page=1&per_page=50" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack1.name, + 'state' => buildpack1.state, + 'filename' => buildpack1.filename, + 'stack' => buildpack1.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack3.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack3.name, + 'state' => buildpack3.state, + 'filename' => buildpack3.filename, + 'stack' => buildpack3.stack, + 'position' => 2, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack2.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack2.name, + 'state' => buildpack2.state, + 'filename' => buildpack2.filename, + 'stack' => buildpack2.stack, + 'position' => 3, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack2.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack2.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack5.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack5.name, + 'state' => buildpack5.state, + 'filename' => buildpack5.filename, + 'stack' => buildpack5.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack4.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack4.name, + 'state' => buildpack4.state, + 'filename' => buildpack4.filename, + 'stack' => buildpack4.stack, + 'position' => 2, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}/upload", + 'method' => 'POST' + } + } + } + ] + } + ) + end + it 'returns a list of filtered buildpacks' do get "/v3/buildpacks?names=#{buildpack1.name},#{buildpack3.name}&stacks=#{stack1.name}", nil, headers @@ -215,13 +355,84 @@ ) end + it 'returns a paginated list of buildpacks filtered by lifecycle' do + get '/v3/buildpacks?lifecycle=buildpack&per_page=2', nil, headers + + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 3, + 'total_pages' => 2, + 'first' => { + 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=buildpack&page=1&per_page=2" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=buildpack&page=2&per_page=2" + }, + 'next' => { + 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=buildpack&page=2&per_page=2" + }, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack1.name, + 'state' => buildpack1.state, + 'filename' => buildpack1.filename, + 'stack' => buildpack1.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack3.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack3.name, + 'state' => buildpack3.state, + 'filename' => buildpack3.filename, + 'stack' => buildpack3.stack, + 'position' => 2, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}/upload", + 'method' => 'POST' + } + } + } + ] + } + ) + end + it 'returns a list of buildpacks filtered by lifecycle' do get '/v3/buildpacks?lifecycle=cnb', nil, headers expect(parsed_response).to be_a_response_like( { 'pagination' => { - 'total_results' => 1, + 'total_results' => 2, 'total_pages' => 1, 'first' => { 'href' => "#{link_prefix}/v3/buildpacks?lifecycle=cnb&page=1&per_page=50" @@ -233,6 +444,29 @@ 'previous' => nil }, 'resources' => [ + { + 'guid' => buildpack5.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack5.name, + 'state' => buildpack5.state, + 'filename' => buildpack5.filename, + 'stack' => buildpack5.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}/upload", + 'method' => 'POST' + } + } + }, { 'guid' => buildpack4.guid, 'lifecycle' => 'cnb', @@ -242,7 +476,7 @@ 'state' => buildpack4.state, 'filename' => buildpack4.filename, 'stack' => buildpack4.stack, - 'position' => 1, + 'position' => 2, 'enabled' => true, 'locked' => false, 'metadata' => { 'labels' => {}, 'annotations' => {} }, @@ -329,6 +563,147 @@ } ) end + + it 'reverse orders by lifecycle (with position as secondary)' do + get '/v3/buildpacks?order_by=-lifecycle', nil, headers + + expect(parsed_response).to be_a_response_like( + expect(parsed_response).to(be_a_response_like( + { + 'pagination' => { + 'total_results' => 5, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/buildpacks?order_by=-lifecycle&page=1&per_page=50" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/buildpacks?order_by=-lifecycle&page=1&per_page=50" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'guid' => buildpack4.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack4.name, + 'state' => buildpack4.state, + 'filename' => buildpack4.filename, + 'stack' => buildpack4.stack, + 'position' => 2, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack4.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack5.guid, + 'lifecycle' => 'cnb', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack5.name, + 'state' => buildpack5.state, + 'filename' => buildpack5.filename, + 'stack' => buildpack5.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack5.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack2.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack2.name, + 'state' => buildpack2.state, + 'filename' => buildpack2.filename, + 'stack' => buildpack2.stack, + 'position' => 3, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack2.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack2.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack3.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack3.name, + 'state' => buildpack3.state, + 'filename' => buildpack3.filename, + 'stack' => buildpack3.stack, + 'position' => 2, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack3.guid}/upload", + 'method' => 'POST' + } + } + }, + { + 'guid' => buildpack1.guid, + 'lifecycle' => 'buildpack', + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'name' => buildpack1.name, + 'state' => buildpack1.state, + 'filename' => buildpack1.filename, + 'stack' => buildpack1.stack, + 'position' => 1, + 'enabled' => true, + 'locked' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}" + }, + 'upload' => { + 'href' => "#{link_prefix}/v3/buildpacks/#{buildpack1.guid}/upload", + 'method' => 'POST' + } + } + } + + ] + } + )) + ) + end end context 'permissions' do diff --git a/spec/unit/controllers/v3/buildpacks_controller_spec.rb b/spec/unit/controllers/v3/buildpacks_controller_spec.rb index f99d423e528..89fcc22444f 100644 --- a/spec/unit/controllers/v3/buildpacks_controller_spec.rb +++ b/spec/unit/controllers/v3/buildpacks_controller_spec.rb @@ -52,9 +52,9 @@ let!(:stack1) { VCAP::CloudController::Stack.make } let!(:stack2) { VCAP::CloudController::Stack.make } - let!(:buildpack1) { VCAP::CloudController::Buildpack.make(stack: stack1.name) } - let!(:buildpack2) { VCAP::CloudController::Buildpack.make(stack: stack2.name) } - let!(:buildpack3) { VCAP::CloudController::Buildpack.make(stack: stack1.name, lifecycle: 'cnb') } + let!(:buildpack1) { VCAP::CloudController::Buildpack.make(stack: stack1.name, position: 2) } + let!(:buildpack2) { VCAP::CloudController::Buildpack.make(stack: stack2.name, position: 1) } + let!(:buildpack3) { VCAP::CloudController::Buildpack.make(stack: stack1.name, lifecycle: 'cnb', position: 1) } before do set_current_user(user) @@ -63,8 +63,9 @@ it 'renders a paginated list of buildpacks' do get :index - expect(parsed_body['resources'].first['guid']).to eq(buildpack1.guid) - expect(parsed_body['resources'].second['guid']).to eq(buildpack2.guid) + expect(parsed_body['resources'].first['guid']).to eq(buildpack2.guid) + expect(parsed_body['resources'].second['guid']).to eq(buildpack1.guid) + expect(parsed_body['resources'].third['guid']).to eq(buildpack3.guid) end it 'renders a lifecycle filtered list of buildpacks' do @@ -90,9 +91,10 @@ it 'renders an ordered list of buildpacks' do get :index, params: { order_by: '-position' } - expect(parsed_body['resources']).to have(2).buildpack - expect(parsed_body['resources'].first['position']).to eq(buildpack2.position) - expect(parsed_body['resources'].second['position']).to eq(buildpack1.position) + expect(parsed_body['resources']).to have(3).buildpack + expect(parsed_body['resources'].first['position']).to eq(2) + expect(parsed_body['resources'].second['position']).to eq(1) + expect(parsed_body['resources'].third['position']).to eq(1) end it 'eager loads associated resources that the presenter specifies' do diff --git a/spec/unit/fetchers/buildpack_list_fetcher_spec.rb b/spec/unit/fetchers/buildpack_list_fetcher_spec.rb index 9f95ea95cf1..c3891f6bc6f 100644 --- a/spec/unit/fetchers/buildpack_list_fetcher_spec.rb +++ b/spec/unit/fetchers/buildpack_list_fetcher_spec.rb @@ -38,7 +38,7 @@ module VCAP::CloudController let(:filters) { {} } it 'fetches all the buildpacks' do - expect(subject).to contain_exactly(buildpack1, buildpack2, buildpack3, buildpack4, buildpack_without_stack) + expect(subject).to contain_exactly(buildpack1, buildpack2, buildpack3, buildpack4, buildpack_without_stack, buildpack5, buildpack6, buildpack7) end end @@ -63,7 +63,7 @@ module VCAP::CloudController end it 'returns all of the desired buildpacks' do - expect(subject).to contain_exactly(buildpack_without_stack) + expect(subject).to contain_exactly(buildpack_without_stack, buildpack7) end end @@ -73,7 +73,7 @@ module VCAP::CloudController end it 'returns all of the desired buildpacks' do - expect(subject).to contain_exactly(buildpack2, buildpack_without_stack) + expect(subject).to contain_exactly(buildpack2, buildpack_without_stack, buildpack6, buildpack7) end end diff --git a/spec/unit/lib/cloud_controller/paging/pagination_options_spec.rb b/spec/unit/lib/cloud_controller/paging/pagination_options_spec.rb index 7197c0b7e5c..a122662c729 100644 --- a/spec/unit/lib/cloud_controller/paging/pagination_options_spec.rb +++ b/spec/unit/lib/cloud_controller/paging/pagination_options_spec.rb @@ -104,6 +104,34 @@ module VCAP::CloudController end end + context 'when secondary_default_order_by is configured' do + before do + pagination_options.secondary_default_order_by = 'id' + pagination_options.default_order_by = 'something' + end + + context 'when order_by is not configured by the user' do + it 'secondary_order_by returns the secondary_default_order_by' do + pagination_options.order_by = nil + expect(pagination_options.secondary_order_by).to eq('id') + end + end + + context 'when order_by is configured by the user' do + it 'secondary_order_by returns nil' do + pagination_options.order_by = 'first_name' + expect(pagination_options.secondary_order_by).to be_nil + end + end + + context 'when order_by is configured by the user to be the same as the default' do + it 'secondary_order_by returns the secondary_default_order_b' do + pagination_options.order_by = 'something' + expect(pagination_options.secondary_order_by).to eq('id') + end + end + end + context 'order_direction' do context 'when the order_direction is nil' do let(:order_direction) { nil } diff --git a/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb b/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb index 7f74df6d7d6..c1ef2d196cd 100644 --- a/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb +++ b/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb @@ -143,6 +143,50 @@ class TableWithoutGuid < Sequel::Model(:table_without_guid); end expect(paginated_result.records.first.keys).to match_array(AppModel.columns) end + it 'orders by secondary_default_order_by if using default order_by' do + Space.make(guid: '1') + Space.make(guid: '2') + Space.make(guid: '3') + Space.make(guid: '4') + options = { page: page, per_page: 4, order_direction: 'asc' } + app_model1.update(guid: '1', space_guid: '2', name: 'yourapp') + app_model2.update(guid: '2', space_guid: '1', name: 'yourapp') + app_model3.update(guid: '3', space_guid: '3', name: 'myapp') + app_model4.update(guid: '4', space_guid: '4', name: 'myapp') + pagination_options = PaginationOptions.new(options) + pagination_options.default_order_by = 'name' + pagination_options.secondary_default_order_by = 'space_guid' + + paginated_result = paginator.get_page(dataset, pagination_options) + + expect(paginated_result.records[0].guid).to eq(app_model3.guid) + expect(paginated_result.records[1].guid).to eq(app_model4.guid) + expect(paginated_result.records[2].guid).to eq(app_model2.guid) + expect(paginated_result.records[3].guid).to eq(app_model1.guid) + end + + it 'does not order by secondary_default_order_by if order_by is set' do + Space.make(guid: '1') + Space.make(guid: '2') + Space.make(guid: '3') + Space.make(guid: '4') + options = { page: page, order_by: 'name', per_page: 4, order_direction: 'asc' } + app_model1.update(guid: '1', space_guid: '2', name: 'yourapp') + app_model2.update(guid: '2', space_guid: '1', name: 'yourapp') + app_model3.update(guid: '3', space_guid: '3', name: 'myapp') + app_model4.update(guid: '4', space_guid: '4', name: 'myapp') + pagination_options = PaginationOptions.new(options) + pagination_options.default_order_by = 'guid' + pagination_options.secondary_default_order_by = 'space_guid' + + paginated_result = paginator.get_page(dataset, pagination_options) + + expect(paginated_result.records[0].guid).to eq(app_model3.guid) + expect(paginated_result.records[1].guid).to eq(app_model4.guid) + expect(paginated_result.records[2].guid).to eq(app_model1.guid) + expect(paginated_result.records[3].guid).to eq(app_model2.guid) + end + it 'orders by GUID as a secondary field when available' do options = { page: page, per_page: 2, order_by: 'created_at', order_direction: 'asc' } app_model1.update(guid: '1', created_at: '2019-12-25T13:00:00Z') diff --git a/spec/unit/messages/buildpack_upload_message_spec.rb b/spec/unit/messages/buildpack_upload_message_spec.rb index 97e689e4b94..fee9b05a1c3 100644 --- a/spec/unit/messages/buildpack_upload_message_spec.rb +++ b/spec/unit/messages/buildpack_upload_message_spec.rb @@ -156,8 +156,8 @@ module VCAP::CloudController allow(File).to receive(:read).and_return("PK\x03\x04".force_encoding('binary')) upload_message = BuildpackUploadMessage.new(opts, lifecycle) expect(upload_message).not_to be_valid - expect(upload_message.errors.full_messages[0]).to - include('buildpack.zip is not a gzip archive or cnb file. Buildpacks of lifecycle "cnb" must be valid gzip archives or cnb files.') + expect(upload_message.errors.full_messages[0]). + to include('buildpack.zip is not a gzip archive or cnb file. Buildpacks of lifecycle "cnb" must be valid gzip archives or cnb files.') end end