diff --git a/app/messages/app_manifest_message.rb b/app/messages/app_manifest_message.rb index 97802f9e361..1b126407b24 100644 --- a/app/messages/app_manifest_message.rb +++ b/app/messages/app_manifest_message.rb @@ -104,7 +104,7 @@ def sidecar_create_messages end def app_update_message - @app_update_message ||= AppUpdateMessage.new(app_lifecycle_hash) + @app_update_message ||= AppUpdateMessage.new(lifecycle: lifecycle_message) end def app_update_environment_variables_message diff --git a/app/messages/buildpack_lifecycle_data_message.rb b/app/messages/buildpack_lifecycle_data_message.rb index 66bf5ed234a..fe6328f3df4 100644 --- a/app/messages/buildpack_lifecycle_data_message.rb +++ b/app/messages/buildpack_lifecycle_data_message.rb @@ -40,7 +40,14 @@ def buildpacks_content def credentials_content return unless credentials.is_a?(Hash) - errors.add(:credentials, 'credentials value must be a hash') if credentials.any? { |_, v| !v.is_a?(Hash) } + credentials.each do |registry, creds| + unless creds.is_a?(Hash) + errors.add(:credentials, "for registry '#{registry}' must be a hash") + next + end + + errors.add(:credentials, "for registry '#{registry}' must include 'username' and 'password'") unless creds.key?('username') && creds.key?('password') + end end end end diff --git a/app/models/runtime/feature_flag.rb b/app/models/runtime/feature_flag.rb index e5d4fc634f7..054ef341f7a 100644 --- a/app/models/runtime/feature_flag.rb +++ b/app/models/runtime/feature_flag.rb @@ -14,6 +14,7 @@ class UndefinedFeatureFlagError < StandardError service_instance_creation: true, diego_docker: false, diego_cnb: false, + diego_custom_stacks: false, set_roles_by_username: true, unset_roles_by_username: true, task_creation: true, diff --git a/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_data_validator.rb b/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_data_validator.rb index 08bed855085..9bd571fcabf 100644 --- a/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_data_validator.rb +++ b/lib/cloud_controller/diego/lifecycles/buildpack_lifecycle_data_validator.rb @@ -8,6 +8,14 @@ class BuildpackLifecycleDataValidator validate :buildpacks_are_uri_or_nil validate :stack_exists_in_db + validate :custom_stack_requires_custom_buildpack + + def custom_stack_requires_custom_buildpack + return unless stack.is_a?(String) && stack.include?('docker://') + return if buildpack_infos.all?(&:custom?) + + errors.add(:buildpack, 'must be a custom buildpack when using a custom stack') + end def buildpacks_are_uri_or_nil buildpack_infos.each do |buildpack_info| @@ -24,7 +32,11 @@ def buildpacks_are_uri_or_nil end def stack_exists_in_db - errors.add(:stack, 'must be an existing stack') if stack.nil? + return if stack.nil? + return if stack.is_a?(String) && stack.include?('docker://') && FeatureFlag.enabled?(:diego_custom_stacks) + return if VCAP::CloudController::Stack.where(name: stack).any? + + errors.add(:stack, 'must be an existing stack') end end end diff --git a/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb b/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb index 671d0bd3d92..832083d3146 100644 --- a/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb +++ b/lib/cloud_controller/diego/lifecycles/lifecycle_base.rb @@ -17,7 +17,13 @@ def initialize(package, staging_message) delegate :valid?, :errors, to: :validator def staging_stack - requested_stack || app_stack || VCAP::CloudController::Stack.default.name + stack = requested_stack || app_stack || VCAP::CloudController::Stack.default.name + FeatureFlag.raise_unless_enabled!(:diego_custom_stacks) if stack.is_a?(String) && stack.include?('docker://') + stack + end + + def credentials + staging_message.lifecycle_data[:credentials] end private diff --git a/lib/cloud_controller/diego/task_recipe_builder.rb b/lib/cloud_controller/diego/task_recipe_builder.rb index 4395d6d0b96..90e8f2eeb72 100644 --- a/lib/cloud_controller/diego/task_recipe_builder.rb +++ b/lib/cloud_controller/diego/task_recipe_builder.rb @@ -91,12 +91,36 @@ def build_staging_task(config, staging_details) "app:#{staging_details.package.app_guid}" ] ), - image_username: staging_details.package.docker_username, - image_password: staging_details.package.docker_password, + image_username: image_username(staging_details), + image_password: image_password(staging_details), volume_mounted_files: ServiceBindingFilesBuilder.build(staging_details.package.app) }.compact) end + def image_username(staging_details) + return staging_details.package.docker_username if staging_details.package.docker_username.present? + return unless staging_details.lifecycle.respond_to?(:credentials) && staging_details.lifecycle.credentials.present? + + cred = get_credentials_for_stack(staging_details) + cred ? cred['user'] : nil + end + + def image_password(staging_details) + return staging_details.package.docker_password if staging_details.package.docker_password.present? + return unless staging_details.lifecycle.respond_to?(:credentials) && staging_details.lifecycle.credentials.present? + + cred = get_credentials_for_stack(staging_details) + cred ? cred['password'] : nil + end + + def get_credentials_for_stack(staging_details) + return nil unless staging_details.lifecycle.staging_stack.include?('docker://') + + stack_uri = URI.parse(staging_details.lifecycle.staging_stack) + host = stack_uri.host + staging_details.lifecycle.credentials[host] + end + private def metric_tags(source) diff --git a/spec/api/documentation/feature_flags_api_spec.rb b/spec/api/documentation/feature_flags_api_spec.rb index ce55fbd2c6b..5fe6740eb30 100644 --- a/spec/api/documentation/feature_flags_api_spec.rb +++ b/spec/api/documentation/feature_flags_api_spec.rb @@ -18,7 +18,7 @@ client.get '/v2/config/feature_flags', {}, headers expect(status).to eq(200) - expect(parsed_response.length).to eq(18) + expect(parsed_response.length).to eq(19) expect(parsed_response).to include( { 'name' => 'user_org_creation', diff --git a/spec/unit/actions/app_update_spec.rb b/spec/unit/actions/app_update_spec.rb index 2b2f9cc6a63..37a866cf14a 100644 --- a/spec/unit/actions/app_update_spec.rb +++ b/spec/unit/actions/app_update_spec.rb @@ -259,6 +259,40 @@ module VCAP::CloudController end end + context 'when the lifecycle is valid' do + let(:message) do + AppUpdateMessage.new({ + lifecycle: { + type: 'buildpack', + data: { buildpacks: ['http://new-buildpack.url', 'ruby'], stack: stack.name } + } + }) + end + + it 'does not raise an error' do + expect do + app_update.update(app_model, message, lifecycle) + end.not_to raise_error + end + end + + context 'when the lifecycle is valid' do + let(:message) do + AppUpdateMessage.new({ + lifecycle: { + type: 'buildpack', + data: { buildpacks: ['http://new-buildpack.url', 'ruby'], stack: stack.name } + } + }) + end + + it 'does not raise an error' do + expect do + app_update.update(app_model, message, lifecycle) + end.not_to raise_error + end + end + context 'when changing the lifecycle type' do let(:message) do AppUpdateMessage.new({ diff --git a/spec/unit/controllers/v3/apps_controller_spec.rb b/spec/unit/controllers/v3/apps_controller_spec.rb index 26c30bc185c..1e4f578a527 100644 --- a/spec/unit/controllers/v3/apps_controller_spec.rb +++ b/spec/unit/controllers/v3/apps_controller_spec.rb @@ -468,7 +468,7 @@ { name: 'some-name', relationships: { space: { data: { guid: space.guid } } }, - lifecycle: { type: 'cnb', data: { buildpacks: ['http://example.com'], credentials: { registry: { user: 'password' } } } } + lifecycle: { type: 'cnb', data: { buildpacks: ['http://example.com'], stack: 'cflinuxfs4', credentials: { 'docker.io' => { 'username' => 'user', 'password' => 'password' } } } } } end @@ -479,7 +479,7 @@ lifecycle_data = response_body['lifecycle']['data'] expect(response).to have_http_status :created - expect(lifecycle_data).to eq({ 'buildpacks' => ['http://example.com'], 'stack' => 'default-stack-name', 'credentials' => '***' }) + expect(lifecycle_data).to eq({ 'buildpacks' => ['http://example.com'], 'stack' => 'cflinuxfs4' }) end end end diff --git a/spec/unit/messages/build_create_message_spec.rb b/spec/unit/messages/build_create_message_spec.rb index 93ed5de3a41..f36f30b2f39 100644 --- a/spec/unit/messages/build_create_message_spec.rb +++ b/spec/unit/messages/build_create_message_spec.rb @@ -126,6 +126,62 @@ module VCAP::CloudController expect(message).not_to be_valid expect(message.errors[:lifecycle]).to include('Buildpacks can only contain strings') end + + context 'credentials' do + it 'is valid with valid credentials' do + params[:lifecycle] = { + type: 'buildpack', + data: { + stack: 'cflinuxfs4', + credentials: { + 'docker.io' => { + 'username' => 'user', + 'password' => 'password' + } + } + } + } + message = BuildCreateMessage.new(params) + expect(message).to be_valid + end + + it 'is invalid with invalid credentials' do + params[:lifecycle] = { + type: 'buildpack', + data: { + stack: 'cflinuxfs4', + credentials: { + 'docker.io' => { + 'username' => 'user' + } + } + } + } + message = BuildCreateMessage.new(params) + expect(message).not_to be_valid + expect(message.errors[:lifecycle]).to include("credentials for docker.io must include 'username' and 'password'") + end + end + + context 'when a custom stack is used with a system buildpack' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'diego_custom_stacks', enabled: true) + VCAP::CloudController::Buildpack.make(name: 'ruby_buildpack') + end + + it 'is not valid' do + params[:lifecycle] = { + type: 'buildpack', + data: { + stack: 'docker://cloudfoundry/cflinuxfs4', + buildpacks: ['ruby_buildpack'] + } + } + message = BuildCreateMessage.new(params) + expect(message).not_to be_valid + expect(message.errors[:lifecycle]).to include('Buildpack must be a custom buildpack when using a custom stack') + end + end end describe 'docker lifecycle' do diff --git a/spec/unit/models/runtime/feature_flag_spec.rb b/spec/unit/models/runtime/feature_flag_spec.rb index 12a1e0a1449..30bc3dfa60c 100644 --- a/spec/unit/models/runtime/feature_flag_spec.rb +++ b/spec/unit/models/runtime/feature_flag_spec.rb @@ -168,6 +168,13 @@ module VCAP::CloudController end end end + + context 'when the diego_custom_stacks feature flag is not overridden' do + it 'returns the default value' do + expect(FeatureFlag.enabled?(:diego_custom_stacks)).to be(false) + expect(FeatureFlag.disabled?(:diego_custom_stacks)).to be(true) + end + end end describe '.raise_unless_enabled!' do