diff --git a/app/controllers/concerns/observability/content_security_policy.rb b/app/controllers/concerns/observability/content_security_policy.rb index 1e25dc492a0837b44add19565794c7f1fa770873..e51d986d36ce599a0e9bc8033f2cdf504075e75f 100644 --- a/app/controllers/concerns/observability/content_security_policy.rb +++ b/app/controllers/concerns/observability/content_security_policy.rb @@ -5,26 +5,23 @@ module ContentSecurityPolicy extend ActiveSupport::Concern included do - content_security_policy_with_context do |p| - current_group = if defined?(group) - group - else - defined?(project) ? project&.group : nil - end - - next if p.directives.blank? || !Feature.enabled?(:observability_group_tab, current_group) + content_security_policy do |p| + next if p.directives.blank? default_frame_src = p.directives['frame-src'] || p.directives['default-src'] - - # When ObservabilityUI is not authenticated, it needs to be able - # to redirect to the GL sign-in page, hence '/users/sign_in' and '/oauth/authorize' + # When Gitlab Observability Backend is not authenticated, it needs to be able + # to redirect to the GitLab sign-in page, hence '/users/sign_in' and '/oauth/authorize' frame_src_values = Array.wrap(default_frame_src) | [ Gitlab::Observability.observability_url, Gitlab::Utils.append_path(Gitlab.config.gitlab.url, '/users/sign_in'), Gitlab::Utils.append_path(Gitlab.config.gitlab.url, '/oauth/authorize') ] - p.frame_src(*frame_src_values) + + default_connect_src = p.directives['connect-src'] || p.directives['default-src'] + connect_src_values = + Array.wrap(default_connect_src) | [Gitlab::Observability.observability_url] + p.connect_src(*connect_src_values) end end end diff --git a/app/controllers/projects/tracing_controller.rb b/app/controllers/projects/tracing_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..71ca03deb8ccfa380fde2cc75400b5e6ded186ba --- /dev/null +++ b/app/controllers/projects/tracing_controller.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Projects + class TracingController < Projects::ApplicationController + include ::Observability::ContentSecurityPolicy + + feature_category :tracing + + before_action :check_tracing_enabled + + def index + # TODO frontend changes coming separately https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125014 + render html: helpers.tag.strong('Tracing') + end + + private + + def check_tracing_enabled + render_404 unless Gitlab::Observability.tracing_enabled?(project) + end + end +end diff --git a/config/feature_flags/development/observability_tracing.yml b/config/feature_flags/development/observability_tracing.yml new file mode 100644 index 0000000000000000000000000000000000000000..a32d84096088d09b1c528e554b7172ccb1d015a4 --- /dev/null +++ b/config/feature_flags/development/observability_tracing.yml @@ -0,0 +1,8 @@ +--- +name: observability_tracing +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124966 +rollout_issue_url: https://gitlab.com/gitlab-org/opstrace/opstrace/-/issues/2252 +milestone: '16.2' +type: development +group: group::observability +default_enabled: false diff --git a/config/routes/project.rb b/config/routes/project.rb index 533e6c63e24b7f530ddf6eafbd8a024e07515042..787d95669d1e31b06b12c5b358e187c40958c2ae 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -404,6 +404,8 @@ end end + resources :tracing, only: [:index], controller: :tracing + namespace :design_management do namespace :designs, path: 'designs/:design_id(/:sha)', constraints: -> (params) { params[:sha].nil? || Gitlab::Git.commit_id?(params[:sha]) } do resource :raw_image, only: :show diff --git a/ee/spec/requests/groups/epics_controller_spec.rb b/ee/spec/requests/groups/epics_controller_spec.rb index a9cbeef35acb889fb7c74dbca98fa38da5159fcf..41014aadc87d91bbdaf87f13ed1e58d26c90c732 100644 --- a/ee/spec/requests/groups/epics_controller_spec.rb +++ b/ee/spec/requests/groups/epics_controller_spec.rb @@ -3,17 +3,20 @@ require 'spec_helper' RSpec.describe Groups::EpicsController, feature_category: :portfolio_management do - let(:group) { create(:group, :private) } - let(:epic) { create(:epic, group: group) } - let(:user) { create(:user) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:epic) { create(:epic, group: group) } + let_it_be(:user) { create(:user) } - describe 'GET #new' do - before do - stub_licensed_features(epics: true) - sign_in(user) - group.add_developer(user) - end + before_all do + group.add_developer(user) + end + before do + stub_licensed_features(epics: true) + sign_in(user) + end + + describe 'GET #new' do it_behaves_like "observability csp policy", described_class do let(:tested_path) do new_group_epic_path(group) @@ -22,29 +25,16 @@ end describe 'GET #show' do - before do - sign_in(user) - end - it_behaves_like "observability csp policy", described_class do - before do - group.add_developer(user) - stub_licensed_features(epics: true) - end - let(:tested_path) do group_epic_path(group, epic) end end context 'for summarize notes feature' do - let(:group) { create(:group, :public) } - before do allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?).with(user, :summarize_notes, epic).and_return(summarize_notes_enabled) - - stub_licensed_features(epics: true) end context 'when feature is available set' do diff --git a/lib/gitlab/observability.rb b/lib/gitlab/observability.rb index f7f65c91339289837f760aba68d181c6b90cfa5f..0e6089e1d211aadffc80a433a282763b84f28dc9 100644 --- a/lib/gitlab/observability.rb +++ b/lib/gitlab/observability.rb @@ -23,7 +23,13 @@ def observability_url 'https://observe.gitlab.com' end - # Returns true if the Observability feature flag is enabled + def oauth_url + "#{Gitlab::Observability.observability_url}/v1/auth/start" + end + + # Returns true if the GitLab Observability UI (GOUI) feature flag is enabled + # + # @deprecated # def enabled?(group = nil) return Feature.enabled?(:observability_group_tab, group) if group @@ -31,6 +37,11 @@ def enabled?(group = nil) Feature.enabled?(:observability_group_tab) end + # Returns true if Tracing UI is enabled + def tracing_enabled?(project) + Feature.enabled?(:observability_tracing, project) + end + # Returns the embeddable Observability URL of a given URL # # - Validates the URL diff --git a/lib/sidebars/projects/menus/monitor_menu.rb b/lib/sidebars/projects/menus/monitor_menu.rb index a74448d0bdc4e2cf87fbb23203b6bc3356bf655f..87b09e42fe11a3ff7904d98438235c66adb8c254 100644 --- a/lib/sidebars/projects/menus/monitor_menu.rb +++ b/lib/sidebars/projects/menus/monitor_menu.rb @@ -8,6 +8,7 @@ class MonitorMenu < ::Sidebars::Menu def configure_menu_items return false unless feature_enabled? + add_item(tracing_menu_item) add_item(error_tracking_menu_item) add_item(alert_management_menu_item) add_item(incidents_menu_item) @@ -62,6 +63,20 @@ def error_tracking_menu_item ) end + def tracing_menu_item + unless Gitlab::Observability.tracing_enabled?(context.project) + return ::Sidebars::NilMenuItem.new(item_id: :tracing) + end + + ::Sidebars::MenuItem.new( + title: _('Tracing'), + link: project_tracing_index_path(context.project), + super_sidebar_parent: ::Sidebars::Projects::SuperSidebarMenus::MonitorMenu, + active_routes: { controller: :tracing }, + item_id: :tracing + ) + end + def alert_management_menu_item unless can?(context.current_user, :read_alert_management_alert, context.project) return ::Sidebars::NilMenuItem.new(item_id: :alert_management) diff --git a/lib/sidebars/projects/super_sidebar_menus/monitor_menu.rb b/lib/sidebars/projects/super_sidebar_menus/monitor_menu.rb index 6e64ac01ffae298c49c94dc2329d749ae0fed830..0441d3b4a033cafee15034df58f5d4e17a3a12c1 100644 --- a/lib/sidebars/projects/super_sidebar_menus/monitor_menu.rb +++ b/lib/sidebars/projects/super_sidebar_menus/monitor_menu.rb @@ -17,6 +17,7 @@ def sprite_icon override :configure_menu_items def configure_menu_items [ + :tracing, :error_tracking, :alert_management, :incidents, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 92bbc28876cbd092d8004e69661e7a087005a28f..daa40c5f62187b83554f6af09ca69ed103f19794 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -48319,6 +48319,9 @@ msgstr "" msgid "TotalRefCountIndicator|1000+" msgstr "" +msgid "Tracing" +msgstr "" + msgid "Track groups of issues that share a theme, across projects and milestones" msgstr "" diff --git a/spec/lib/gitlab/observability_spec.rb b/spec/lib/gitlab/observability_spec.rb index 5082d193197c749e13135e6ec4a244b16c2e9efe..84d591e25204e617503f17a5bf0e0781e53e44a9 100644 --- a/spec/lib/gitlab/observability_spec.rb +++ b/spec/lib/gitlab/observability_spec.rb @@ -31,6 +31,12 @@ end end + describe '.oauth_url' do + subject { described_class.oauth_url } + + it { is_expected.to eq("#{described_class.observability_url}/v1/auth/start") } + end + describe '.build_full_url' do let_it_be(:group) { build_stubbed(:group, id: 123) } let(:observability_url) { described_class.observability_url } @@ -148,6 +154,27 @@ end end + describe '.tracing_enabled?' do + let_it_be(:project) { create(:project, :repository) } + + it 'returns true if feature is enabled globally' do + expect(described_class.tracing_enabled?(project)).to eq(true) + end + + it 'returns true if feature is enabled for the project' do + stub_feature_flags(observability_tracing: false) + stub_feature_flags(observability_tracing: project) + + expect(described_class.tracing_enabled?(project)).to eq(true) + end + + it 'returns false if feature is disabled globally' do + stub_feature_flags(observability_tracing: false) + + expect(described_class.tracing_enabled?(project)).to eq(false) + end + end + describe '.allowed_for_action?' do let(:group) { build_stubbed(:group) } let(:user) { build_stubbed(:user) } diff --git a/spec/lib/sidebars/projects/menus/monitor_menu_spec.rb b/spec/lib/sidebars/projects/menus/monitor_menu_spec.rb index 363822ee5e45cf758d6b0e7cf8b2185c753c5003..c0787aa9db53c8a8a53f1bdbad8a1a7f505748fa 100644 --- a/spec/lib/sidebars/projects/menus/monitor_menu_spec.rb +++ b/spec/lib/sidebars/projects/menus/monitor_menu_spec.rb @@ -7,7 +7,9 @@ let(:user) { project.first_owner } let(:show_cluster_hint) { true } - let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project, show_cluster_hint: show_cluster_hint) } + let(:context) do + Sidebars::Projects::Context.new(current_user: user, container: project, show_cluster_hint: show_cluster_hint) + end subject { described_class.new(context) } @@ -86,5 +88,19 @@ it_behaves_like 'access rights checks' end + + describe 'Tracing' do + let(:item_id) { :tracing } + + specify { is_expected.not_to be_nil } + + describe 'when feature is disabled' do + before do + stub_feature_flags(observability_tracing: false) + end + + specify { is_expected.to be_nil } + end + end end end diff --git a/spec/lib/sidebars/projects/super_sidebar_menus/monitor_menu_spec.rb b/spec/lib/sidebars/projects/super_sidebar_menus/monitor_menu_spec.rb index e59062c7eaf12044ca5818b7443c5c55d17315e1..e5c5204e0b44f30e28f98528d4e775ee204af54e 100644 --- a/spec/lib/sidebars/projects/super_sidebar_menus/monitor_menu_spec.rb +++ b/spec/lib/sidebars/projects/super_sidebar_menus/monitor_menu_spec.rb @@ -15,6 +15,7 @@ it 'defines list of NilMenuItem placeholders' do expect(items.map(&:class).uniq).to eq([Sidebars::NilMenuItem]) expect(items.map(&:item_id)).to eq([ + :tracing, :error_tracking, :alert_management, :incidents, diff --git a/spec/requests/groups/observability_controller_spec.rb b/spec/requests/groups/observability_controller_spec.rb index b82cf2b0bad2c2004340ac63f56d2ba91ec32b7e..247535bc990d8dc3dcbb2b34ae084756bd764502 100644 --- a/spec/requests/groups/observability_controller_spec.rb +++ b/spec/requests/groups/observability_controller_spec.rb @@ -17,6 +17,10 @@ end it_behaves_like 'observability csp policy' do + before_all do + group.add_developer(user) + end + let(:tested_path) { path } end diff --git a/spec/requests/projects/issues_controller_spec.rb b/spec/requests/projects/issues_controller_spec.rb index 583fd5f586ee23e783fbc55301f3be4776428cbd..1ae65939c86a8e988f390d5ad0e60cf170b5deec 100644 --- a/spec/requests/projects/issues_controller_spec.rb +++ b/spec/requests/projects/issues_controller_spec.rb @@ -17,6 +17,11 @@ describe 'GET #new' do include_context 'group project issue' + before do + group.add_developer(user) + login_as(user) + end + it_behaves_like "observability csp policy", described_class do let(:tested_path) do new_project_issue_path(project) @@ -26,11 +31,13 @@ describe 'GET #show' do before do + group.add_developer(user) login_as(user) end it_behaves_like "observability csp policy", described_class do include_context 'group project issue' + let(:tested_path) do project_issue_path(project, issue) end diff --git a/spec/requests/projects/merge_requests/creations_spec.rb b/spec/requests/projects/merge_requests/creations_spec.rb index ace6ef0f7b822b86884cfe6d2894b6c7bbc9f84f..e8a073fef5f3a986c7fbb323b8dc9aac0354f7d2 100644 --- a/spec/requests/projects/merge_requests/creations_spec.rb +++ b/spec/requests/projects/merge_requests/creations_spec.rb @@ -6,8 +6,13 @@ describe 'GET /:namespace/:project/merge_requests/new' do include ProjectForksHelper - let(:project) { create(:project, :repository) } - let(:user) { project.first_owner } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:user) { create(:user) } + + before_all do + group.add_developer(user) + end before do login_as(user) @@ -26,16 +31,13 @@ def get_new end it_behaves_like "observability csp policy", Projects::MergeRequests::CreationsController do - let_it_be(:group) { create(:group) } - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, group: group) } let(:tested_path) do project_new_merge_request_path(project, merge_request: { title: 'Some feature', - source_branch: 'fix', - target_branch: 'feature', - target_project: project, - source_project: project + source_branch: 'fix', + target_branch: 'feature', + target_project: project, + source_project: project }) end end diff --git a/spec/requests/projects/merge_requests_controller_spec.rb b/spec/requests/projects/merge_requests_controller_spec.rb index 955e682221119d42570e7a4a27eab5f7797a02df..955b6e53686f165c6e536eb5b0b67c0bd1befad1 100644 --- a/spec/requests/projects/merge_requests_controller_spec.rb +++ b/spec/requests/projects/merge_requests_controller_spec.rb @@ -16,6 +16,7 @@ context 'when logged in' do before do + group.add_developer(user) login_as(user) end diff --git a/spec/requests/projects/tracing_controller_spec.rb b/spec/requests/projects/tracing_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..520e99b120adaecfe667912199581e44d794ef97 --- /dev/null +++ b/spec/requests/projects/tracing_controller_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::TracingController, feature_category: :tracing do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let(:path) { nil } + let(:observability_tracing_ff) { true } + + subject do + get path + response + end + + describe 'GET #index' do + before do + stub_feature_flags(observability_tracing: observability_tracing_ff) + sign_in(user) + end + + let(:path) { project_tracing_index_path(project) } + + it_behaves_like 'observability csp policy' do + before_all do + project.add_developer(user) + end + + let(:tested_path) { path } + end + + context 'when user does not have permissions' do + it 'returns 404' do + expect(subject).to have_gitlab_http_status(:not_found) + end + end + + context 'when user has permissions' do + before_all do + project.add_developer(user) + end + + it 'returns 200' do + expect(subject).to have_gitlab_http_status(:ok) + end + + context 'when feature is disabled' do + let(:observability_tracing_ff) { false } + + it 'returns 404' do + expect(subject).to have_gitlab_http_status(:not_found) + end + end + end + end +end diff --git a/spec/support/shared_contexts/navbar_structure_context.rb b/spec/support/shared_contexts/navbar_structure_context.rb index efb4d244c109bfd5fde18a11f16d3185a981fdd5..0abf688566a17acc2ccc6054416cf99983d944c9 100644 --- a/spec/support/shared_contexts/navbar_structure_context.rb +++ b/spec/support/shared_contexts/navbar_structure_context.rb @@ -82,6 +82,7 @@ { nav_item: _('Monitor'), nav_sub_items: [ + _('Tracing'), _('Error Tracking'), _('Alerts'), _('Incidents') diff --git a/spec/support/shared_examples/observability/csp_shared_examples.rb b/spec/support/shared_examples/observability/csp_shared_examples.rb index 9d6e7e75f4de940df42a8bbe3e2f0f03f7246dec..9002ccd5878f481fa2cb15155ec4257ea1599d9b 100644 --- a/spec/support/shared_examples/observability/csp_shared_examples.rb +++ b/spec/support/shared_examples/observability/csp_shared_examples.rb @@ -5,23 +5,28 @@ # It requires the following variables declared in the context including this example: # # - `tested_path`: the path under test -# - `user`: the test user -# - `group`: the test group # # e.g. # # ``` -# let_it_be(:group) { create(:group) } -# let_it_be(:user) { create(:user) } # it_behaves_like "observability csp policy" do # let(:tested_path) { ....the path under test } # end # ``` # +# Note that the context's user is expected to be logged-in and the +# related resources (group, project, etc) are supposed to be provided with proper +# permissions already, e.g. +# +# before do +# login_as(user) +# group.add_developer(user) +# end +# # It optionally supports specifying the controller class handling the tested path as a parameter, e.g. # # ``` -# it_behaves_like "observability csp policy", Groups::ObservabilityController +# it_behaves_like "observability csp policy", Projects::TracingController # ``` # (If not specified it will default to `described_class`) # @@ -41,9 +46,6 @@ before do setup_csp_for_controller(controller_class, csp, any_time: true) - group.add_developer(user) - login_as(user) - stub_feature_flags(observability_group_tab: true) end subject do @@ -59,93 +61,127 @@ end end - context 'when observability is disabled' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.frame_src 'https://something.test' + describe 'frame-src' do + context 'when frame-src exists in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.frame_src 'https://something.test' + end end - end - before do - stub_feature_flags(observability_group_tab: false) + it 'appends the proper url to frame-src CSP directives' do + expect(subject).to include( + "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") + end end - it 'does not add observability urls to the csp header' do - expect(subject).to include("frame-src https://something.test") - expect(subject).not_to include("#{observability_url} #{signin_url} #{oauth_url}") - end - end + context 'when signin url is already present in the policy' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.frame_src signin_url + end + end - context 'when frame-src exists in the CSP config' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.frame_src 'https://something.test' + it 'does not append signin again' do + expect(subject).to include( + "frame-src #{signin_url} #{observability_url} #{oauth_url};") end end - it 'appends the proper url to frame-src CSP directives' do - expect(subject).to include( - "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") - end - end + context 'when oauth url is already present in the policy' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.frame_src oauth_url + end + end - context 'when signin is already present in the policy' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.frame_src signin_url + it 'does not append oauth again' do + expect(subject).to include( + "frame-src #{oauth_url} #{observability_url} #{signin_url};") end end - it 'does not append signin again' do - expect(subject).to include( - "frame-src #{signin_url} #{observability_url} #{oauth_url};") - end - end + context 'when default-src exists in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.default_src 'https://something.test' + end + end + + it 'does not change default-src' do + expect(subject).to include( + "default-src https://something.test;") + end - context 'when oauth is already present in the policy' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.frame_src oauth_url + it 'appends the proper url to frame-src CSP directives' do + expect(subject).to include( + "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") end end - it 'does not append oauth again' do - expect(subject).to include( - "frame-src #{oauth_url} #{observability_url} #{signin_url};") + context 'when frame-src and default-src exist in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.default_src 'https://something_default.test' + p.frame_src 'https://something.test' + end + end + + it 'appends to frame-src CSP directives' do + expect(subject).to include( + "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") + expect(subject).to include( + "default-src https://something_default.test") + end end end - context 'when default-src exists in the CSP config' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.default_src 'https://something.test' + describe 'connect-src' do + context 'when connect-src exists in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.connect_src 'https://something.test' + end end - end - it 'does not change default-src' do - expect(subject).to include( - "default-src https://something.test;") + it 'appends the proper url to connect-src CSP directives' do + expect(subject).to include( + "connect-src https://something.test localhost #{observability_url}") + end end - it 'appends the proper url to frame-src CSP directives' do - expect(subject).to include( - "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") - end - end + context 'when default-src exists in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.default_src 'https://something.test' + end + end + + it 'does not change default-src' do + expect(subject).to include( + "default-src https://something.test;") + end - context 'when frame-src and default-src exist in the CSP config' do - let(:csp) do - ActionDispatch::ContentSecurityPolicy.new do |p| - p.default_src 'https://something_default.test' - p.frame_src 'https://something.test' + it 'appends the proper url to connect-src CSP directives' do + expect(subject).to include( + "connect-src https://something.test localhost #{observability_url}") end end - it 'appends to frame-src CSP directives' do - expect(subject).to include( - "frame-src https://something.test #{observability_url} #{signin_url} #{oauth_url}") - expect(subject).to include( - "default-src https://something_default.test") + context 'when connect-src and default-src exist in the CSP config' do + let(:csp) do + ActionDispatch::ContentSecurityPolicy.new do |p| + p.default_src 'https://something_default.test' + p.connect_src 'https://something.test' + end + end + + it 'appends to connect-src CSP directives' do + expect(subject).to include( + "connect-src https://something.test localhost #{observability_url}") + expect(subject).to include( + "default-src https://something_default.test") + end end end end