# HG changeset patch # User Sami Hiltunen <shiltunen@gitlab.com> # Date 1677107807 0 # Wed Feb 22 23:16:47 2023 +0000 # Node ID a0f05d22ef782cfc26ae4d2d61a13748d15ba9a4 # Parent 2ba2d18b947d10346011cf2ac6c90a358032a526 Match Gitaly configuration format in Omnibus The configuration formats of Gitaly and Omnibus deviate from each other creating a few problems: 1. Configuration keys for the same option differ between the context of Omnibus and Gitaly creating confusion for the user. 2. Each new key added requires duplicate work to map the different keys into Omnibus. This change introduces gitaly['configuration'] matching configuration in Omnibus with Gitaly's own format. The previous configuration keys are remapped into this new format to assist administrators during the transition period. The configuration values are now serialized directly into TOML instead of using a template. Removes unneeded values allowing Gitaly's own source to be the single source of truth for configuration. Updates tests because the new serialization method does not output empty config sections. Guards are included to prevent errors with latency configurations that must be arrays where the previous configuration expected a string until such time as Gitaly's own validation subroutines are implemented. Changelog: changed diff --git a/Gemfile b/Gemfile --- a/Gemfile +++ b/Gemfile @@ -41,6 +41,7 @@ gem 'yard' gem 'toml-rb' gem 'retriable' +gem "tomlib", "~> 0.5.0" group :packagecloud, optional: true do gem 'package_cloud' diff --git a/Gemfile.lock b/Gemfile.lock --- a/Gemfile.lock +++ b/Gemfile.lock @@ -479,6 +479,7 @@ thread_safe (0.3.6) toml-rb (2.2.0) citrus (~> 3.0, > 3.0) + tomlib (0.5.0) tomlrb (1.3.0) train-core (3.9.2) addressable (~> 2.5) @@ -584,6 +585,7 @@ simplecov-cobertura thor (~> 1.2) toml-rb + tomlib (~> 0.5.0) yard BUNDLED WITH diff --git a/config/patches/tomlib/license/add-license-file.patch b/config/patches/tomlib/license/add-license-file.patch new file mode 100644 --- /dev/null +++ b/config/patches/tomlib/license/add-license-file.patch @@ -0,0 +1,27 @@ +diff --git a/LICENSE.txt b/LICENSE.txt +new file mode 100644 +index 000000000..82cf5177b +--- /dev/null ++++ b/LICENSE.txt +@@ -0,0 +1,21 @@ ++The MIT License (MIT) ++ ++Copyright (c) 2022 Kamil Giszczak ++ ++Permission is hereby granted, free of charge, to any person obtaining a copy ++of this software and associated documentation files (the "Software"), to deal ++in the Software without restriction, including without limitation the rights ++to use, copy, modify, merge, publish, distribute, sublicense, and/or sell ++copies of the Software, and to permit persons to whom the Software is ++furnished to do so, subject to the following conditions: ++ ++The above copyright notice and this permission notice shall be included in ++all copies or substantial portions of the Software. ++ ++THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR ++IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, ++FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE ++AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER ++LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, ++OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN ++THE SOFTWARE. diff --git a/config/software/gitaly.rb b/config/software/gitaly.rb --- a/config/software/gitaly.rb +++ b/config/software/gitaly.rb @@ -30,6 +30,7 @@ dependency 'ruby' dependency 'bundler' dependency 'libicu' +dependency 'tomlib' # Technically, gitaly depends on git also. But because of how omnibus arranges # components to be built, this causes git to be built early in the process. But diff --git a/config/software/tomlib.rb b/config/software/tomlib.rb new file mode 100644 --- /dev/null +++ b/config/software/tomlib.rb @@ -0,0 +1,36 @@ +# +# Copyright 2016-2023 GitLab Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name 'tomlib' + +# The version here should be in agreement with /Gemfile.lock so that our rspec +# testing stays consistent with the package contents. +default_version '0.5.0' + +license 'MIT' +license_file 'LICENSE.txt' + +skip_transitive_dependency_licensing true + +dependency 'ruby' + +build do + patch source: "license/add-license-file.patch" + env = with_standard_compiler_flags(with_embedded_path) + + gem 'install tomlib' \ + " --version '#{version}'", env: env +end diff --git a/files/gitlab-cookbooks/gitaly/attributes/default.rb b/files/gitlab-cookbooks/gitaly/attributes/default.rb --- a/files/gitlab-cookbooks/gitaly/attributes/default.rb +++ b/files/gitlab-cookbooks/gitaly/attributes/default.rb @@ -1,53 +1,24 @@ default['gitaly']['enable'] = false default['gitaly']['ha'] = false default['gitaly']['dir'] = "/var/opt/gitlab/gitaly" -default['gitaly']['runtime_dir'] = "#{node['gitaly']['dir']}/run" -default['gitaly']['log_directory'] = "/var/log/gitlab/gitaly" default['gitaly']['env_directory'] = "/opt/gitlab/etc/gitaly/env" -default['gitaly']['graceful_restart_timeout'] = nil # default['gitaly']['env'] is set in ../recipes/enable.rb default['gitaly']['bin_path'] = "/opt/gitlab/embedded/bin/gitaly" -default['gitaly']['socket_path'] = "#{node['gitaly']['dir']}/gitaly.socket" -default['gitaly']['listen_addr'] = nil -default['gitaly']['tls_listen_addr'] = nil -default['gitaly']['certificate_path'] = nil -default['gitaly']['key_path'] = nil -default['gitaly']['gpg_signing_key_path'] = nil -default['gitaly']['prometheus_listen_addr'] = "localhost:9236" -default['gitaly']['logging_level'] = nil -default['gitaly']['logging_format'] = "json" -default['gitaly']['logging_sentry_dsn'] = nil -default['gitaly']['logging_ruby_sentry_dsn'] = nil -default['gitaly']['logging_sentry_environment'] = nil -default['gitaly']['prometheus_grpc_latency_buckets'] = nil default['gitaly']['storage'] = [] -default['gitaly']['auth_token'] = nil -default['gitaly']['auth_transitioning'] = false -default['gitaly']['git_catfile_cache_size'] = nil -default['gitaly']['git_bin_path'] = "#{node['package']['install-dir']}/embedded/bin/git" -default['gitaly']['use_bundled_git'] = true default['gitaly']['open_files_ulimit'] = 15000 -default['gitaly']['ruby_max_rss'] = nil -default['gitaly']['ruby_graceful_restart_timeout'] = nil -default['gitaly']['ruby_restart_delay'] = nil -default['gitaly']['ruby_num_workers'] = nil -default['gitaly']['concurrency'] = nil -default['gitaly']['rate_limiting'] = nil -default['gitaly']['daily_maintenance_start_hour'] = nil -default['gitaly']['daily_maintenance_start_minute'] = nil -default['gitaly']['daily_maintenance_duration'] = nil -default['gitaly']['daily_maintenance_storages'] = nil -default['gitaly']['daily_maintenance_disabled'] = false -default['gitaly']['cgroups_mountpoint'] = nil -default['gitaly']['cgroups_hierarchy_root'] = nil -default['gitaly']['cgroups_memory_bytes'] = nil -default['gitaly']['cgroups_cpu_shares'] = nil -default['gitaly']['cgroups_repositories_count'] = nil -default['gitaly']['cgroups_repositories_memory_bytes'] = nil -default['gitaly']['cgroups_repositories_cpu_shares'] = nil -default['gitaly']['pack_objects_cache_enabled'] = false -default['gitaly']['pack_objects_cache_dir'] = nil -default['gitaly']['pack_objects_cache_max_age'] = nil default['gitaly']['consul_service_name'] = 'gitaly' -default['gitaly']['custom_hooks_dir'] = nil default['gitaly']['consul_service_meta'] = nil + +default['gitaly']['configuration'] = { + runtime_dir: "#{node['gitaly']['dir']}/run", + socket_path: "#{node['gitaly']['dir']}/gitaly.socket", + prometheus_listen_addr: 'localhost:9236', + logging: { + dir: '/var/log/gitlab/gitaly', + format: 'json' + }, + git: { + use_bundled_binaries: true, + bin_path: "#{node['package']['install-dir']}/embedded/bin/git" + } +} diff --git a/files/gitlab-cookbooks/gitaly/libraries/gitaly.rb b/files/gitlab-cookbooks/gitaly/libraries/gitaly.rb --- a/files/gitlab-cookbooks/gitaly/libraries/gitaly.rb +++ b/files/gitlab-cookbooks/gitaly/libraries/gitaly.rb @@ -16,6 +16,7 @@ # require 'chef/mash' +require 'tomlib' require_relative '../../package/libraries/helpers/output_helper.rb' module Gitaly @@ -27,9 +28,16 @@ parse_gitaly_storages parse_gitconfig parse_legacy_cgroup_variables + + remap_legacy_values + check_array_entries end def parse_legacy_cgroup_variables + # Don't map these if the new configuraton is set. Doing so would set the old keys + # and raise an error as both keys are set. + return unless Gitlab['gitaly'].dig('configuration', 'cgroups').nil? + # maintain backwards compatibility with pre 15.0 Gitaly cgroups config cgroups_repositories_memory_bytes = Gitlab['gitaly']['cgroups_repositories_memory_bytes'] || (Gitlab['gitaly']['cgroups_memory_limit'] if Gitlab['gitaly']['cgroups_memory_enabled']) cgroups_repositories_cpu_shares = Gitlab['gitaly']['cgroups_repositories_cpu_shares'] || (Gitlab['gitaly']['cgroups_cpu_shares'] if Gitlab['gitaly']['cgroups_cpu_enabled']) @@ -43,9 +51,9 @@ end def gitaly_address - socket_path = user_config['socket_path'] || package_default['socket_path'] - listen_addr = user_config['listen_addr'] || package_default['listen_addr'] - tls_listen_addr = user_config['tls_listen_addr'] || package_default['tls_listen_addr'] + socket_path = user_config.dig('configuration', 'socket_path') || user_config['socket_path'] || package_default.dig('configuration', 'socket_path') + listen_addr = user_config.dig('configuration', 'listen_addr') || user_config['listen_addr'] || package_default.dig('configuration', 'listen_addr') + tls_listen_addr = user_config.dig('configuration', 'tls_listen_addr') || user_config['tls_listen_addr'] || package_default.dig('configuration', 'tls_listen_addr') # Default to using socket path if available if tls_listen_addr && !tls_listen_addr.empty? @@ -76,7 +84,7 @@ end def parse_gitaly_storages - return unless Gitlab['gitaly']['storage'].nil? + return unless Gitlab['gitaly']['storage'].nil? && Gitlab['gitaly'].dig('configuration', 'storage').nil? storages = [] Gitlab['gitlab_rails']['repositories_storages'].each do |key, value| @@ -168,5 +176,170 @@ def package_default Gitlab['node']['gitaly'].to_hash end + + # remap_legacy_values moves configuration values from their legacy locations to where they are + # in Gitaly's own configuration. All of the configuration was previously grouped under Gitlab['gitaly'] + # but now Gitaly's own config is under Gitlab['gitaly']['configuration']. This then allows us to + # simply encode the map as TOML to get the resulting Gitaly configuration file without having to manually + # template every key. As existing configuration files may can still have the configuration in its old place, + # this method provides backwards compatibility by moving the old values to their new locations. This can + # compatibility wrapper can be removed in 16.0 + def remap_legacy_values + Gitlab['gitaly']['configuration'] = {} unless Gitlab['gitaly']['configuration'] + + remap_recursive( + { + socket_path: 'socket_path', + runtime_dir: 'runtime_dir', + listen_addr: 'listen_addr', + prometheus_listen_addr: 'prometheus_listen_addr', + tls_listen_addr: 'tls_listen_addr', + tls: { + certificate_path: 'certificate_path', + key_path: 'key_path' + }, + graceful_restart_timeout: 'graceful_restart_timeout', + logging: { + level: 'logging_level', + format: 'logging_format', + sentry_dsn: 'logging_sentry_dsn', + ruby_sentry_dsn: 'logging_ruby_sentry_dsn', + sentry_environment: 'logging_sentry_environment', + dir: 'log_directory' + }, + prometheus: { + grpc_latency_buckets: lambda { + return [] unless Gitlab['gitaly'].key?('prometheus_grpc_latency_buckets') + raise "Legacy configuration key 'prometheus_grpc_latency_buckets' can't be set when its new key 'configuration.prometheus.grpc_latency_buckets' is set." if (Gitlab['gitaly'].dig('configuration', 'prometheus') || {}).key?('grpc_latency_buckets') + + # The legacy value is not actually an array but a string like '[0, 1, 2]'. + # The template evaluated Ruby code, so the array string got evaluated to + # an array. Parse the array into a Ruby array here. + JSON.parse(Gitlab['gitaly']['prometheus_grpc_latency_buckets']) + }, + }, + auth: { + token: 'auth_token', + transitioning: 'auth_transitioning' + }, + git: { + catfile_cache_size: 'git_catfile_cache_size', + bin_path: 'git_bin_path', + use_bundled_binaries: 'use_bundled_git', + signing_key: 'gpg_signing_key_path', + config: lambda { + return [] unless Gitlab['gitaly']['gitconfig'] + raise "Legacy configuration keys 'gitconfig' and 'omnibus_gitconfig' can't be set when its new key 'configuration.git.config' is set." if (Gitlab['gitaly'].dig('configuration', 'git') || {}).key?('config') + + Gitlab['gitaly']['gitconfig'].map do |entry| + { + key: [entry['section'], entry['subsection'], entry['key']].compact.join('.'), + value: entry['value'] + } + end + } + }, + 'gitaly-ruby': { + max_rss: 'ruby_max_rss', + graceful_restart_timeout: 'ruby_graceful_restart_timeout', + restart_delay: 'ruby_restart_delay', + num_workers: 'ruby_num_workers' + }, + storage: 'storage', + hooks: { + custom_hooks_dir: 'custom_hooks_dir' + }, + daily_maintenance: { + disabled: 'daily_maintenance_disabled', + start_hour: 'daily_maintenance_start_hour', + start_minute: 'daily_maintenance_start_minute', + duration: 'daily_maintenance_duration', + storages: 'daily_maintenance_storages' + }, + cgroups: { + mountpoint: 'cgroups_mountpoint', + hierarchy_root: 'cgroups_hierarchy_root', + memory_bytes: 'cgroups_memory_bytes', + cpu_shares: 'cgroups_cpu_shares', + repositories: { + count: 'cgroups_repositories_count', + memory_bytes: 'cgroups_repositories_memory_bytes', + cpu_shares: 'cgroups_repositories_cpu_shares' + } + }, + concurrency: 'concurrency', + rate_limiting: 'rate_limiting', + pack_objects_cache: { + enabled: 'pack_objects_cache_enabled', + dir: 'pack_objects_cache_dir', + max_age: 'pack_objects_cache_max_age' + } + }, + Gitlab['gitaly']['configuration'], + ['configuration'] + ) + end + + # remap_recursive goes over the mappings to remap the configuration from the old format into the new + # format: + # - Hash values indicate a subsection in the destination configuration. Hashes are recursed into to + # build the expected configuration structure. + # - Proc values are mapping functions that return the new value when executed. + # - String values indicate an old configuration key that should be copied into the new configuration + # under the new key. + # + # new_parent contains the parent key path on each level of recursion. + def remap_recursive(mappings, new_configuration, new_parent) + mappings.each do |new_key, mapping| + # If this is a hash, recurse the tree to create the correct structure. + if mapping.is_a?(Hash) + new_value = remap_recursive( + mappings[new_key], + # If there is already a section in the new configuration under the key, use that. If not, + # initialize and empty hash for the section. + Gitlab['gitaly'].dig(*[new_parent, new_key].flatten) || {}, + [new_parent, new_key].flatten + ) + + new_configuration[new_key] = new_value unless new_value.empty? + next + end + + # If this is a Proc, it's a mapping function that returns the value for the + # new key. + if mapping.is_a?(Proc) + new_value = mapping.call + new_configuration[new_key] = new_value unless new_value.empty? + next + end + + # If the mapping is not a hash nor a lambda, then it is a String. The value gets copied from the + # mapping to the new_key as is. + # + # If there is no old key, then there's nothing to map. Proceed to the next key. + next unless Gitlab['gitaly'].key?(mapping) + + # Raise an error if both the old key and the new key are present in the configuration as it would be + # ambigious which key is ued in the final configuration. + raise "Legacy configuration key '#{mapping}' can't be set when its new key '#{[new_parent, new_key].flatten.join('.')}' is set." if new_configuration.key?(new_key) + + new_configuration[new_key] = Gitlab['gitaly'][mapping] + end + + new_configuration + end + + # check_array_entries checks that array values in the new configuration are actually arrays. + # These values were historically configured as strings. This check guards against copy paster mistakes + # users may do while migrating to the new configuration. This check can be removed along the + # backwards compatibility code in 16.0. + def check_array_entries + [ + [:prometheus, :grpc_latency_buckets] + ].each do |key_path| + value = Gitlab['gitaly']['configuration'].dig(*key_path) + raise "gitaly['configuration']#{key_path.map { |e| "[:#{e}]" }.join('')} must be an array, not a string" unless value.nil? || value.is_a?(Array) + end + end end end diff --git a/files/gitlab-cookbooks/gitaly/recipes/enable.rb b/files/gitlab-cookbooks/gitaly/recipes/enable.rb --- a/files/gitlab-cookbooks/gitaly/recipes/enable.rb +++ b/files/gitlab-cookbooks/gitaly/recipes/enable.rb @@ -18,17 +18,17 @@ omnibus_helper = OmnibusHelper.new(node) working_dir = node['gitaly']['dir'] -log_directory = node['gitaly']['log_directory'] +log_directory = node.dig('gitaly', 'configuration', 'logging', 'dir') env_directory = node['gitaly']['env_directory'] config_path = File.join(working_dir, "config.toml") gitaly_path = node['gitaly']['bin_path'] wrapper_path = "#{gitaly_path}-wrapper" pid_file = File.join(working_dir, "gitaly.pid") -json_logging = node['gitaly']['logging_format'].eql?('json') +json_logging = node.dig('gitaly', 'configuration', 'logging', 'format').eql?('json') open_files_ulimit = node['gitaly']['open_files_ulimit'] -runtime_dir = node['gitaly']['runtime_dir'] -cgroups_mountpoint = node['gitaly']['cgroups_mountpoint'] -cgroups_hierarchy_root = node['gitaly']['cgroups_hierarchy_root'] +runtime_dir = node.dig('gitaly', 'configuration', 'runtime_dir') +cgroups_mountpoint = node.dig('gitaly', 'configuration', 'cgroups', 'mountpoint') +cgroups_hierarchy_root = node.dig('gitaly', 'configuration', 'cgroups', 'hierarchy_root') directory working_dir do owner account_helper.gitlab_user @@ -81,7 +81,6 @@ end gitlab_url, gitlab_relative_path = WebServerHelper.internal_api_url(node) -custom_hooks_dir = node.dig('gitaly', 'custom_hooks_dir') template "Create Gitaly config.toml" do path config_path @@ -90,10 +89,41 @@ group account_helper.gitlab_group mode "0640" variables node['gitaly'].to_hash.merge( - { gitlab_shell: node['gitlab']['gitlab-shell'].to_hash, - gitlab_url: gitlab_url, - gitlab_relative_path: gitlab_relative_path, - custom_hooks_dir: custom_hooks_dir } + { + configuration: node.dig('gitaly', 'configuration').merge( + { + # The gitlab section is not configured by the user directly. Its values are derived + # from other configuration. + gitlab: { + url: gitlab_url, + relative_url_root: gitlab_relative_path, + 'http-settings': node.dig('gitlab', 'gitlab-shell', 'http_settings') + }.compact, + + # These options below were historically hard coded values in the template. They + # are set here to retain the behavior of them not being overridable by the user. + bin_dir: '/opt/gitlab/embedded/bin', + git: (node.dig('gitaly', 'configuration', 'git') || {}).merge( + { + # Ignore gitconfig files so that the only source of truth for how Git commands + # are configured are Gitaly's own defaults and the Git configuration injected + # in this file. + ignore_gitconfig: true + } + ), + 'gitlab-shell': (node.dig('gitaly', 'configuration', 'gitlab-shell') || {}).merge( + { + dir: '/opt/gitlab/embedded/service/gitlab-shell' + } + ), + 'gitaly-ruby': (node.dig('gitaly', 'configuration', 'gitaly-ruby') || {}).merge( + { + dir: '/opt/gitlab/embedded/service/gitaly-ruby' + } + ) + } + ) + } ) notifies :hup, "runit_service[gitaly]" if omnibus_helper.should_notify?('gitaly') sensitive true @@ -141,6 +171,6 @@ id 'gitaly' meta node['gitaly']['consul_service_meta'] action Prometheus.service_discovery_action - socket_address node['gitaly']['prometheus_listen_addr'] + socket_address node.dig('gitaly', 'configuration', 'prometheus_listen_addr') reload_service false unless Services.enabled?('consul') end diff --git a/files/gitlab-cookbooks/gitaly/templates/default/gitaly-config.toml.erb b/files/gitlab-cookbooks/gitaly/templates/default/gitaly-config.toml.erb --- a/files/gitlab-cookbooks/gitaly/templates/default/gitaly-config.toml.erb +++ b/files/gitlab-cookbooks/gitaly/templates/default/gitaly-config.toml.erb @@ -4,211 +4,4 @@ # and run: # sudo gitlab-ctl reconfigure -socket_path = '<%= @socket_path %>' - -runtime_dir = '<%= @runtime_dir %>' -bin_dir = '/opt/gitlab/embedded/bin' -<% if @graceful_restart_timeout %> -graceful_restart_timeout = '<%= @graceful_restart_timeout %>' -<% end %> - -<% if @listen_addr %> -# Optional: listen on a TCP socket. This is insecure (no authentication) -listen_addr = '<%= @listen_addr %>' -<% end %> - -<% if @prometheus_listen_addr %> -# Optional: export metrics via Prometheus -prometheus_listen_addr = '<%= @prometheus_listen_addr %>' -<% end %> - -<% if @tls_listen_addr %> -# Optional: listen on a secure TCP socket -tls_listen_addr = '<%= @tls_listen_addr %>' -[tls] -certificate_path = '<%= @certificate_path %>' -key_path = '<%= @key_path %>' -<% end %> - -<% @storage.each do |shard| %> -[[storage]] -<% if shard.has_key?('name') %> -name = '<%= shard['name'] %>' -<% end %> -<% if shard.has_key?('path') %> -path = '<%= shard['path'] %>' -<% end %> -<% end %> - -[logging] -<% if @logging_level %> -level = '<%= @logging_level %>' -<% end %> -<% if @logging_format %> -format = '<%= @logging_format %>' -<% end %> -<% if @logging_sentry_dsn %> -sentry_dsn = '<%= @logging_sentry_dsn %>' -<% end %> -<% if @logging_ruby_sentry_dsn %> -ruby_sentry_dsn = '<%= @logging_ruby_sentry_dsn %>' -<% end %> -<% if @logging_sentry_environment %> -sentry_environment = '<%= @logging_sentry_environment %>' -<% end %> -dir = '<%= @log_directory %>' - -<% if @prometheus_grpc_latency_buckets %> -# # You can optionally configure Gitaly to record histogram latencies on GRPC method calls -[prometheus] -grpc_latency_buckets = <%= @prometheus_grpc_latency_buckets %> -<% end %> - -[auth] -<% if @auth_token %> -token = '<%= @auth_token %>' -<% end %> -<% if @auth_transitioning %> -transitioning = <%= @auth_transitioning %> -<% end %> - -[git] -<% if @git_catfile_cache_size %> -catfile_cache_size = <%= @git_catfile_cache_size %> -<% end %> -<% if @git_bin_path %> -bin_path = '<%= @git_bin_path %>' -<% end %> -<% if @use_bundled_git %> -use_bundled_binaries = <%= @use_bundled_git %> -<% end %> -# Ignore gitconfig files so that the only source of truth for how Git commands -# are configured are Gitaly's own defaults and the Git configuration injected -# in this file. -ignore_gitconfig = true -<% if @gpg_signing_key_path %> -signing_key = '<%= @gpg_signing_key_path %>' -<% end %> - -<% if @gitconfig %> - <% @gitconfig.each do |gitconfig| %> -[[git.config]] -key = "<%= [ gitconfig['section'], gitconfig['subsection'], gitconfig['key'] ].compact.join('.') %>" -value = "<%= gitconfig['value'] %>" - - <% end %> -<% end %> -[gitaly-ruby] -dir = "/opt/gitlab/embedded/service/gitaly-ruby" -<% if @ruby_max_rss %> -max_rss = <%= @ruby_max_rss %> -<% end %> -<% if @ruby_graceful_restart_timeout %> -graceful_restart_timeout = '<%= @ruby_graceful_restart_timeout %>' -<% end %> -<% if @ruby_restart_delay %> -restart_delay = '<%= @ruby_restart_delay %>' -<% end %> -<% if @ruby_num_workers %> -num_workers = <%= @ruby_num_workers %> -<% end %> - -[gitlab-shell] -dir = "/opt/gitlab/embedded/service/gitlab-shell" - -[gitlab] -<% if @gitlab_url %> -url = '<%= @gitlab_url %>' -<% end %> -<% if @gitlab_relative_path %> -relative_url_root = '<%= @gitlab_relative_path %>' -<% end %> - -<% if @gitlab_shell['http_settings'] %> -<% http_settings = @gitlab_shell['http_settings'] %> -[gitlab.http-settings] -<% if http_settings['read_timeout'] %> -read_timeout = <%= http_settings['read_timeout'] %> -<% end %> -<% if http_settings['user'] %> -user = '<%= http_settings['user'] %>' -<% end %> -<% if http_settings['password'] %> -password = '<%= http_settings['password'] %>' -<% end %> -<% if http_settings['ca_file'] %> -ca_file = '<%= http_settings['ca_file'] %>' -<% end %> -<% if http_settings['ca_path'] %> -ca_path = '<%= http_settings['ca_path'] %>' -<% end %> -<% end %> - -[hooks] -<% if @custom_hooks_dir %> -custom_hooks_dir = '<%= @custom_hooks_dir %>' -<% end %> - -<% if @concurrency %> - <% @concurrency.each do |endpoint| %> - -[[concurrency]] -rpc = "<%= endpoint['rpc'] %>" - <% if endpoint['max_per_repo'] %> -max_per_repo = <%= endpoint['max_per_repo'] %> - <% end %> - <% if endpoint['max_queue_size'] %> -max_queue_size = <%= endpoint['max_queue_size'] %> - <% end %> - <% if endpoint['max_queue_wait'] %> -max_queue_wait = "<%= endpoint['max_queue_wait'] %>" - <% end %> - <% end %> -<% end %> - -<% if @rate_limiting %> - <% @rate_limiting.each do |endpoint| %> -[[rate_limiting]] -rpc = "<%= endpoint['rpc'] %>" -interval = "<%= endpoint['interval'] %>" -burst = <%= endpoint['burst'] %> - <% end %> -<% end %> - -[daily_maintenance] -<% if @daily_maintenance_disabled %> -disabled = <%= @daily_maintenance_disabled %> -<% else %> -<%= "start_hour = #{@daily_maintenance_start_hour}" if @daily_maintenance_start_hour %> -<%= "start_minute = #{@daily_maintenance_start_minute}" if @daily_maintenance_start_minute %> -<%= "duration = '#{@daily_maintenance_duration}'" if @daily_maintenance_duration %> -<%= "storages = #{@daily_maintenance_storages}" if @daily_maintenance_storages %> -<% end %> - -<% if @cgroups_repositories_count %> -[cgroups] -mountpoint = '<%= @cgroups_mountpoint %>' -hierarchy_root = '<%= @cgroups_hierarchy_root %>' - <% if @cgroups_memory_bytes %> -memory_bytes = <%= @cgroups_memory_bytes %> - <% end %> - <% if @cgroups_cpu_shares %> -cpu_shares = <%= @cgroups_cpu_shares %> - <% end %> - -[cgroups.repositories] -count = <%= @cgroups_repositories_count %> - <% if @cgroups_repositories_memory_bytes %> -memory_bytes = <%= @cgroups_repositories_memory_bytes %> - <% end %> - <% if @cgroups_repositories_cpu_shares %> -cpu_shares = <%= @cgroups_repositories_cpu_shares %> - <% end %> -<% end %> - -<% if @pack_objects_cache_enabled %> -[pack_objects_cache] -enabled = true -<%= "dir = '#{@pack_objects_cache_dir}'" if @pack_objects_cache_dir %> -<%= "max_age = '#{@pack_objects_cache_max_age}'" if @pack_objects_cache_max_age %> -<% end %> +<%= Tomlib.dump(@configuration) %> diff --git a/files/gitlab-cookbooks/monitoring/libraries/prometheus.rb b/files/gitlab-cookbooks/monitoring/libraries/prometheus.rb --- a/files/gitlab-cookbooks/monitoring/libraries/prometheus.rb +++ b/files/gitlab-cookbooks/monitoring/libraries/prometheus.rb @@ -241,8 +241,8 @@ # Don't parse if gitaly is explicitly disabled return unless Services.enabled?('gitaly') || service_discovery - default_config = Gitlab['node']['gitaly'].to_hash - user_config = Gitlab['gitaly'] + default_config = Gitlab['node']['gitaly']['configuration'].to_hash + user_config = Gitlab['gitaly']['configuration'] # Don't enable a scrape config if the listen address is empty. return if user_config['prometheus_listen_addr'] && user_config['prometheus_listen_addr'].empty? diff --git a/spec/chef/cookbooks/gitaly/recipes/gitaly_spec.rb b/spec/chef/cookbooks/gitaly/recipes/gitaly_spec.rb --- a/spec/chef/cookbooks/gitaly/recipes/gitaly_spec.rb +++ b/spec/chef/cookbooks/gitaly/recipes/gitaly_spec.rb @@ -100,14 +100,13 @@ it 'populates gitaly config.toml with defaults' do expect(get_rendered_toml(chef_run, '/var/opt/gitlab/gitaly/config.toml')).to eq( { - auth: {}, bin_dir: '/opt/gitlab/embedded/bin', - daily_maintenance: {}, git: { bin_path: '/opt/gitlab/embedded/bin/git', ignore_gitconfig: true, use_bundled_binaries: true }, + cgroups: { repositories: {} }, 'gitaly-ruby': { dir: '/opt/gitlab/embedded/service/gitaly-ruby' }, @@ -118,7 +117,6 @@ 'gitlab-shell': { dir: '/opt/gitlab/embedded/service/gitlab-shell' }, - hooks: {}, logging: { dir: '/var/log/gitlab/gitaly', format: 'json' @@ -173,8 +171,8 @@ it 'populates gitaly cgroups with correct values' do cgroups_section = Regexp.new([ %r{\[cgroups\]}, - %r{mountpoint = '#{cgroups_mountpoint}'}, - %r{hierarchy_root = '#{cgroups_hierarchy_root}'}, + %r{mountpoint = "#{cgroups_mountpoint}"}, + %r{hierarchy_root = "#{cgroups_hierarchy_root}"}, %r{\[cgroups.repositories\]}, %r{count = 100}, %r{memory_bytes = #{cgroups_memory_bytes}}, @@ -187,6 +185,44 @@ end end + context 'with new gitconfig' do + it 'raises an error with legacy gitconfig set' do + stub_gitlab_rb( + { + gitaly: { + gitconfig: [{ key: 'legacy_key', value: 'legacy_value' }], + configuration: { + git: { + config: [{ key: 'new_key', value: 'new_value' }] + } + } + } + } + ) + + expect { chef_run }.to raise_error "Legacy configuration keys 'gitconfig' and 'omnibus_gitconfig' can't be set when its new key 'configuration.git.config' is set." + end + + it 'raises an error if omnibus_gitconfig is set' do + stub_gitlab_rb( + { + omnibus_gitconfig: { + system: { nondefault: ['bar = baz'] } + }, + gitaly: { + configuration: { + git: { + config: [{ key: 'new_key', value: 'new_value' }] + } + } + } + } + ) + + expect { chef_run }.to raise_error "Legacy configuration keys 'gitconfig' and 'omnibus_gitconfig' can't be set when its new key 'configuration.git.config' is set." + end + end + context 'with Omnibus gitconfig' do let(:omnibus_gitconfig) { nil } let(:gitaly_gitconfig) { nil } @@ -425,6 +461,134 @@ end end + context 'with array configured as string' do + it 'raises an error with prometheus grpc latency buckets' do + stub_gitlab_rb( + { + gitaly: { + enable: true, + configuration: { + prometheus: { + grpc_latency_buckets: '[0, 1, 2]' + } + } + } + } + ) + + expect { chef_run }.to raise_error("gitaly['configuration'][:prometheus][:grpc_latency_buckets] must be an array, not a string") + end + end + + context 'with some defaults overridden with custom configuration' do + before do + stub_gitlab_rb( + gitaly: { + enable: true, + configuration: { + socket_path: 'overridden_socket_path', + logging: { + dir: 'overridden_logging_path' + }, + git: { + bin_path: 'overridden_git_bin_path' + }, + custom_section: { + custom_key: 'custom_value' + }, + storage: [ + { + name: 'custom_storage', + path: 'custom_path' + }, + ], + cgroups: { + cpu_shares: 100, + }, + } + } + ) + end + + it 'renders config.toml with' do + expect(get_rendered_toml(chef_run, '/var/opt/gitlab/gitaly/config.toml')).to eq( + { + 'gitaly-ruby': { + dir: '/opt/gitlab/embedded/service/gitaly-ruby' + }, + 'gitlab-shell': { + dir: '/opt/gitlab/embedded/service/gitlab-shell' + }, + bin_dir: '/opt/gitlab/embedded/bin', + custom_section: { custom_key: 'custom_value' }, + git: { + bin_path: 'overridden_git_bin_path', + ignore_gitconfig: true, + use_bundled_binaries: true, + }, + gitlab: { + url: 'http+unix://%2Fvar%2Fopt%2Fgitlab%2Fgitlab-workhorse%2Fsockets%2Fsocket', + relative_url_root: '', + }, + logging: { + dir: 'overridden_logging_path', + format: 'json', + }, + prometheus_listen_addr: 'localhost:9236', + runtime_dir: '/var/opt/gitlab/gitaly/run', + socket_path: 'overridden_socket_path', + storage: [ + { + name: 'custom_storage', + path: 'custom_path', + } + ], + cgroups: { + cpu_shares: 100, + } + } + ) + end + end + + context 'with old key and its new key set' do + it 'raises an error in generic case' do + stub_gitlab_rb( + { + gitaly: { + enable: true, + certificate_path: 'legacy_certificate_path_key', + configuration: { + tls: { + certificate_path: 'new_certificate_path_key', + } + } + } + } + ) + + expect { chef_run }.to raise_error "Legacy configuration key 'certificate_path' can't be set when its new key 'configuration.tls.certificate_path' is set." + end + + it 'raises an error in generic case' do + stub_gitlab_rb( + { + gitaly: { + enable: true, + prometheus_grpc_latency_buckets: '[0, 1, 2]', + configuration: { + prometheus: { + grpc_latency_buckets: '[0, 1, 3]', + } + } + } + } + ) + + expect { chef_run }.to raise_error "Legacy configuration key 'prometheus_grpc_latency_buckets' can't be set when its new key 'configuration.prometheus.grpc_latency_buckets' is set." + end + end + context 'with user settings' do before do stub_gitlab_rb( @@ -477,7 +641,7 @@ ruby_num_workers: ruby_num_workers, git_catfile_cache_size: git_catfile_cache_size, git_bin_path: git_bin_path, - use_bundled_git: true, + use_bundled_git: false, open_files_ulimit: open_files_ulimit, daily_maintenance_start_hour: daily_maintenance_start_hour, daily_maintenance_start_minute: daily_maintenance_start_minute, @@ -494,7 +658,14 @@ pack_objects_cache_enabled: pack_objects_cache_enabled, pack_objects_cache_dir: pack_objects_cache_dir, pack_objects_cache_max_age: pack_objects_cache_max_age, - custom_hooks_dir: gitaly_custom_hooks_dir + custom_hooks_dir: gitaly_custom_hooks_dir, + # Sanity check that configuration values get printed out. + configuration: { + string_value: 'some value', + subsection: { + array_value: [1, 2, 3] + } + } }, gitlab_rails: { internal_api_url: gitlab_url @@ -555,6 +726,7 @@ } ], daily_maintenance: { + disabled: false, duration: '45m', start_hour: 21, start_minute: 9, @@ -565,7 +737,7 @@ catfile_cache_size: 50, ignore_gitconfig: true, signing_key: '/path/to/signing_key.gpg', - use_bundled_binaries: true + use_bundled_binaries: false }, 'gitaly-ruby': { dir: '/opt/gitlab/embedded/service/gitaly-ruby', @@ -633,6 +805,8 @@ path: '/mnt/nfs1' } ], + string_value: 'some value', + subsection: { array_value: [1, 2, 3] }, tls: { certificate_path: '/path/to/cert.pem', key_path: '/path/to/key.pem' @@ -666,7 +840,7 @@ it 'renders daily_maintenance with disabled set to true' do expect(chef_run).to render_file(config_path).with_content { |content| - expect(content).to include("[daily_maintenance]\ndisabled = true\n\n") + expect(content).to include("[daily_maintenance]\ndisabled = true\n") } end end @@ -688,9 +862,9 @@ it 'populates gitaly config.toml with custom storages' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[\[storage\]\]\s+name = 'default'\s+path = '/tmp/default/git-data/repositories'}) + .with_content(%r{\[\[storage\]\]\s+name = "default"\s+path = "/tmp/default/git-data/repositories"}) expect(chef_run).to render_file(config_path) - .with_content(%r{\[\[storage\]\]\s+name = 'nfs1'\s+path = '/mnt/nfs1/repositories'}) + .with_content(%r{\[\[storage\]\]\s+name = "nfs1"\s+path = "/mnt/nfs1/repositories"}) expect(chef_run).not_to render_file(config_path) .with_content('gitaly_address: "/var/opt/gitlab/gitaly/gitaly.socket"') end @@ -711,7 +885,7 @@ it 'populates gitaly config.toml with custom storages' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[\[storage\]\]\s+name = 'default'\s+path = '/var/opt/gitlab/git-data/repositories'}) + .with_content(%r{\[\[storage\]\]\s+name = "default"\s+path = "/var/opt/gitlab/git-data/repositories"}) expect(chef_run).not_to render_file(config_path) .with_content('gitaly_address: "tcp://gitaly.internal:8075"') end @@ -903,7 +1077,7 @@ it 'create config file with provided values' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[gitlab\]\s+url = 'http\+unix://%2Ffake%2Fworkhorse%2Fsocket'\s+relative_url_root = ''}) + .with_content(%r{\[gitlab\]\s+url = "http\+unix://%2Ffake%2Fworkhorse%2Fsocket"\s+relative_url_root = ""}) end end @@ -914,7 +1088,7 @@ it 'create config file with provided values' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[gitlab\]\s+url = 'http\+unix://%2Ffake%2Fworkhorse%2Fsockets%2Fsocket'\s+relative_url_root = ''}) + .with_content(%r{\[gitlab\]\s+url = "http\+unix://%2Ffake%2Fworkhorse%2Fsockets%2Fsocket"\s+relative_url_root = ""}) end end @@ -925,7 +1099,7 @@ it 'create config file with provided values' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[gitlab\]\s+url = 'http\+unix://%2Fsockets%2Fin%2Fthe%2Fwind'\s+relative_url_root = ''}) + .with_content(%r{\[gitlab\]\s+url = "http\+unix://%2Fsockets%2Fin%2Fthe%2Fwind"\s+relative_url_root = ""}) end end end @@ -943,7 +1117,7 @@ it 'create config file with only the URL set' do expect(chef_run).to render_file(config_path).with_content { |content| - expect(content).to match(%r{\[gitlab\]\s+url = 'http://localhost:1234/gitlab'}) + expect(content).to match(%r{\[gitlab\]\s+url = "http://localhost:1234/gitlab"}) expect(content).not_to match(/relative_url_root/) } end @@ -956,7 +1130,7 @@ it 'create config file with the relative_url_root set' do expect(chef_run).to render_file(config_path) - .with_content(%r{\[gitlab\]\s+url = 'http\+unix://%2Fvar%2Fopt%2Fgitlab%2Fgitlab-workhorse%2Fsockets%2Fsocket'\s+relative_url_root = '/gitlab'}) + .with_content(%r{\[gitlab\]\s+url = "http\+unix://%2Fvar%2Fopt%2Fgitlab%2Fgitlab-workhorse%2Fsockets%2Fsocket"\s+relative_url_root = "/gitlab"}) end end