Skip to content
Snippets Groups Projects

Add HTTP integration GraphQL query

7 unresolved threads

What does this MR do?

Related to #321674 (closed)

Adds a new alertManagementHttpIntegration to fetch information about a single HTTP Integration by ID.

Why there is no alertManagementIntegration query similar to alertManagementHttpIntegration?

Alert Management Integration consists of two kinds: HTTP Integrations and Prometheus integration. The last can only be present in a single copy and I think there is no need to filter it by ID when it's already fetched from the alertManagementIntegrations query.

If we ignore filtering Prometheus integration in alertManagementIntegration, then alertManagementIntegration and alertManagementHttpIntegration will do exactly the same job.

One may argue, in the future, we may have more integration types and alertManagementIntegration is going to allow us to filter any of them by ID. I would suggest getting back to that discussion once we have more integration types.

GraphQL

Query

query Extract($fullPath: ID!, $id: AlertManagementHttpIntegrationID!) {
  project(fullPath: $fullPath) {
    alertManagementHttpIntegration(id: $id) {
      id
      active
      name
      payloadExample
      payloadAttributeMappings {
        fieldName
        label
        type
      }
    }
  }
}

Query variables

{
  "fullPath": "<PROJECT-FULL-PATH>",
  "id": "gid://gitlab/AlertManagement::HttpIntegration/<ID>"
}

Screenshots (strongly suggested)

Screenshot_2021-03-15_at_14.09.14

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Vitali Tatarintev

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
279 279 description: 'HTTP Integrations which can receive alerts for the project.',
280 280 resolver: Resolvers::AlertManagement::HttpIntegrationsResolver
281 281
282 field :alert_management_http_integration,
  • We already have a field on the ProjectType that seems suitable for this: Project.alertManagementHttpIntegrations (I notice we also have Project.alertManagementIntegrations - I am worried that we are accumulating several very similar fields on Project that might be confusing to users.

    Did you consider other approaches here, such as adding filter arguments to Project.alertManagementIntegrations?

  • Author Maintainer

    I was considering two approaches here. The one with the separate Project.alertManagementHttpIntegration and another one is adding a new filtering argument to the Project.alertManagementHttpIntegrations.

    I chose the new field approach due to its simplicity and single responsibility.

    But I don't mind flipping the solution in favor of Project.alertManagementHttpIntegrations.

    I didn't add the field to Project.alertManagementIntegrations because (I was trying to explain that in the MR description) that field returns two kinds of integrations: HTTP Integrations and the Prometheus service.

    The Prometheus service in there is always a single record (or blank I guess). That, I think, makes filtering by ID is a little bit tricky, because we need to identify the type to which is that ID belongs. Then, if the ID belongs to Prometheus, it doesn't make sense to filter by its ID again, because it's always a single record. Here we can only filter by HTTP integrations, but I thought it's could be odd to dismiss one of the types.

    Project.alertManagementHttpIntegrations on the other hand is only focused on HTTP integration, which has a couple of specific pieces of information we need when we edit one of the integration's settings.

  • I didn't add the field to Project.alertManagementIntegrations because (I was trying to explain that in the MR description) that field returns two kinds of integrations: HTTP Integrations and the Prometheus service.

    that is correct, but it doesn't seem like a large problem. We can distinguish which kind of integration this is with id.model_class.

  • Author Maintainer

    To filter by ID we need to get the ID. Get it id.model_class. Then check if the model class is ::AlertManagement::HttpIntegration.

    If it's an HttpIntegration we pass the integer ID to ::AlertManagement::HttpIntegrationsFinder to filter the HTTP Integrations. https://gitlab.com/gitlab-org/gitlab/-/blob/d3250ebe5506b66bb7d019841a1f66981456974b/app/graphql/resolvers/alert_management/integrations_resolver.rb#L25

    If it's not an HttpIntegration we compare the project.prometheus_service ID with the ID we have, if return the Prometheus service if IDs match.

    Did I understand that right?

  • Please register or sign in to reply
  • 2
    3 module Resolvers
    4 module AlertManagement
    5 class HttpIntegrationResolver < BaseResolver
    6 alias_method :project, :synchronized_object
    7
    8 type Types::AlertManagement::HttpIntegrationType, null: true
    9
    10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration],
    11 required: true,
    12 description: 'ID of the integration.'
    13
    14 def resolve(**args)
    15 return unless Ability.allowed?(current_user, :admin_operations, project)
    16
    17 GitlabSchema.object_from_id(args[:id], expected_class: ::AlertManagement::HttpIntegration)
  • 1 # frozen_string_literal: true
    2
    3 module Resolvers
    4 module AlertManagement
    5 class HttpIntegrationResolver < BaseResolver
    6 alias_method :project, :synchronized_object
    7
    8 type Types::AlertManagement::HttpIntegrationType, null: true
    9
    10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration],
    11 required: true,
    12 description: 'ID of the integration.'
    13
    14 def resolve(**args)
    15 return unless Ability.allowed?(current_user, :admin_operations, project)
    • We have field authorizations working now (huzzah!)

      That means this line (and the entire implementation) could be moved to the field instead, as:

      # in project_type.rb
      field :alert_management_http_integration
            type: Types::AlertManagement::HttpIntegrationType,
            null: true,
            description: 'A single HTTP integration which can receive alerts for the project.',
            authorize: :admin_operations do
              argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration],
                       required: true,
                       description: 'ID of the integration.'
            end
      
      def alert_management_http_integration(id:)
        id = Types::AlertManagement::HttpIntegrationType.coerce_isolated_input(id)
        GitlabSchema.find_by_gid(id)
      end

      we could even leverage :loads to make this even smaller, but I think this needs to wait for #257883 (closed) to be fully robust:

      # in project_type.rb
      field :alert_management_http_integration
            type: Types::AlertManagement::HttpIntegrationType,
            null: true,
            description: 'A single HTTP integration which can receive alerts for the project.',
            authorize: :admin_operations do
              argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration],
                       required: true,
                       loads: Types::AlertManagement::HttpIntegrationType,
                       as: :integration,
                       description: 'ID of the integration.'
            end
      
      def alert_management_http_integration(integration:)
        integration
      end

      Do we really need a resolver?

    • Please register or sign in to reply
  • 12 let_it_be(:project) { create(:project) }
    13 let_it_be(:integration) { create(:alert_management_http_integration, project: project) }
    14 let_it_be(:args) { { id: global_id_of(integration) } }
    15
    16 subject { sync(resolve_http_integration(args: args)) }
    17
    18 before do
    19 project.add_developer(developer)
    20 project.add_maintainer(maintainer)
    21 end
    22
    23 specify do
    24 expect(described_class).to have_nullable_graphql_type(Types::AlertManagement::HttpIntegrationType)
    25 end
    26
    27 context 'when user does not have permission' do
    • Hi @ck3g

      Thanks for this, but I think I need convincing that:

      • we need a new field for this
      • that the field needs to go under project
      • that we need a resolver in the first place

      Could you please explain these choices?

      Also, I think we are definitely missing a few tests. I think we can skip the N+1 specs, since the implementation is so simple it is guaranteed to be correct.

      Edited by Alex Kalderimis
    • Author Maintainer

      Thanks for the detailed review @alexkalderimis

      I've added an explanation about the choice between the two solutions. But I'm fine switching to the solution you're suggesting. Let me know which one you think would be the best here.

      Also, I think we are definitely missing a few tests.

      What tests are we missing here? I'll add them

    • What tests are we missing here? I'll add them

      I'm very sorry - I appear not to have posted a comment.

      We need a test that a user with access to project A and project B cannot request integration A.x from project B.

      There is no security risk here, but there is the risk of really confusing data (and just incorrect modelling) if we allow loading the integrations of one project underneath another.

      i.e.:

      query($id: AlertManagementHttpIntegrationID!) {
        a: project(fullPath: "a/b") {
          alertManagementHttpIntegration(id: $id) {
            name
          }
        }
        b: project(fullPath: "b/a") {
          alertManagementHttpIntegration(id: $id) {
            name
          }
        }
      }

      can logically return {a: nil, b: nil}, {a: nil, b: something} and {a: something, b: nil} but never: {a: something, b: something}.

    • Author Maintainer

      To summarize, those are the changes I need to make:

      1. Move filtering by ID to Project.alertManagementHttpIntegrations
      2. Also add filtering by ID tp Project.alertManagementIntegrations using a trick with id.model_class (described here !56647 (comment 534605189))
      3. Cover the scenario (if not yet covered) where a user cannot access an Integration that doesn't belong to the current project (from this question !56647 (comment 534607715))

      In this case, the question about resolvers is off the table, because we'll extend existing resolvers.

      @alexkalderimis Is everything correct here? Did I miss something?

    • Yes I think that makes sense.

      However:

      I am not sure what the front-end requirements are here, but we could avoid (3) by moving this field to Query and allow a single integration to be retrieved by ID (without the nesting, we would not need to verify consistency - and we already have a policy check on the class itself).

      So if we choose to model this as a nested field on Project, then we need to ensure that data is consistent. But we don't have to model it this way.

      There are tradeoffs to both positions in regard to (1) and (2). i.e. both adding a new field and adding an argument to an existing one add complexity. It really depends where we want that complexity to go, and how large we want the schema to get. What is the front-end requirement here? Are we prepared to maintain a large number of alert-management fields in the project type? Or would it be better to have a more complex resolver?

      I really am not insisting here - I am happy to be persuaded depending on what is right for you and your team.

    • Author Maintainer

      The main requirement for the frontend now is to be able to get a couple of extra fields which belong only to HTTP Integrations.

      The integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once. Also, there won't be many integrations per project.

      At the moment, there is no need to filter non HTTP integrations, so implementing this filter is mostly for sync functionality between alertManagementIntegrations and alertManagementHttpIntegrations. That's why I was trying to avoid that in the first place. I would still rather avoid doing that because we're going to implement a piece of functionality we don't need at the moment.

    • The main requirement for the frontend now is to be able to get a couple of extra fields which belong only to HTTP Integrations.

      we can do this now with inline fragments, no?

      The integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once.

      this is a public API - there are no guarantees on how clients will use it. If we advertise an entity as nested beneath another, that is a claim we need to be able to assure clients of.

      Also, there won't be many integrations per project.

      is there a hard limit? I see right now we are returning arrays (i.e. we are loading all integrations in memory on each request). Do we have an issue to track abuse of this feature?

      At the moment, there is no need to filter non HTTP integrations, so implementing this filter is mostly for sync functionality between alertManagementIntegrations and alertManagementHttpIntegrations

      so we are implementing a new field, to sync data between two other fields that only differ in their return type, and the type of one is an interface implemented by the other? That seems really odd to me. Surely, with fragments to select the fields we need, we just don't need that.

      Edited by Alex Kalderimis
    • Author Maintainer

      The integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once.

      this is a public API - there are no guarantees on how clients will use it. If we advertise an entity as nested beneath another, that is a claim we need to be able to assure clients of.

      Here I was actually defending the need to limit the query to filter only integrations of a specified project.

      is there a hard limit? I see right now we are returning arrays (i.e. we are loading all integrations in memory on each request). Do we have an issue to track abuse of this feature?

      Not sure about that. Probably not

      so we are implementing a new field, to sync data between two other fields that only differ in their return type, and the type of one is an interface implemented by the other? That seems really odd to me. Surely, with fragments to select the fields we need, we just don't need that.

      I'm lost here, I'm sorry. Let me rephrase it.

      I was trying to say, that there are two similar types alertManagementIntegrations and alertManagementHttpIntegrations. The last one is the subset of the first. On top of that HttpIntegrations can return additional information, which Integrations don't have. When we edit an HTTP integration we need to get that additional information. That's why we need to filter HTTP integrations by the ID.

      At the moment, we don't need to filter all integrations by ID. If we're going to add filtering to alertManagementIntegrations by ID, that's only if we want to have the same (or similar) filtering functionality in both these types.

      That's why I was trying to avoid adding new code, which we're not going to use at the moment.

    • I was trying to say, that there are two similar types alertManagementIntegrations and alertManagementHttpIntegrations. The last one is the subset of the first. On top of that HttpIntegrations can return additional information, which Integrations don't have.

      yes, thank you for clarifying. I saw that when I explored the code. I see that we have app/graphql/types/alert_management/integration_type.rb which defines an interface implemented by app/graphql/types/alert_management/http_integration_type.rb. This means that any field that returns AlertManagementIntegration entities may be returning objects of the concrete type AlertManagementHttpIntegration.

      The system in GraphQL for dealing with this (selecting fields of implementations of interfaces that do not belong on the interface) is called inline interfaces.

      That would look like:

      query {
        project(fullPath: "a/b") {
          alertManagementIntegrations {
            nodes {
              id type name
              ... on AlertManagementHttpIntegration {
                payloadExample
                payloadAlertFields {
                  type path label
                }
              }
            }
          }
        }
      }

      I can run this precise query today and it is completely valid against our current schema.

      Edited by Alex Kalderimis
    • Author Maintainer

      Oh, that's cool. I didn't know about that. Thanks.

      Ok. Let me update the solution and I'll get back to you.

    • Please register or sign in to reply
  • Alex Kalderimis assigned to @ck3g and unassigned @alexkalderimis

    assigned to @ck3g and unassigned @alexkalderimis

  • changed milestone to %13.11

  • 2
    3 module Resolvers
    4 module AlertManagement
    5 class HttpIntegrationResolver < BaseResolver
    6 alias_method :project, :synchronized_object
    7
    8 type Types::AlertManagement::HttpIntegrationType, null: true
    9
    10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration],
    11 required: true,
    12 description: 'ID of the integration.'
    13
    14 def resolve(**args)
    15 return unless Ability.allowed?(current_user, :admin_operations, project)
    16
    17 GitlabSchema.object_from_id(args[:id], expected_class: ::AlertManagement::HttpIntegration)
  • Vitali Tatarintev mentioned in merge request !57590 (merged)

    mentioned in merge request !57590 (merged)

  • Author Maintainer

    Closing in favour of !57590 (merged)

  • mentioned in issue #326295 (closed)

  • Please register or sign in to reply
    Loading