Commit e7d9434e authored by Stan Hu's avatar Stan Hu Committed by Alex Kalderimis

Fix consolidated storage settings

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52750 broke
consolidated storage settings because it was scanning storage-specific
settings. For example, let's say you had:

```yaml
object_store:
    enabled: true
    objects:
        artifacts:
            enabled: false

artifacts:
    enabled: true
```

In `1_settings.rb`, `ObjectStoreSettings#legacy_parse!` will
automatically add default `object_store` settings to the
storage-specific settings:

```yaml
artifacts:
    enabled: true
    object_store:
        enabled: false
        ...
```

As seen above, `artifacts.object_store.enabled` defaults to `false`, so
the check introduced in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52750 would mean
that object storage would never be enabled unless storage-specific
settings were also enabled.

The right fix: skip parsing of consolidated settings only if the object
type has explicitly disabled it in the consolidated settings. In the
example above, this means we check the parameter in
`object_store.objects.artifacts.enabled`, which overrides the default
consolidated settings.

This should fix https://gitlab.com/gitlab-org/gitlab/-/issues/233826.
parent 63ad0841
...@@ -124,9 +124,12 @@ class ObjectStoreSettings ...@@ -124,9 +124,12 @@ class ObjectStoreSettings
target_config = common_config.merge(overrides.slice(*ALLOWED_OBJECT_STORE_OVERRIDES)) target_config = common_config.merge(overrides.slice(*ALLOWED_OBJECT_STORE_OVERRIDES))
section = settings.try(store_type) section = settings.try(store_type)
next unless uses_object_storage?(section) # Admins can selectively disable object storage for a specific
# type as an override in the consolidated settings.
next unless overrides.fetch('enabled', true)
next unless section
if target_config['bucket'].blank? if section['enabled'] && target_config['bucket'].blank?
missing_bucket_for(store_type) missing_bucket_for(store_type)
next next
end end
...@@ -146,20 +149,6 @@ class ObjectStoreSettings ...@@ -146,20 +149,6 @@ class ObjectStoreSettings
private private
# Admins can selectively disable object storage for a specific type. If
# this hasn't been set, we assume that the consolidated settings
# should be used.
def uses_object_storage?(section)
# Use to_h to avoid https://gitlab.com/gitlab-org/gitlab/-/issues/286873
section = section.to_h
enabled_globally = section.fetch('enabled', false)
object_store_settings = section.fetch('object_store', {})
os_enabled = object_store_settings.fetch('enabled', true)
enabled_globally && os_enabled
end
# We only can use the common object storage settings if: # We only can use the common object storage settings if:
# 1. The common settings are defined # 1. The common settings are defined
# 2. The legacy settings are not defined # 2. The legacy settings are not defined
......
...@@ -130,13 +130,14 @@ RSpec.describe ObjectStoreSettings do ...@@ -130,13 +130,14 @@ RSpec.describe ObjectStoreSettings do
end end
end end
context 'when object storage is selectively disabled for artifacts' do context 'when object storage is disabled for artifacts with no bucket' do
before do before do
config['artifacts'] = { config['artifacts'] = {
'enabled' => true, 'enabled' => true,
'object_store' => { 'object_store' => {}
'enabled' => false }
} config['object_store']['objects']['artifacts'] = {
'enabled' => false
} }
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment