diff --git a/ee/lib/remote_development/workspaces/create/devfile_flattener.rb b/ee/lib/remote_development/workspaces/create/devfile_flattener.rb index 4315ba43a56d5cb82438ea1c7a3f4bc0765a4b46..1383c8f4f4ffe4265e2677051563c360658d7cb7 100644 --- a/ee/lib/remote_development/workspaces/create/devfile_flattener.rb +++ b/ee/lib/remote_development/workspaces/create/devfile_flattener.rb @@ -11,9 +11,11 @@ class DevfileFlattener def self.flatten(value) value => { devfile_yaml: String => devfile_yaml } - # NOTE: We do not attempt to rescue any exceptions from Devfile::Parser.flatten here because we assume that - # the input devfile_yaml has already been fully validated by the pre-flatten devfile validator. - flattened_devfile_yaml = Devfile::Parser.flatten(devfile_yaml) + begin + flattened_devfile_yaml = Devfile::Parser.flatten(devfile_yaml) + rescue Devfile::CliError => e + return Result.err(WorkspaceCreateDevfileFlattenFailed.new(details: e.message)) + end # NOTE: We do not attempt to rescue any exceptions from YAML.safe_load here because we assume that the # Devfile::Parser gem will not produce invalid YAML. We own the gem, and will fix and add any regression @@ -23,7 +25,7 @@ def self.flatten(value) processed_devfile['components'] ||= [] - value.merge(processed_devfile: processed_devfile) + Result.ok(value.merge(processed_devfile: processed_devfile)) end end end diff --git a/ee/lib/remote_development/workspaces/create/main.rb b/ee/lib/remote_development/workspaces/create/main.rb index 71afb6f908c6b45e01567a002911e188e46b26df..75e9c4c9ee90142528ea5e10886878175e857738 100644 --- a/ee/lib/remote_development/workspaces/create/main.rb +++ b/ee/lib/remote_development/workspaces/create/main.rb @@ -20,7 +20,7 @@ def self.main(value) .and_then(Authorizer.method(:authorize)) .and_then(DevfileFetcher.method(:fetch)) .and_then(PreFlattenDevfileValidator.method(:validate)) - .map(DevfileFlattener.method(:flatten)) + .and_then(DevfileFlattener.method(:flatten)) .and_then(PostFlattenDevfileValidator.method(:validate)) .map(VolumeDefiner.method(:define)) .map(VolumeComponentInjector.method(:inject)) diff --git a/ee/lib/remote_development/workspaces/reconcile/desired_config_generator.rb b/ee/lib/remote_development/workspaces/reconcile/desired_config_generator.rb index 5a1883d3a2d3566920471808e3fc718e61ec1c8d..1db8815d328f4711e53d847135f2ea10c3422faa 100644 --- a/ee/lib/remote_development/workspaces/reconcile/desired_config_generator.rb +++ b/ee/lib/remote_development/workspaces/reconcile/desired_config_generator.rb @@ -4,7 +4,9 @@ module RemoteDevelopment module Workspaces module Reconcile # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 - # noinspection RubyInstanceMethodNamingConvention - See https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/code-inspection/why-are-there-noinspection-comments/ + # rubocop:disable Layout/LineLength + # noinspection RubyInstanceMethodNamingConvention,RubyLocalVariableNamingConvention,RubyParameterNamingConvention - See https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/code-inspection/why-are-there-noinspection-comments/ + # rubocop:enable Layout/LineLength class DesiredConfigGenerator include States @@ -40,6 +42,12 @@ def generate_desired_config(workspace:) annotations: annotations, user: user ) + # If we got no resources back from the devfile parser, this indicates some error was encountered in parsing + # the processed_devfile. So we return an empty array which will result in no updates being applied by the + # agent. We should not continue on and try to add anything else to the resources, as this would result + # in an invalid configuration being applied to the cluster. + return [] if workspace_resources.empty? + workspace_resources.insert(0, workspace_inventory_config_map) remote_development_agent_config = workspace.agent.remote_development_agent_config diff --git a/ee/lib/remote_development/workspaces/reconcile/devfile_parser.rb b/ee/lib/remote_development/workspaces/reconcile/devfile_parser.rb index 0ad8b87678ad980da08bb2949b9d845523fb437c..7c30bcd3f2e8d33d0d055c638aa7a40cfb58282f 100644 --- a/ee/lib/remote_development/workspaces/reconcile/devfile_parser.rb +++ b/ee/lib/remote_development/workspaces/reconcile/devfile_parser.rb @@ -17,16 +17,28 @@ class DevfileParser # @param [User] user # @return [Array] def get_all(processed_devfile:, name:, namespace:, replicas:, domain_template:, labels:, annotations:, user:) - workspace_resources_yaml = Devfile::Parser.get_all( - processed_devfile, - name, - namespace, - YAML.dump(labels.deep_stringify_keys), - YAML.dump(annotations.deep_stringify_keys), - replicas, - domain_template, - 'none' - ) + begin + workspace_resources_yaml = Devfile::Parser.get_all( + processed_devfile, + name, + namespace, + YAML.dump(labels.deep_stringify_keys), + YAML.dump(annotations.deep_stringify_keys), + replicas, + domain_template, + 'none' + ) + rescue Devfile::CliError => e + logger.warn( + message: 'Error parsing devfile with Devfile::Parser.get_all', + error_type: 'reconcile_devfile_parser_error', + workspace_name: name, + workspace_namespace: namespace, + devfile_parser_error: e.message + ) + return [] + end + workspace_resources = YAML.load_stream(workspace_resources_yaml) workspace_resources = set_security_context(workspace_resources: workspace_resources) workspace_resources = set_git_configuration(workspace_resources: workspace_resources, user: user) @@ -38,6 +50,11 @@ def get_all(processed_devfile:, name:, namespace:, replicas:, domain_template:, private + # @return [RemoteDevelopment::Logger] + def logger + @logger ||= RemoteDevelopment::Logger.build + end + # Devfile library allows specifying the security context of pods/containers as mentioned in # https://github.com/devfile/api/issues/920 through `pod-overrides` and `container-overrides` attributes. # However, https://github.com/devfile/library/pull/158 which is implementing this feature, diff --git a/ee/spec/fixtures/remote_development/example.invalid-extra-field-devfile.yaml b/ee/spec/fixtures/remote_development/example.invalid-extra-field-devfile.yaml new file mode 100644 index 0000000000000000000000000000000000000000..a352ee627d86b1b40514e5fba4d33eb0c3ec32fe --- /dev/null +++ b/ee/spec/fixtures/remote_development/example.invalid-extra-field-devfile.yaml @@ -0,0 +1,14 @@ +random: field +schemaVersion: 2.1.0 +metadata: + name: go + language: go +components: +- name: runtime + container: + endpoints: + - name: http + targetPort: 8080 + image: quay.io/devfile/golang:latest + memoryLimit: 1024Mi + mountSources: true diff --git a/ee/spec/lib/remote_development/fast_spec_helper.rb b/ee/spec/lib/remote_development/fast_spec_helper.rb index 24987e1cdca8f807958b3b797bacc005ef99324a..040f4bfedb753832afe06e498e74f6a3fe1cfbb5 100644 --- a/ee/spec/lib/remote_development/fast_spec_helper.rb +++ b/ee/spec/lib/remote_development/fast_spec_helper.rb @@ -22,6 +22,7 @@ require 'rspec-parameterized' require 'json_schemer' +require 'devfile' RSpec.configure do |config| config.include NextInstanceOf diff --git a/ee/spec/lib/remote_development/workspaces/create/devfile_flattener_spec.rb b/ee/spec/lib/remote_development/workspaces/create/devfile_flattener_spec.rb index 0b618dc0fed0cfa0dcde0c108016fc306432ce0f..1f984317964cd380699995015e452e637fb0072e 100644 --- a/ee/spec/lib/remote_development/workspaces/create/devfile_flattener_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/devfile_flattener_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require "spec_helper" +require_relative "../../fast_spec_helper" RSpec.describe RemoteDevelopment::Workspaces::Create::DevfileFlattener, feature_category: :remote_development do include_context 'with remote development shared fixtures' @@ -15,10 +15,12 @@ it "merges flattened devfile to passed value" do expect(subject).to eq( - { - devfile_yaml: devfile_yaml, - processed_devfile: expected_processed_devfile - } + Result.ok( + { + devfile_yaml: devfile_yaml, + processed_devfile: expected_processed_devfile + } + ) ) end @@ -29,7 +31,27 @@ end it "adds an empty components entry" do - expect(subject.fetch(:processed_devfile)).to eq(expected_processed_devfile) + expect(subject).to eq( + Result.ok( + { + devfile_yaml: devfile_yaml, + processed_devfile: expected_processed_devfile + } + ) + ) + end + end + + context "when flatten raises a Devfile::CliError" do + let(:devfile_yaml) { read_devfile('example.invalid-extra-field-devfile.yaml') } + + it "returns the error message from the CLI" do + expected_error_message = + "failed to populateAndParseDevfile: invalid devfile schema. errors :\n" \ + "- (root): Additional property random is not allowed\n" + message = subject.unwrap_err + expect(message).to be_a(RemoteDevelopment::Messages::WorkspaceCreateDevfileFlattenFailed) + expect(message.context).to eq(details: expected_error_message) end end end diff --git a/ee/spec/lib/remote_development/workspaces/create/main_spec.rb b/ee/spec/lib/remote_development/workspaces/create/main_spec.rb index 4a9a121994175077f07efd5e0ac1e422056e7927..58bf97b476c6e0cce37c5103783b2f0f1b4c295e 100644 --- a/ee/spec/lib/remote_development/workspaces/create/main_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/create/main_spec.rb @@ -134,11 +134,10 @@ stub_methods_to_return_ok_result( authorizer_method, devfile_fetcher_method, - pre_flatten_devfile_validator_method - ) - stub_methods_to_return_value( + pre_flatten_devfile_validator_method, devfile_flattener_method ) + stub_methods_to_return_err_result( method: post_flatten_devfile_validator_method, message_class: RemoteDevelopment::Messages::WorkspaceCreatePostFlattenDevfileValidationFailed @@ -167,7 +166,6 @@ post_flatten_devfile_validator_method ) stub_methods_to_return_value( - devfile_flattener_method, volume_definer_method, volume_component_injector_method, editor_component_injector_method, @@ -203,7 +201,6 @@ post_flatten_devfile_validator_method ) stub_methods_to_return_value( - devfile_flattener_method, volume_definer_method, volume_component_injector_method, editor_component_injector_method, @@ -234,7 +231,6 @@ post_flatten_devfile_validator_method ) stub_methods_to_return_value( - devfile_flattener_method, volume_definer_method, volume_component_injector_method, editor_component_injector_method, diff --git a/ee/spec/lib/remote_development/workspaces/reconcile/desired_config_generator_spec.rb b/ee/spec/lib/remote_development/workspaces/reconcile/desired_config_generator_spec.rb index 25503d1c6251a5053446e0a433a0d09ca068e1f4..a5aaadf998b406b597b477a73af28da3a2f9d71b 100644 --- a/ee/spec/lib/remote_development/workspaces/reconcile/desired_config_generator_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/reconcile/desired_config_generator_spec.rb @@ -78,5 +78,19 @@ expect(workspace_resources).to eq(expected_config) end end + + context 'when DevfileParser returns empty array' do + before do + allow_next_instance_of(RemoteDevelopment::Workspaces::Reconcile::DevfileParser) do |instance| + allow(instance).to receive(:get_all).and_return([]) + end + end + + it 'returns an empty array' do + workspace_resources = subject.generate_desired_config(workspace: workspace) + + expect(workspace_resources).to eq([]) + end + end end end diff --git a/ee/spec/lib/remote_development/workspaces/reconcile/devfile_parser_spec.rb b/ee/spec/lib/remote_development/workspaces/reconcile/devfile_parser_spec.rb index d5fbb521d0a12edd2ebaaf14cecaa842ae340a62..bae91592013125d1d298cc98db6a14d9b815b833 100644 --- a/ee/spec/lib/remote_development/workspaces/reconcile/devfile_parser_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/reconcile/devfile_parser_spec.rb @@ -53,4 +53,36 @@ expect(workspace_resources).to eq(expected_workspace_resources) end end + + context "when Devfile::CliError is raised" do + let(:logger) { instance_double(RemoteDevelopment::Logger) } + + before do + allow(Devfile::Parser).to receive(:get_all).and_raise(Devfile::CliError.new("some error")) + allow(RemoteDevelopment::Logger).to receive(:build) { logger } + end + + it "logs the error" do + expect(logger).to receive(:warn).with( + message: 'Error parsing devfile with Devfile::Parser.get_all', + error_type: 'reconcile_devfile_parser_error', + workspace_name: workspace.name, + workspace_namespace: workspace.namespace, + devfile_parser_error: "some error" + ) + + workspace_resources = subject.get_all( + processed_devfile: "", + name: workspace.name, + namespace: workspace.namespace, + replicas: 1, + domain_template: "", + labels: {}, + annotations: {}, + user: user + ) + + expect(workspace_resources).to eq([]) + end + end end diff --git a/ee/spec/lib/remote_development/workspaces/reconcile/reconcile_processor_spec.rb b/ee/spec/lib/remote_development/workspaces/reconcile/reconcile_processor_spec.rb index ccfc612f7136036c4c168b5cd7d134d491d464b9..0d794ee567c020c0058077487360dba0c68870f2 100644 --- a/ee/spec/lib/remote_development/workspaces/reconcile/reconcile_processor_spec.rb +++ b/ee/spec/lib/remote_development/workspaces/reconcile/reconcile_processor_spec.rb @@ -170,6 +170,7 @@ .to eq(expected_deployment_resource_version) # test the config to apply first to get a more specific diff if it fails + # noinspection RubyLocalVariableNamingConvention - See https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/code-inspection/why-are-there-noinspection-comments/ provisioned_workspace_rails_info = workspace_rails_infos.detect { |info| info.fetch(:name) == workspace.name } # Since the workspace is now in Error state, the config should not be returned to the agent @@ -180,6 +181,75 @@ end end + context 'when only some workspaces fail in devfile flattener' do + let(:workspace) { create(:workspace, name: "workspace1", agent: agent, user: user) } + let(:invalid_devfile_yaml) { read_devfile('example.invalid-extra-field-devfile.yaml') } + let(:workspace2) do + create(:workspace, devfile: invalid_devfile_yaml, name: "workspace-failing-flatten", + agent: agent, user: user) + end + + let(:workspace2_agent_info) do + create_workspace_agent_info( + workspace_id: workspace2.id, + workspace_name: workspace2.name, + workspace_namespace: workspace2.namespace, + agent_id: workspace2.agent.id, + owning_inventory: owning_inventory, + resource_version: deployment_resource_version_from_agent, + previous_actual_state: previous_actual_state, + current_actual_state: current_actual_state, + workspace_exists: workspace_exists, + user_name: user.name, + user_email: user.email + ) + end + + # NOTE: Reverse the order so that the failing one is processed first and ensures that the second valid + # one is still processed successfully. + let(:workspace_agent_infos) { [workspace2_agent_info, workspace_agent_info] } + + let(:expected_workspace2_rails_info) do + { + name: workspace2.name, + namespace: workspace2.namespace, + desired_state: expected_desired_state, + actual_state: expected_actual_state, + deployment_resource_version: expected_deployment_resource_version, + config_to_apply: nil + } + end + + let(:expected_workspace_rails_infos) { [expected_workspace2_rails_info, expected_workspace_rails_info] } + + it 'returns proper workspace_rails_info entries' do + payload, error = subject.process( + agent: agent, + workspace_agent_infos: workspace_agent_infos, + update_type: update_type + ) + expect(error).to be_nil + workspace_rails_infos = payload.fetch(:workspace_rails_infos) + expect(workspace_rails_infos.length).to eq(2) + + workspace.reload + workspace2.reload + + expect(workspace.deployment_resource_version) + .to eq(expected_deployment_resource_version) + + expect(workspace2.deployment_resource_version) + .to eq(expected_deployment_resource_version) + + workspace2_rails_info = + workspace_rails_infos.detect { |info| info.fetch(:name) == workspace2.name } + expect(workspace2_rails_info.fetch(:config_to_apply)).to be_nil + + # then test everything in the infos + expect(workspace_rails_infos).to eq(expected_workspace_rails_infos) + end + end + context 'with timestamp precondition checks' do # NOTE: rubocop:disable RSpec/ExpectInHook could be avoided with a helper method or custom expectation, # but this works for now.