From 97d9d6ce7762d3893658a26e931f31d9b716ae18 Mon Sep 17 00:00:00 2001 From: Ashvin Sharma Date: Thu, 7 Aug 2025 15:13:01 +0530 Subject: [PATCH] Validate only kubernetes objects using JSON schema Unlike previously, when the sequence in the array was also validated. This is done because JSON schema cannot handle validate an array for relative ordering of elements when there are arbitrary amount of items present. In other words, unless we do not know the exact amount of items order of the items in an array cannot be validated. --- .../json_schemas/workspaces_kubernetes.json | 163 ++++-------------- .../desired_config_array_validator.rb | 107 ++++++++++++ .../workspace_operations/desired_config.rb | 1 + .../example.desired_config.json | 33 ++++ .../desired_config_spec.rb | 8 +- .../fast_spec_helper_support.rb | 1 + .../desired_config_array_validator_spec.rb | 64 +++++++ 7 files changed, 248 insertions(+), 129 deletions(-) create mode 100644 ee/app/validators/remote_development/desired_config_array_validator.rb create mode 100644 ee/spec/validators/remote_development/desired_config_array_validator_spec.rb diff --git a/ee/app/validators/json_schemas/workspaces_kubernetes.json b/ee/app/validators/json_schemas/workspaces_kubernetes.json index c7732393179360..47683746b34934 100644 --- a/ee/app/validators/json_schemas/workspaces_kubernetes.json +++ b/ee/app/validators/json_schemas/workspaces_kubernetes.json @@ -1,131 +1,44 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", "description": "This schema contains the objects created by devfile-gem in the order they are supposed to appear.", - "anyOf": [ - { - "type": "array", - "unevaluatedItems": false, - "prefixItems": [ - { - "$ref": "#/definitions/workspace_inventory_config_map" - }, - { - "$ref": "#/definitions/deployment" - }, - { - "$ref": "#/definitions/service" - }, - { - "$ref": "#/definitions/persistent_volume_claim" - }, - { - "$ref": "#/definitions/service_account" - }, - { - "$ref": "#/definitions/network_policy" - }, - { - "$ref": "#/definitions/scripts_config_map" - }, - { - "$ref": "#/definitions/secrets_inventory_config_map" - }, - { - "$ref": "#/definitions/resource_quota" - }, - { - "$ref": "#/definitions/env_var_secret" - }, - { - "$ref": "#/definitions/file_secret" - } - ] - }, - { - "type": "array", - "unevaluatedItems": false, - "prefixItems": [ - { - "$ref": "#/definitions/workspace_inventory_config_map" - }, - { - "$ref": "#/definitions/deployment" - }, - { - "$ref": "#/definitions/service" - }, - { - "$ref": "#/definitions/persistent_volume_claim" - }, - { - "$ref": "#/definitions/service_account" - }, - { - "$ref": "#/definitions/network_policy" - }, - { - "$ref": "#/definitions/scripts_config_map" - }, - { - "$ref": "#/definitions/secrets_inventory_config_map" - }, - { - "$ref": "#/definitions/env_var_secret" - }, - { - "$ref": "#/definitions/file_secret" - } - ] - }, - { - "type": "array", - "unevaluatedItems": false, - "prefixItems": [ - { - "$ref": "#/definitions/workspace_inventory_config_map" - }, - { - "$ref": "#/definitions/secrets_inventory_config_map" - } - ] - }, - { - "type": "array", - "unevaluatedItems": false, - "prefixItems": [ - { - "$ref": "#/definitions/workspace_inventory_config_map" - }, - { - "$ref": "#/definitions/deployment" - }, - { - "$ref": "#/definitions/service" - }, - { - "$ref": "#/definitions/persistent_volume_claim" - }, - { - "$ref": "#/definitions/service_account" - }, - { - "$ref": "#/definitions/network_policy" - }, - { - "$ref": "#/definitions/secrets_inventory_config_map" - }, - { - "$ref": "#/definitions/resource_quota" - }, - { - "$ref": "#/definitions/env_var_secret" - }, - { - "$ref": "#/definitions/file_secret" - } - ] - } - ], + "type": "array", + "items": { + "anyOf": [ + { + "$ref": "#/definitions/workspace_inventory_config_map" + }, + { + "$ref": "#/definitions/deployment" + }, + { + "$ref": "#/definitions/service" + }, + { + "$ref": "#/definitions/persistent_volume_claim" + }, + { + "$ref": "#/definitions/service_account" + }, + { + "$ref": "#/definitions/network_policy" + }, + { + "$ref": "#/definitions/scripts_config_map" + }, + { + "$ref": "#/definitions/secrets_inventory_config_map" + }, + { + "$ref": "#/definitions/resource_quota" + }, + { + "$ref": "#/definitions/env_var_secret" + }, + { + "$ref": "#/definitions/file_secret" + } + ] + }, "additionalProperties": false, "definitions": { "workspace_inventory_config_map": { diff --git a/ee/app/validators/remote_development/desired_config_array_validator.rb b/ee/app/validators/remote_development/desired_config_array_validator.rb new file mode 100644 index 00000000000000..4a254251c24133 --- /dev/null +++ b/ee/app/validators/remote_development/desired_config_array_validator.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +module RemoteDevelopment + class DesiredConfigArrayValidator < ActiveModel::EachValidator + EXPECTED_SEQUENCE = [ + { kind: "ConfigMap", name_pattern: /workspace-inventory$/ }, + { kind: "Deployment", name_pattern: /.*/ }, + { kind: "Service", name_pattern: /.*/ }, + { kind: "PersistentVolumeClaim", name_pattern: /.*/ }, + { kind: "ServiceAccount", name_pattern: /.*/ }, + { kind: "NetworkPolicy", name_pattern: /.*/ }, + { kind: "ConfigMap", name_pattern: /scripts-configmap$/ }, + { kind: "ConfigMap", name_pattern: /secrets-inventory$/ }, + { kind: "ResourceQuota", name_pattern: /.*/ }, + { kind: "Secret", name_pattern: /env-var$/ }, + { kind: "Secret", name_pattern: /file$/ } + ].freeze + + # @param [RemoteDevelopment::DesiredConfig] record + # @param [Symbol] attribute + # @param [Array] value + # @return [void] The result is stored in the errors param + def validate_each(record, attribute, value) + unless value.is_a?(Array) + record.errors.add(attribute, "must be an array") + return + end + + if value.empty? + record.errors.add(attribute, "must not be empty") + return + end + + value = value.map(&:deep_symbolize_keys) + normalized_expected_array = normalize_expected_order(value) + normalized_config_array = value.map do |item| + item => { + kind: String => item_kind, + metadata: { + name: String => item_name + }, + ** + } + + "#{item_kind}/#{item_name}" + end + + validate_order(normalized_config_array, normalized_expected_array, record.errors, attribute) + end + + private + + # @param [Array] normalized_config_array + # @param [Array] normalized_expected_array + # @param [ActiveModel::Errors] errors - stores the validation results + # @param [Symbol] attribute_symbol - symbol of the attribute associated with the error + # @return [Void] + def validate_order(normalized_config_array, normalized_expected_array, errors, attribute_symbol) + normalized_config_array.each_with_index do |item, index| + expected_index = normalized_expected_array.index(item) + + if expected_index.nil? + errors.add(attribute_symbol, "item #{item} at index #{index} is unexpected") + next + end + + next if expected_index == index + + errors.add(attribute_symbol, "item #{item} at index #{index} must be at #{expected_index}") + end + end + + # This method normalizes the expected order of items based on the config array + # It creates a mapping of expected positions for each kind/name combination + # + # @param [Array] config_array The array of configuration items to normalize + # @return [Array] An array with expected order of items + def normalize_expected_order(config_array) + expected_positions = [] + + EXPECTED_SEQUENCE.each_with_index do |expected, _| + config_array.each do |item| + item => { + kind: String => item_kind, + metadata: { + name: String => item_name + } + } + + expected_positions << "#{item_kind}/#{item_name}" if matches?(item_kind, item_name, expected) + end + end + + expected_positions + end + + # Validates if the given configuration matches the expected value + # + # @param [String] kind The type of configuration to validate + # @param [String] name The name of the configuration item + # @param [Object] expected The expected value to match against. See {#EXPECTED_SEQUENCE} above. + # @return [Boolean] Returns true if the configuration matches the expected value, false otherwise + def matches?(kind, name, expected) + kind == expected[:kind] && expected[:name_pattern].match?(name) + end + end +end diff --git a/ee/lib/remote_development/workspace_operations/desired_config.rb b/ee/lib/remote_development/workspace_operations/desired_config.rb index cc3b15076599d0..9bb737bb1daa7e 100644 --- a/ee/lib/remote_development/workspace_operations/desired_config.rb +++ b/ee/lib/remote_development/workspace_operations/desired_config.rb @@ -20,6 +20,7 @@ class DesiredConfig detail_errors: true, size_limit: 64.kilobytes } + validates :desired_config_array, 'remote_development/desired_config_array': true # @param [DesiredConfig] other # @return [Boolean] diff --git a/ee/spec/fixtures/remote_development/example.desired_config.json b/ee/spec/fixtures/remote_development/example.desired_config.json index f6f91165c69a01..70cbf6f9b6d087 100644 --- a/ee/spec/fixtures/remote_development/example.desired_config.json +++ b/ee/spec/fixtures/remote_development/example.desired_config.json @@ -332,6 +332,39 @@ }, "status": {} }, + { + "apiVersion": "v1", + "kind": "PersistentVolumeClaim", + "metadata": { + "annotations": { + "environment": "production", + "team": "engineering", + "config.k8s.io/owning-inventory": "workspace-991-990-fedcba-workspace-inventory", + "workspaces.gitlab.com/host-template": "{{.port}}-workspace-991-990-fedcba.workspaces.dev.test", + "workspaces.gitlab.com/id": "993", + "workspaces.gitlab.com/max-resources-per-workspace-sha256": "24aefc317e11db538ede450d1773e273966b9801b988d49e1219f2a9bf8e7f66" + }, + "creationTimestamp": null, + "labels": { + "app": "workspace", + "tier": "development", + "agent.gitlab.com/id": "991" + }, + "name": "workspace-991-990-fedcba-gl-workspace-custom-pvc", + "namespace": "gl-rd-ns-991-990-fedcba" + }, + "spec": { + "accessModes": [ + "ReadWriteOnce" + ], + "resources": { + "requests": { + "storage": "50Gi" + } + } + }, + "status": {} + }, { "apiVersion": "v1", "automountServiceAccountToken": false, diff --git a/ee/spec/lib/remote_development/workspace_operations/desired_config_spec.rb b/ee/spec/lib/remote_development/workspace_operations/desired_config_spec.rb index 5e1f90997a7f1a..e434cab9b2da4c 100644 --- a/ee/spec/lib/remote_development/workspace_operations/desired_config_spec.rb +++ b/ee/spec/lib/remote_development/workspace_operations/desired_config_spec.rb @@ -46,7 +46,7 @@ end end - it 'fails' do + it 'fails', :unlimited_max_formatted_output_length do expect(desired_config).to be_invalid # This validation does not fail for other kinds except for ConfigMap @@ -54,8 +54,8 @@ expect(desired_config.errors[:desired_config_array]) .to include( "object property at `/0/invalid-field` is a disallowed additional property", - "object property at `/6/invalid-field` is a disallowed additional property", - "object property at `/7/invalid-field` is a disallowed additional property" + "object property at `/7/invalid-field` is a disallowed additional property", + "object property at `/8/invalid-field` is a disallowed additional property" ) end end @@ -122,7 +122,7 @@ let(:expected_difference) do [ [ - "-", "[10]", + "-", "[11]", { apiVersion: "v1", data: {}, diff --git a/ee/spec/support/fast_spec/remote_development/fast_spec_helper_support.rb b/ee/spec/support/fast_spec/remote_development/fast_spec_helper_support.rb index fdfe3933a4aa96..c37b5949901bd7 100644 --- a/ee/spec/support/fast_spec/remote_development/fast_spec_helper_support.rb +++ b/ee/spec/support/fast_spec/remote_development/fast_spec_helper_support.rb @@ -13,6 +13,7 @@ require_relative '../../../../app/models/remote_development/enums/workspace_variable' require_relative '../../../../app/models/remote_development/workspace_state_helpers' require_relative '../../../../app/validators/ee/json_schema_validator' +require_relative '../../../../app/validators/remote_development/desired_config_array_validator' require_relative '../../../../lib/remote_development/files' require_relative '../../../../lib/remote_development/remote_development_constants' require_relative '../../../../lib/remote_development/workspace_operations/create/create_constants' diff --git a/ee/spec/validators/remote_development/desired_config_array_validator_spec.rb b/ee/spec/validators/remote_development/desired_config_array_validator_spec.rb new file mode 100644 index 00000000000000..a721c374420f6a --- /dev/null +++ b/ee/spec/validators/remote_development/desired_config_array_validator_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoteDevelopment::DesiredConfigArrayValidator, feature_category: :workspaces do + include_context "with remote development shared fixtures" + let(:model) do + Class.new do + # @return [String] + def self.name + "DesiredConfigArrayValidatorTest" + end + + include ActiveModel::Model + include ActiveModel::Validations + + attr_accessor :desired_config_array + alias_method :desired_config_before_type_cast, :desired_config_array + + validates :desired_config_array, 'remote_development/desired_config_array': true + end.new + end + + let(:desired_config_array_with_jumbled_items) do + items = create_desired_config_array + items[0], items[-1] = items[-1], items[0] + items[6], items[7] = items[7], items[6] + items + end + + let(:desired_config_array_with_unexpected_items) do + items = create_desired_config_array + items << { kind: "UnexpectedKind", metadata: { name: "unexpected-item" } } + items + end + + let(:desired_config_array_with_missing_items) { create_desired_config_array.tap { |items| items.delete_at(3) } } + + using RSpec::Parameterized::TableSyntax + + where(:desired_config_array, :validity, :errors) do + # rubocop:disable Layout/LineLength -- The RSpec table syntax often requires long lines for errors + # @formatter:off - Turn off RubyMine autoformatting + create_desired_config_array | true | {} + ref(:desired_config_array_with_missing_items) | true | {} + ref(:desired_config_array_with_unexpected_items) | false | { desired_config_array: ["item UnexpectedKind/unexpected-item at index 12 is unexpected"] } + ref(:desired_config_array_with_jumbled_items) | false | { desired_config_array: ["item Secret/workspace-991-990-fedcba-file at index 0 must be at 11", "item ConfigMap/workspace-991-990-fedcba-scripts-configmap at index 6 must be at 7", "item NetworkPolicy/workspace-991-990-fedcba at index 7 must be at 6", "item ConfigMap/workspace-991-990-fedcba-workspace-inventory at index 11 must be at 0"] } + nil | false | { desired_config_array: ['must be an array'] } + {} | false | { desired_config_array: ['must be an array'] } + [] | false | { desired_config_array: ['must not be empty'] } + # @formatter:on + # rubocop:enable Layout/LineLength + end + + with_them do + before do + model.desired_config_array = desired_config_array + model.validate + end + + it { expect(model.valid?).to eq(validity) } + it { expect(model.errors.messages).to eq(errors) } + end +end -- GitLab