From 93f4de88cfbf100c3cf938b22903a6cb94e21a02 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Tue, 14 Jun 2022 15:56:39 +0300 Subject: [PATCH 1/3] Add pull-policy support for service images on CI config In the runner config.toml file, you can already define pull_policy for runners. This config determines how to fetch the needed images. In this change, we are adding this config to CI config YAML. The config can be a string or array of strings. The config will be filtered by the allowed_pull_policies config in the Runner side. We already did this for job images. This is behind a feature flag ci_docker_image_pull_policy --- app/assets/javascripts/editor/schema/ci.json | 27 +++++++++ doc/ci/yaml/index.md | 46 +++++++++++++++ lib/api/entities/ci/job_request/service.rb | 1 + lib/gitlab/ci/config/entry/image.rb | 2 +- lib/gitlab/ci/config/entry/service.rb | 34 ++++++++--- .../entities/ci/job_request/service_spec.rb | 51 ++++++++++++++++ spec/lib/gitlab/ci/build/image_spec.rb | 5 +- spec/lib/gitlab/ci/config/entry/image_spec.rb | 11 +--- .../gitlab/ci/config/entry/service_spec.rb | 58 +++++++++++++++++- spec/lib/gitlab/ci/yaml_processor_spec.rb | 47 ++++++++++++++- .../api/ci/runner/jobs_request_post_spec.rb | 59 ++++++++++++++++--- 11 files changed, 314 insertions(+), 27 deletions(-) create mode 100644 spec/lib/api/entities/ci/job_request/service_spec.rb diff --git a/app/assets/javascripts/editor/schema/ci.json b/app/assets/javascripts/editor/schema/ci.json index c8015f884b7d2d..e8b96c25965244 100644 --- a/app/assets/javascripts/editor/schema/ci.json +++ b/app/assets/javascripts/editor/schema/ci.json @@ -436,6 +436,33 @@ "type": "string" } }, + "pull_policy": { + "markdownDescription": "Specifies how to pull the image in Runner. It can be one of `always`, `never` or `if-not-present`. The default value is `always`. [Learn more](https://docs.gitlab.com/ee/ci/yaml/#servicepull_policy).", + "default": "always", + "oneOf": [ + { + "type": "string", + "enum": [ + "always", + "never", + "if-not-present" + ] + }, + { + "type": "array", + "items": { + "type": "string", + "enum": [ + "always", + "never", + "if-not-present" + ] + }, + "minItems": 1, + "uniqueItems": true + } + ] + }, "command": { "type": "array", "description": "Command or script that should be used as the container's command. It will be translated to arguments passed to Docker after the image's name. The syntax is similar to Dockerfile's CMD directive, where each shell token is a separate string in the array.", diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 912eca364c9794..179fc13b0bfbc9 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3519,6 +3519,52 @@ in that container. - [Run your CI/CD jobs in Docker containers](../docker/using_docker_images.md). - [Use Docker to build Docker images](../docker/using_docker_build.md). +#### `service:pull_policy` + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/21619) in GitLab 15.1 [with a flag](../../administration/feature_flags.md) named `ci_docker_image_pull_policy`. Disabled by default. +> - Requires GitLab Runner 15.1 or later. + +FLAG: +On self-managed GitLab, by default this feature is not available. To make it available, +ask an administrator to [enable the feature flag](../../administration/feature_flags.md) named `ci_docker_image_pull_policy`. +The feature is not ready for production use. + +The pull policy that the runner uses to fetch the Docker image. + +**Keyword type**: Job keyword. You can use it only as part of a job or in the [`default` section](#default). + +**Possible inputs**: + +- A single pull policy, or multiple pull policies in an array. + Can be `always`, `if-not-present`, or `never`. + +**Examples of `service:pull_policy`**: + +```yaml +job1: + script: echo "A single pull policy." + services: + - name: postgres:11.6 + pull_policy: if-not-present + +job2: + script: echo "Multiple pull policies." + services: + - name: postgres:11.6 + pull_policy: [always, if-not-present] +``` + +**Additional details**: + +- If the runner does not support the defined pull policy, the job fails with an error similar to: + `ERROR: Job failed (system failure): the configured PullPolicies ([always]) are not allowed by AllowedPullPolicies ([never])`. + +**Related topics**: + +- [Run your CI/CD jobs in Docker containers](../docker/using_docker_images.md). +- [How runner pull policies work](https://docs.gitlab.com/runner/executors/docker.html#how-pull-policies-work). +- [Using multiple pull policies](https://docs.gitlab.com/runner/executors/docker.html#using-multiple-pull-policies). + ### `stage` Use `stage` to define which [stage](#stages) a job runs in. Jobs in the same diff --git a/lib/api/entities/ci/job_request/service.rb b/lib/api/entities/ci/job_request/service.rb index d9da2c92ec7c99..7d494c7e516491 100644 --- a/lib/api/entities/ci/job_request/service.rb +++ b/lib/api/entities/ci/job_request/service.rb @@ -8,6 +8,7 @@ class Service < Grape::Entity expose :name, :entrypoint expose :ports, using: Entities::Ci::JobRequest::Port + expose :pull_policy, if: ->(_) { ::Feature.enabled?(:ci_docker_image_pull_policy) } expose :alias, :command expose :variables end diff --git a/lib/gitlab/ci/config/entry/image.rb b/lib/gitlab/ci/config/entry/image.rb index 79443f69b03f1a..96ac959a3f4ecc 100644 --- a/lib/gitlab/ci/config/entry/image.rb +++ b/lib/gitlab/ci/config/entry/image.rb @@ -48,7 +48,7 @@ def value { name: @config[:name], entrypoint: @config[:entrypoint], - ports: ports_value, + ports: (ports_value if ports_defined?), pull_policy: (ci_docker_image_pull_policy_enabled? ? pull_policy_value : nil) }.compact else diff --git a/lib/gitlab/ci/config/entry/service.rb b/lib/gitlab/ci/config/entry/service.rb index f27dca4986e007..e88a2d7abc356e 100644 --- a/lib/gitlab/ci/config/entry/service.rb +++ b/lib/gitlab/ci/config/entry/service.rb @@ -15,11 +15,13 @@ class Service < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Attributable include ::Gitlab::Config::Entry::Configurable - ALLOWED_KEYS = %i[name entrypoint command alias ports variables].freeze + ALLOWED_KEYS = %i[name entrypoint command alias ports variables pull_policy].freeze + LEGACY_ALLOWED_KEYS = %i[name entrypoint command alias ports variables].freeze validations do validates :config, hash_or_string: true - validates :config, allowed_keys: ALLOWED_KEYS + validates :config, allowed_keys: ALLOWED_KEYS, if: :ci_docker_image_pull_policy_enabled? + validates :config, allowed_keys: LEGACY_ALLOWED_KEYS, unless: :ci_docker_image_pull_policy_enabled? validates :config, disallowed_keys: %i[ports], unless: :with_image_ports? validates :name, type: String, presence: true validates :entrypoint, array_of_strings: true, allow_nil: true @@ -32,11 +34,14 @@ class Service < ::Gitlab::Config::Entry::Node entry :ports, Entry::Ports, description: 'Ports used to expose the service' + entry :pull_policy, Entry::PullPolicy, + description: 'Pull policy for the service' + entry :variables, ::Gitlab::Ci::Config::Entry::Variables, description: 'Environment variables available for this service.', inherit: false - attributes :ports + attributes :ports, :pull_policy, :variables def alias value[:alias] @@ -55,16 +60,31 @@ def entrypoint end def value - return { name: @config } if string? - return @config if hash? - - {} + if string? + { name: @config } + elsif hash? + { + name: @config[:name], + entrypoint: @config[:entrypoint], + ports: (ports_value if ports_defined?), + pull_policy: (ci_docker_image_pull_policy_enabled? ? pull_policy_value : nil), + command: @config[:command], + alias: @config[:alias], + variables: (variables_value if variables_defined?) + }.compact + else + {} + end end def with_image_ports? opt(:with_image_ports) end + def ci_docker_image_pull_policy_enabled? + ::Feature.enabled?(:ci_docker_image_pull_policy) + end + def skip_config_hash_validation? true end diff --git a/spec/lib/api/entities/ci/job_request/service_spec.rb b/spec/lib/api/entities/ci/job_request/service_spec.rb new file mode 100644 index 00000000000000..47c2c4e04c904e --- /dev/null +++ b/spec/lib/api/entities/ci/job_request/service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::Entities::Ci::JobRequest::Service do + let(:ports) { [{ number: 80, protocol: 'http', name: 'name' }]} + let(:service) do + instance_double( + ::Gitlab::Ci::Build::Image, + name: 'image_name', + entrypoint: ['foo'], + ports: ports, + pull_policy: ['if-not-present'], + alias: 'alias', + command: 'command', + variables: [{ key: 'key', value: 'value' }] + ) + end + + let(:entity) { described_class.new(service) } + + subject(:result) { entity.as_json } + + it 'exposes attributes' do + expect(result).to eq( + name: 'image_name', + entrypoint: ['foo'], + ports: ports, + pull_policy: ['if-not-present'], + alias: 'alias', + command: 'command', + variables: [{ key: 'key', value: 'value' }] + ) + end + + context 'when the ports param is nil' do + let(:ports) { nil } + + it 'does not return the ports' do + expect(subject[:ports]).to be_nil + end + end + + context 'when the FF ci_docker_image_pull_policy is disabled' do + before do + stub_feature_flags(ci_docker_image_pull_policy: false) + end + + it { is_expected.not_to have_key(:pull_policy) } + end +end diff --git a/spec/lib/gitlab/ci/build/image_spec.rb b/spec/lib/gitlab/ci/build/image_spec.rb index 8f77a1f60ad2cf..4895077a731262 100644 --- a/spec/lib/gitlab/ci/build/image_spec.rb +++ b/spec/lib/gitlab/ci/build/image_spec.rb @@ -98,9 +98,11 @@ let(:service_entrypoint) { '/bin/sh' } let(:service_alias) { 'db' } let(:service_command) { 'sleep 30' } + let(:pull_policy) { %w[always if-not-present] } let(:job) do create(:ci_build, options: { services: [{ name: service_image_name, entrypoint: service_entrypoint, - alias: service_alias, command: service_command, ports: [80] }] }) + alias: service_alias, command: service_command, ports: [80], + pull_policy: pull_policy }] }) end it 'fabricates an non-empty array of objects' do @@ -114,6 +116,7 @@ expect(subject.first.entrypoint).to eq(service_entrypoint) expect(subject.first.alias).to eq(service_alias) expect(subject.first.command).to eq(service_command) + expect(subject.first.pull_policy).to eq(pull_policy) port = subject.first.ports.first expect(port.number).to eq 80 diff --git a/spec/lib/gitlab/ci/config/entry/image_spec.rb b/spec/lib/gitlab/ci/config/entry/image_spec.rb index bd1ab5d8c412e9..9f53b6859234e5 100644 --- a/spec/lib/gitlab/ci/config/entry/image_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/image_spec.rb @@ -9,6 +9,7 @@ before do stub_feature_flags(ci_docker_image_pull_policy: true) + entry.compose! end let(:entry) { described_class.new(config) } @@ -129,19 +130,16 @@ describe '#valid?' do it 'is valid' do - entry.compose! - expect(entry).to be_valid end context 'when the feature flag ci_docker_image_pull_policy is disabled' do before do stub_feature_flags(ci_docker_image_pull_policy: false) + entry.compose! end it 'is not valid' do - entry.compose! - expect(entry).not_to be_valid expect(entry.errors).to include('image config contains unknown keys: pull_policy') end @@ -150,8 +148,6 @@ describe '#value' do it "returns value" do - entry.compose! - expect(entry.value).to eq( name: 'image:1.0', pull_policy: ['if-not-present'] @@ -161,11 +157,10 @@ context 'when the feature flag ci_docker_image_pull_policy is disabled' do before do stub_feature_flags(ci_docker_image_pull_policy: false) + entry.compose! end it 'is not valid' do - entry.compose! - expect(entry.value).to eq( name: 'image:1.0' ) diff --git a/spec/lib/gitlab/ci/config/entry/service_spec.rb b/spec/lib/gitlab/ci/config/entry/service_spec.rb index 2795cc9dddfbd1..fb34dd828c5452 100644 --- a/spec/lib/gitlab/ci/config/entry/service_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/service_spec.rb @@ -1,14 +1,19 @@ # frozen_string_literal: true -require 'spec_helper' +require 'fast_spec_helper' +require 'support/helpers/stubbed_feature' +require 'support/helpers/stub_feature_flags' RSpec.describe Gitlab::Ci::Config::Entry::Service do - let(:entry) { described_class.new(config) } + include StubFeatureFlags before do + stub_feature_flags(ci_docker_image_pull_policy: true) entry.compose! end + let(:entry) { described_class.new(config) } + context 'when configuration is a string' do let(:config) { 'postgresql:9.5' } @@ -90,6 +95,12 @@ end end + describe '#pull_policy' do + it "returns nil" do + expect(entry.pull_policy).to be_nil + end + end + context 'when configuration has ports' do let(:ports) { [{ number: 80, protocol: 'http', name: 'foobar' }] } let(:config) do @@ -134,6 +145,49 @@ end end end + + context 'when configuration has pull_policy' do + let(:config) { { name: 'postgresql:9.5', pull_policy: 'if-not-present' } } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + + context 'when the feature flag ci_docker_image_pull_policy is disabled' do + before do + stub_feature_flags(ci_docker_image_pull_policy: false) + entry.compose! + end + + it 'is not valid' do + expect(entry).not_to be_valid + expect(entry.errors).to include('service config contains unknown keys: pull_policy') + end + end + end + + describe '#value' do + it "returns value" do + expect(entry.value).to eq( + name: 'postgresql:9.5', + pull_policy: ['if-not-present'] + ) + end + + context 'when the feature flag ci_docker_image_pull_policy is disabled' do + before do + stub_feature_flags(ci_docker_image_pull_policy: false) + end + + it 'is not valid' do + expect(entry.value).to eq( + name: 'postgresql:9.5' + ) + end + end + end + end end context 'when entry value is not correct' do diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 3dd9ca35881635..15567b3667330a 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -980,7 +980,7 @@ module Ci it { is_expected.to be_valid } - it "returns image and service when defined" do + it "returns with image" do expect(processor.stage_builds_attributes("test")).to contain_exactly({ stage: "test", stage_idx: 2, @@ -1010,6 +1010,51 @@ module Ci end end end + + context 'when a service has pull_policy' do + let(:config) do + <<~YAML + services: + - name: postgres:11.9 + pull_policy: if-not-present + + test: + script: exit 0 + YAML + end + + it { is_expected.to be_valid } + + it "returns with service" do + expect(processor.stage_builds_attributes("test")).to contain_exactly({ + stage: "test", + stage_idx: 2, + name: "test", + only: { refs: %w[branches tags] }, + options: { + script: ["exit 0"], + services: [{ name: "postgres:11.9", pull_policy: ["if-not-present"] }] + }, + allow_failure: false, + when: "on_success", + job_variables: [], + root_variables_inheritance: true, + scheduling_type: :stage + }) + end + + context 'when the feature flag ci_docker_image_pull_policy is disabled' do + before do + stub_feature_flags(ci_docker_image_pull_policy: false) + end + + it { is_expected.not_to be_valid } + + it "returns no job" do + expect(processor.jobs).to eq({}) + end + end + end end describe 'Variables' do diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb index 3c6f9ac2816d34..738c0bed6cfff6 100644 --- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb +++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb @@ -216,13 +216,17 @@ expect(json_response['token']).to eq(job.token) expect(json_response['job_info']).to eq(expected_job_info) expect(json_response['git_info']).to eq(expected_git_info) - expect(json_response['image']).to eq({ 'name' => 'image:1.0', 'entrypoint' => '/bin/sh', 'ports' => [], 'pull_policy' => nil }) - expect(json_response['services']).to eq([{ 'name' => 'postgres', 'entrypoint' => nil, - 'alias' => nil, 'command' => nil, 'ports' => [], 'variables' => nil }, - { 'name' => 'docker:stable-dind', 'entrypoint' => '/bin/sh', - 'alias' => 'docker', 'command' => 'sleep 30', 'ports' => [], 'variables' => [] }, - { 'name' => 'mysql:latest', 'entrypoint' => nil, - 'alias' => nil, 'command' => nil, 'ports' => [], 'variables' => [{ 'key' => 'MYSQL_ROOT_PASSWORD', 'value' => 'root123.' }] }]) + expect(json_response['image']).to eq( + { 'name' => 'image:1.0', 'entrypoint' => '/bin/sh', 'ports' => [], 'pull_policy' => nil } + ) + expect(json_response['services']).to eq([ + { 'name' => 'postgres', 'entrypoint' => nil, 'alias' => nil, 'command' => nil, 'ports' => [], + 'variables' => nil, 'pull_policy' => nil }, + { 'name' => 'docker:stable-dind', 'entrypoint' => '/bin/sh', 'alias' => 'docker', 'command' => 'sleep 30', + 'ports' => [], 'variables' => [], 'pull_policy' => nil }, + { 'name' => 'mysql:latest', 'entrypoint' => nil, 'alias' => nil, 'command' => nil, 'ports' => [], + 'variables' => [{ 'key' => 'MYSQL_ROOT_PASSWORD', 'value' => 'root123.' }], 'pull_policy' => nil } + ]) expect(json_response['steps']).to eq(expected_steps) expect(json_response['artifacts']).to eq(expected_artifacts) expect(json_response['cache']).to match(expected_cache) @@ -849,6 +853,47 @@ end end + context 'when service has pull_policy' do + let(:job) { create(:ci_build, :pending, :queued, pipeline: pipeline, options: options) } + + let(:options) do + { + services: [{ + name: 'postgres:11.9', + pull_policy: ['if-not-present'] + }] + } + end + + it 'returns the service with pull policy' do + request_job + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).to include( + 'id' => job.id, + 'services' => [{ 'alias' => nil, 'command' => nil, 'entrypoint' => nil, 'name' => 'postgres:11.9', + 'ports' => [], 'pull_policy' => ['if-not-present'], 'variables' => [] }] + ) + end + + context 'when the FF ci_docker_image_pull_policy is disabled' do + before do + stub_feature_flags(ci_docker_image_pull_policy: false) + end + + it 'returns the service without pull policy' do + request_job + + expect(response).to have_gitlab_http_status(:created) + expect(json_response).to include( + 'id' => job.id, + 'services' => [{ 'alias' => nil, 'command' => nil, 'entrypoint' => nil, 'name' => 'postgres:11.9', + 'ports' => [], 'variables' => [] }] + ) + end + end + end + describe 'a job with excluded artifacts' do context 'when excluded paths are defined' do let(:job) do -- GitLab From 2ea9778702257b4cc50bc10185c8ac03fd3e09f8 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Wed, 15 Jun 2022 17:27:03 +0300 Subject: [PATCH 2/3] Apply reviewer feedback --- lib/gitlab/ci/config/entry/service.rb | 2 +- spec/lib/gitlab/ci/config/entry/image_spec.rb | 1 + spec/lib/gitlab/ci/config/entry/service_spec.rb | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/config/entry/service.rb b/lib/gitlab/ci/config/entry/service.rb index e88a2d7abc356e..37c5ef9e7cb41b 100644 --- a/lib/gitlab/ci/config/entry/service.rb +++ b/lib/gitlab/ci/config/entry/service.rb @@ -67,7 +67,7 @@ def value name: @config[:name], entrypoint: @config[:entrypoint], ports: (ports_value if ports_defined?), - pull_policy: (ci_docker_image_pull_policy_enabled? ? pull_policy_value : nil), + pull_policy: (pull_policy_value if ci_docker_image_pull_policy_enabled?), command: @config[:command], alias: @config[:alias], variables: (variables_value if variables_defined?) diff --git a/spec/lib/gitlab/ci/config/entry/image_spec.rb b/spec/lib/gitlab/ci/config/entry/image_spec.rb index 9f53b6859234e5..0fa6d4f8804dcd 100644 --- a/spec/lib/gitlab/ci/config/entry/image_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/image_spec.rb @@ -9,6 +9,7 @@ before do stub_feature_flags(ci_docker_image_pull_policy: true) + entry.compose! end diff --git a/spec/lib/gitlab/ci/config/entry/service_spec.rb b/spec/lib/gitlab/ci/config/entry/service_spec.rb index fb34dd828c5452..3c000fd09ed37a 100644 --- a/spec/lib/gitlab/ci/config/entry/service_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/service_spec.rb @@ -12,7 +12,7 @@ entry.compose! end - let(:entry) { described_class.new(config) } + subject(:entry) { described_class.new(config) } context 'when configuration is a string' do let(:config) { 'postgresql:9.5' } -- GitLab From 80817de20386bdd3537097a80781fb31adff4989 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Tue, 21 Jun 2022 16:14:50 +0300 Subject: [PATCH 3/3] Use a simpler change --- lib/gitlab/ci/config/entry/service.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/ci/config/entry/service.rb b/lib/gitlab/ci/config/entry/service.rb index 37c5ef9e7cb41b..1a35f7de6cfb1a 100644 --- a/lib/gitlab/ci/config/entry/service.rb +++ b/lib/gitlab/ci/config/entry/service.rb @@ -63,15 +63,9 @@ def value if string? { name: @config } elsif hash? - { - name: @config[:name], - entrypoint: @config[:entrypoint], - ports: (ports_value if ports_defined?), - pull_policy: (pull_policy_value if ci_docker_image_pull_policy_enabled?), - command: @config[:command], - alias: @config[:alias], - variables: (variables_value if variables_defined?) - }.compact + @config.merge( + pull_policy: (pull_policy_value if ci_docker_image_pull_policy_enabled?) + ).compact else {} end -- GitLab