From b0c1622a39a5b2686c60d2eab27881283a1d2d12 Mon Sep 17 00:00:00 2001 From: Daniele Rossetti <drossetti@gitlab.com> Date: Tue, 11 Jul 2023 16:42:52 +0000 Subject: [PATCH] Scaffold Tracing UI - Add tracing feature flag - Add nav menu - Set up basic tracing route/controller - Make observability csp concern more generic - Add connect-src to observability csp rules --- .../observability/content_security_policy.rb | 21 +-- .../projects/tracing_controller.rb | 22 +++ .../development/observability_tracing.yml | 8 + config/routes/project.rb | 2 + .../requests/groups/epics_controller_spec.rb | 34 ++-- lib/gitlab/observability.rb | 13 +- lib/sidebars/projects/menus/monitor_menu.rb | 15 ++ .../super_sidebar_menus/monitor_menu.rb | 1 + locale/gitlab.pot | 3 + spec/lib/gitlab/observability_spec.rb | 27 +++ .../projects/menus/monitor_menu_spec.rb | 18 +- .../super_sidebar_menus/monitor_menu_spec.rb | 1 + .../groups/observability_controller_spec.rb | 4 + .../projects/issues_controller_spec.rb | 7 + .../projects/merge_requests/creations_spec.rb | 20 +- .../merge_requests_controller_spec.rb | 1 + .../projects/tracing_controller_spec.rb | 56 ++++++ .../navbar_structure_context.rb | 1 + .../observability/csp_shared_examples.rb | 172 +++++++++++------- 19 files changed, 313 insertions(+), 113 deletions(-) create mode 100644 app/controllers/projects/tracing_controller.rb create mode 100644 config/feature_flags/development/observability_tracing.yml create mode 100644 spec/requests/projects/tracing_controller_spec.rb diff --git a/app/controllers/concerns/observability/content_security_policy.rb b/app/controllers/concerns/observability/content_security_policy.rb index 1e25dc492a08..e51d986d36ce 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 000000000000..71ca03deb8cc --- /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 000000000000..a32d84096088 --- /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 533e6c63e24b..787d95669d1e 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 a9cbeef35acb..41014aadc87d 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 f7f65c913392..0e6089e1d211 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 a74448d0bdc4..87b09e42fe11 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 6e64ac01ffae..0441d3b4a033 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 92bbc28876cb..daa40c5f6218 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 5082d193197c..84d591e25204 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 363822ee5e45..c0787aa9db53 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 e59062c7eaf1..e5c5204e0b44 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 b82cf2b0bad2..247535bc990d 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 583fd5f586ee..1ae65939c86a 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 ace6ef0f7b82..e8a073fef5f3 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 955e68222111..955b6e53686f 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 000000000000..520e99b120ad --- /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 efb4d244c109..0abf688566a1 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 9d6e7e75f4de..9002ccd5878f 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 -- GitLab