Merge branch 'jorlando/integration-slack-rate-limiting' of github.com…

…:grafana/oncall into jorlando/integration-slack-rate-limiting

.github/workflows/linting-and-tests.yml

@@ -85,6 +85,59 @@ jobs:
           python manage.py makemigrations --check
           python manage.py lintmigrations
 
+  # the following CI check is to prevent developers from dropping columns in a way that could cause downtime
+  # (the proper way to drop columns is documented in dev/README.md)
+  #
+  # we've been bitten by this before (see https://raintank-corp.slack.com/archives/C081TNWM73N as an example)
+  ensure-database-migrations-drop-columns-the-correct-way:
+    name: "Ensure database migrations drop columns the correct way"
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout PR code
+        uses: actions/checkout@v3
+        with:
+          # Fetch all history so we can compare with the base branch
+          fetch-depth: 0
+          # Checkout the head commit of the PR
+          ref: ${{ github.event.pull_request.head.sha }}
+
+      - name: Fetch base branch
+        run: git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }}
+
+      - name: Check for RemoveField in Migrations
+        # yamllint disable rule:line-length
+        run: |
+          # Get the list of files changed in the PR
+          git diff --name-only ${{ github.event.pull_request.base.ref }}...${{ github.event.pull_request.head.sha }} > changed_files.txt
+
+          # Filter for migration files
+          grep -E '^.*/migrations/.*\.py$' changed_files.txt > migration_files.txt || true
+
+          # Initialize a flag
+          FAILED=0
+
+          # Check each migration file for 'migrations.RemoveField'
+          if [ -s migration_files.txt ]; then
+            while IFS= read -r file; do
+              echo "Checking $file for migrations.RemoveField..."
+              if grep -q 'migrations.RemoveField' "$file"; then
+                echo "❌ Error: Found migrations.RemoveField in $file"
+                FAILED=1
+              else
+                echo "✅ No RemoveField found in $file"
+              fi
+            done < migration_files.txt
+          else
+            echo "No migration files changed."
+          fi
+
+          # Fail the job if RemoveField was found
+          if [ "$FAILED" -eq 1 ]; then
+            echo "❌ Error: Found migrations.RemoveField in one or more migration files. Please check out our documentation at https://github.com/grafana/oncall/tree/dev/dev#removing-a-nullable-field-from-a-model on how to properly drop columns."
+            exit 1
+          fi
+        # yamllint enable rule:line-length
+
   unit-test-helm-chart:
     name: "Helm Chart Unit Tests"
     runs-on: ubuntu-latest-16-cores
@@ -193,7 +246,7 @@ jobs:
       ONCALL_TESTING_RBAC_ENABLED: ${{ matrix.rbac_enabled }}
     services:
       redis_test:
-        image: redis:7.0.5
+        image: redis:7.0.15
         ports:
           - 6379:6379
         options: >-

dev/scripts/generate-fake-data/requirements.txt

@@ -1,3 +1,3 @@
-aiohttp==3.10.2
+aiohttp==3.10.11
 Faker==16.4.0
 tqdm==4.66.3

docker-compose-developer.yml

@@ -172,7 +172,7 @@ services:
   redis:
     container_name: redis
     labels: *oncall-labels
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     ports:
       - "6379:6379"

docker-compose-mysql-rabbitmq.yml

@@ -87,7 +87,7 @@ services:
       retries: 10
 
   redis:
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     expose:
       - 6379

docker-compose.yml

@@ -54,7 +54,7 @@ services:
         condition: service_healthy
 
   redis:
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     expose:
       - 6379

engine/apps/alerts/incident_log_builder/incident_log_builder.py

@@ -152,6 +152,15 @@ def _get_user_notification_log_records_for_log_report(self) -> "RelatedManager['
                     Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_TRIGGERED)
                     & Q(notification_policy__step=UserNotificationPolicy.Step.WAIT)
                 )
+                # Exclude SUCCESS + ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED, these cause confusions as the user
+                # has already been notified by another path so this step should not be displayed, although it is kept
+                # for auditing.
+                | Q(
+                    Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS)
+                    & Q(
+                        notification_error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED
+                    )
+                )
             )
             .select_related("author")
             .distinct()

engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py

@@ -12,14 +12,6 @@ class Migration(migrations.Migration):
 
     operations = [
         linter.IgnoreMigration(),
-        migrations.RemoveField(
-            model_name='channelfilter',
-            name='_slack_channel_id',
-        ),
-        migrations.RemoveField(
-            model_name='resolutionnoteslackmessage',
-            name='_slack_channel_id',
-        ),
         migrations.DeleteModel(
             name='AlertGroupPostmortem',
         ),

engine/apps/alerts/migrations/0067_remove_channelfilter__slack_channel_id_state.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:21
+
+import common.migrations.remove_field
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0066_remove_channelfilter__slack_channel_id_and_more'),
+    ]
+
+    operations = [
+        common.migrations.remove_field.RemoveFieldState(
+            model_name='channelfilter',
+            name='_slack_channel_id',
+        ),
+    ]

engine/apps/alerts/migrations/0068_remove_resolutionnoteslackmessage__slack_channel_id_state.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:23
+
+import common.migrations.remove_field
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0067_remove_channelfilter__slack_channel_id_state'),
+    ]
+
+    operations = [
+        common.migrations.remove_field.RemoveFieldState(
+            model_name='resolutionnoteslackmessage',
+            name='_slack_channel_id',
+        ),
+    ]

engine/apps/alerts/migrations/0069_remove_channelfilter__slack_channel_id_db.py

@@ -0,0 +1,21 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:21
+
+import common.migrations.remove_field
+import django_migration_linter as linter
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0068_remove_resolutionnoteslackmessage__slack_channel_id_state'),
+    ]
+
+    operations = [
+        linter.IgnoreMigration(),
+        common.migrations.remove_field.RemoveFieldDB(
+            model_name='channelfilter',
+            name='_slack_channel_id',
+            remove_state_migration=('alerts', '0067_remove_channelfilter__slack_channel_id_state'),
+        ),
+    ]

engine/apps/alerts/migrations/0070_remove_resolutionnoteslackmessage__slack_channel_id_db.py

@@ -0,0 +1,21 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:23
+
+import common.migrations.remove_field
+import django_migration_linter as linter
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0069_remove_channelfilter__slack_channel_id_db'),
+    ]
+
+    operations = [
+        linter.IgnoreMigration(),
+        common.migrations.remove_field.RemoveFieldDB(
+            model_name='resolutionnoteslackmessage',
+            name='_slack_channel_id',
+            remove_state_migration=('alerts', '0068_remove_resolutionnoteslackmessage__slack_channel_id_state'),
+        ),
+    ]

engine/apps/alerts/tasks/notify_user.py

@@ -528,7 +528,7 @@ def perform_notification(log_record_pk, use_default_notification_policy_fallback
                 )
                 UserNotificationPolicyLogRecord(
                     author=user,
-                    type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED,
+                    type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS,
                     notification_policy=notification_policy,
                     reason="Prevented from posting in Slack",
                     alert_group=alert_group,

engine/apps/alerts/tests/test_notify_user.py

@@ -315,7 +315,7 @@ def test_perform_notification_slack_prevent_posting(
 
     mocked_send_slack_notification.assert_not_called()
     last_log_record = UserNotificationPolicyLogRecord.objects.last()
-    assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED
+    assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS
     assert last_log_record.reason == "Prevented from posting in Slack"
     assert (
         last_log_record.notification_error_code

engine/apps/email/inbound.py

@@ -77,6 +77,7 @@ def dispatch(self, request):
         integration_token = self.get_integration_token_from_request(request)
         if integration_token is None:
             return HttpResponse(status=status.HTTP_400_BAD_REQUEST)
+        request.inbound_email_integration_token = integration_token  # used in RequestTimeLoggingMiddleware
         return super().dispatch(request, alert_channel_key=integration_token)
 
     def post(self, request):
@@ -138,14 +139,15 @@ def message(self) -> AnymailInboundMessage | None:
             try:
                 view.run_validators(self.request)
                 events = view.parse_events(self.request)
-            except (AnymailWebhookValidationFailure, AnymailAPIError) as e:
-                logger.info(f"inbound email webhook validation failed for ESP {esp}: {e}")
+            except (AnymailWebhookValidationFailure, AnymailAPIError):
                 continue
 
             messages = [event.message for event in events if isinstance(event, AnymailInboundEvent)]
             if messages:
+                logger.info(f"Received inbound email message from ESP: {esp}")
                 return messages[0]
 
+        logger.error("Failed to parse inbound email message")
         return None
 
     def check_inbound_email_settings_set(self):

engine/apps/public_api/serializers/resolution_notes.py

@@ -1,6 +1,7 @@
 from rest_framework import serializers
 
 from apps.alerts.models import AlertGroup, ResolutionNote
+from apps.user_management.models import ServiceAccountUser
 from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField, UserIdField
 from common.api_helpers.exceptions import BadRequest
 from common.api_helpers.mixins import EagerLoadingMixin
@@ -34,8 +35,9 @@ class Meta:
     SELECT_RELATED = ["alert_group", "resolution_note_slack_message", "author"]
 
     def create(self, validated_data):
-        if self.context["request"].user.pk:
-            validated_data["author"] = self.context["request"].user
+        user = self.context["request"].user
+        if not isinstance(user, ServiceAccountUser) and user.pk:
+            validated_data["author"] = user
         validated_data["source"] = ResolutionNote.Source.WEB
         return super().create(validated_data)
 

engine/apps/public_api/tests/test_resolution_notes.py

@@ -1,13 +1,16 @@
 from unittest.mock import patch
 
+import httpretty
 import pytest
 from django.urls import reverse
 from rest_framework import status
 from rest_framework.test import APIClient
 
 from apps.alerts.models import ResolutionNote
+from apps.api import permissions
 from apps.auth_token.auth import ApiTokenAuthentication, GrafanaServiceAccountAuthentication
 from apps.auth_token.models import ApiAuthToken, ServiceAccountToken
+from apps.auth_token.tests.helpers import setup_service_account_api_mocks
 
 
 @pytest.mark.django_db
@@ -146,6 +149,53 @@ def test_create_resolution_note(
     mock_send_update_resolution_note_signal.assert_called_once()
 
 
+@pytest.mark.django_db
+@httpretty.activate(verbose=True, allow_net_connect=False)
+@patch("apps.alerts.tasks.send_update_resolution_note_signal.send_update_resolution_note_signal.apply_async")
+def test_create_resolution_note_via_service_account(
+    mock_send_update_resolution_note_signal,
+    make_organization,
+    make_service_account_for_organization,
+    make_token_for_service_account,
+    make_alert_receive_channel,
+    make_alert_group,
+):
+    organization = make_organization(grafana_url="http://grafana.test")
+    service_account = make_service_account_for_organization(organization)
+    token_string = "glsa_token"
+    make_token_for_service_account(service_account, token_string)
+
+    perms = {
+        permissions.RBACPermission.Permissions.ALERT_GROUPS_WRITE.value: ["*"],
+    }
+    setup_service_account_api_mocks(organization, perms)
+
+    alert_receive_channel = make_alert_receive_channel(organization)
+    alert_group = make_alert_group(alert_receive_channel)
+    data = {
+        "alert_group_id": alert_group.public_primary_key,
+        "text": "Test Resolution Note Message",
+    }
+    url = reverse("api-public:resolution_notes-list")
+    client = APIClient()
+    response = client.post(
+        url,
+        data=data,
+        format="json",
+        HTTP_AUTHORIZATION=f"{token_string}",
+        HTTP_X_GRAFANA_URL=organization.grafana_url,
+    )
+    if not organization.is_rbac_permissions_enabled:
+        assert response.status_code == status.HTTP_403_FORBIDDEN
+    else:
+        assert response.status_code == status.HTTP_201_CREATED
+        mock_send_update_resolution_note_signal.assert_called_once()
+        resolution_note = ResolutionNote.objects.get(public_primary_key=response.data["id"])
+        assert resolution_note.author is None
+        assert resolution_note.text == data["text"]
+        assert resolution_note.alert_group == alert_group
+
+
 @pytest.mark.django_db
 def test_create_resolution_note_invalid_text(
     make_organization_and_user_with_token,

engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py

@@ -20,6 +20,15 @@
 
 
 class AmixrRecurringIcalEventsAdapter(IcalService):
+    def _normalize(self, dt: datetime.datetime) -> datetime.datetime:
+        tz = getattr(dt, "tzinfo", None)
+        if tz:
+            normalized = tz.normalize(dt)
+            if normalized.tzinfo != tz:
+                diff = dt.dst() - normalized.dst()
+                dt = normalized + diff
+        return dt
+
     def get_events_from_ical_between(
         self, calendar: Calendar, start_date: datetime.datetime, end_date: datetime.datetime
     ) -> typing.List[Event]:
@@ -38,6 +47,10 @@ def get_events_from_ical_between(
             end_date + datetime.timedelta(days=EXTRA_LOOKUP_DAYS),
         )
 
+        for event in events:
+            # account for timezones not being properly calculated when DST changes.
+            event[ICAL_DATETIME_END].dt = self._normalize(event[ICAL_DATETIME_END].dt)
+
         def filter_extra_days(event):
             event_start, event_end = self.get_start_and_end_with_respect_to_event_type(event)
             if event_start > event_end:

engine/apps/schedules/migrations/0020_remove_oncallschedule_channel.py → engine/apps/schedules/migrations/0020_remove_oncallschedule_channel_state.py

@@ -1,7 +1,7 @@
-# Generated by Django 4.2.16 on 2024-11-06 21:13
+# Generated by Django 4.2.16 on 2024-11-20 20:12
 
+import common.migrations.remove_field
 from django.db import migrations
-import django_migration_linter as linter
 
 
 class Migration(migrations.Migration):
@@ -11,8 +11,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
-        linter.IgnoreMigration(),
-        migrations.RemoveField(
+        common.migrations.remove_field.RemoveFieldState(
             model_name='oncallschedule',
             name='channel',
         ),