feat: support mattermost group level integration

[BoxBoxJason]

Description of your changes

Adds a groups.gitlab.(m.)crossplane.io/v1alpha1 IntegrationMattermost managed resource to configure the Mattermost notifications integration of a GitLab group, modelled on the group Harbor integration.

It also adds an opt-in way to detect changes to write-only secret values (such as the Mattermost webhook, passwords,...), which the GitLab API never returns, and fixes two small bugs in the project-level IntegrationMattermost types.

Closes #320

Group Mattermost integration

apiVersion: groups.gitlab.m.crossplane.io/v1alpha1
kind: IntegrationMattermost
metadata:
  name: mattermost-g7
  namespace: default
spec:
  providerConfigRef:
    name: gitlab-provider
    kind: ProviderConfig
  forProvider:
    groupId: 20
    webhookSecretRef:
      name: mattermost-webhook
      key: webhook
    username: gitlab-bot
    channel: gitlab-notifications
    pipelineEvents: true
    pipelineChannel: gitlab-pipelines

• Supports groupId / groupIdRef / groupIdSelector and every option of the group integration API. Compared with the project-level resource it adds deployment, alert and vulnerability events and channels, labelsToBeNotified(Behavior) and useInheritedSettings.
• The webhook is read from a secret (webhookSecretRef), in the same way as Harbor's passwordSecretRef, because the Mattermost webhook URL contains a secret token. It is never shown in status.atProvider.
• Cluster-scoped variants, CRDs and deepcopy/managed methods are generated by make generate.

Project IntegrationMattermost fixes

• The doc comments of confidentialNoteEvents and confidentialNoteChannel were swapped. The same wrong comment is also fixed on the unused CommonIntegrationParameters.
• Removed the BRANCH print column, which pointed at a spec.forProvider.branchName field that does not exist.

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

• make reviewable passes (generate, lint, unit tests).
• Unit tests:
  • Group Mattermost client helpers: building the request options (the webhook is left out when empty), building the observation (the webhook is never shown), and drift detection, including a check that the four fields above are ignored.
  • Group Mattermost controller: Observe / Create / Update / Delete, with and without a hasher:
    • webhook rotated, hash annotation missing, webhook secret unreadable;
    • hash set on create and persisted on update, patch failure;
    • no secret read and no patch when disabled;
    • deletion not blocked by a missing secret.
  • SecretHasher: HMAC output checked against the RFC 4231 test vector (case 2), key too short or missing, bad key tolerated while deleting, key rotation, merge-patch persistence, and disabled/nil behaviour.
  • ProviderConfig resolution: secretHashKeySecretRef is resolved for the legacy, namespaced v1beta2 (pinned to the ProviderConfig namespace) and ClusterProviderConfig flavors.
• Successfully built and ran the provider locally against a kubernetes cluster.
• Successfully created, updated, and deleted resources of the types I changed / created.
• Ensured reconciliation loops for the changed / created resource complete without error.
  • Creation
  • Update
  • Deletion
• Deleted the resource on the app side to ensure the provider correctly handles
  unexpected drift. (should result in recreation of the resource if applicable)
• Updated the resource on the app side to ensure the provider correctly handles
  unexpected drift. (should result in an update of the resource if applicable)

[henrysachs]

Hi @BoxBoxJason, thank you so much for this! Group-level Mattermost has been wanted for a while (#320), and this is a really thorough contribution:

• It covers both scopes.
• The tests are extensive, including the RFC 4231 vector.
• The reasoning in the description is careful. I checked the Create-vs-Update annotation persistence against crossplane-runtime v2.4.2, and it's spot on.

Taking you up on the split

Thanks for offering! I'd love to keep this PR to the group Mattermost resource (the small project-level fixes can stay) and move the write-only secret tracking to a follow-up. Two reasons:

• It adds public fields to all four ProviderConfig flavors.
• Validating it is easier once Harbor is a second consumer.

The Mattermost resource is useful without it. A few notes for that follow-up are at the end.

Before merging

1. Inheritance isn't sent or compared

• When useInheritedSettings is unset, client-go omits use_inherited_settings (clients/groups/integration_mattermost.go:56).
• GitLab then keeps the current state (Integrations::UpdateService#handle_inherited_settings?). For an inheriting record it only re-saves inherit_from_id and ignores all submitted attributes.
• Such records are common: if a parent group or the instance has Mattermost configured, GitLab propagates an inherited copy into every descendant group.
• Effects:
  • an active inherited record is "updated" on every poll, nothing changes, and the resource stays Ready without the webhook ever being applied;
  • an inactive inherited one loops through Create;
  • IsIntegrationMattermostUpToDate doesn't compare inheritance either, so false never detaches it.
• Suggestion:
  • Default nil to false and always send it.
  • Compare it against observation.Inherited.
  • While inheriting, skip the other comparisons and the webhook hash check.

2. alertEvents / alertChannel aren't supported by Mattermost

• Mattermost's SUPPORTED_EVENTS (from Base::ChatNotification) has no alert, so GitLab drops alert_events.
• The response still returns that column, which defaults to true. So alertEvents: false never converges.
• alert_channel is never returned at all.
• I'd remove both (integration_mattermost_types.go:123, :171).

3. Comparison tests. Only 4 of the 26 compared fields have a "differs" case. Removing the VulnerabilityChannel, ConfidentialNoteChannel or TagPushEvents comparison keeps the suite green. One case per field would pin it down.

4. Empty webhook. It's omitted from the request "so that existing remote state is not cleared", but GitLab requires webhook and answers 400. A clear provider error up front would be easier to debug.

Smaller things

• vulnerabilityEvents and vulnerabilityChannel only exist on GitLab EE. Worth noting in the field docs, since on CE they never converge.
• GitLab returns confidential_issue_channel.presence || issue_channel (same for notes). An explicit "" therefore keeps updating; MinLength=1 avoids it.
• Docs:
  • The group integrations API needs GitLab 17.9+.
  • Inheritance comes from the closest ancestor with custom settings, or else the instance.
  • A few options aren't covered yet (incident, work item, group mention, notify_only_when_pipeline_status_changes), mostly due to client-go.
• Optional: client-go's GetGroupMattermostSettings decodes the four excluded fields from properties, so drift detection for them looks possible today.

Notes for the secret-tracking follow-up

• Design question. Would an opaque marker of the Secret's UID, resourceVersion and key name at apply time be enough?

  • It needs no key management and no ProviderConfig fields.
  • The trade-off is an occasional harmless re-apply.
  • If we keep the HMAC, binding it to the resource and field (e.g. UID, field name, value) would stop anyone sharing a ProviderConfig from testing guesses against another resource's annotation.

• Tests. Reverting a77c0cc, or dropping o.SetResourceVersion(...), still passes. With the fake client both cause a 409 on the following status update, so a fake-client test asserting each of these would pin it:

  • only the annotation is patched;
  • the patch goes to a copy;
  • the resourceVersion is carried back;
  • in-memory status survives.

  The deletion skip and Connect's key-error path aren't covered with an enabled hasher.

• Smaller.

  • Hash is exported and nil-unsafe.
  • The README should show the snippet per ProviderConfig flavor; namespace is rejected on gitlab.m v1beta2.
  • The deprecation warning doesn't mention the new field.

Checked and correct

• Codegen is in sync, and both scopes are registered.
• It merges cleanly with master; build, tests and lint pass.
• The webhook never leaks.
• The key lookup respects tenancy.
• The enums match GitLab.
• The project-level fixes are right.
• The codecov/project failure is unrelated: master uploads have been failing for a long time.

Thanks again for all the work here. Happy to help or review the follow-up!

[henrysachs]

Hi @BoxBoxJason, heads-up: master moved to client-go v3 (#419), so I merged master into your branch and switched the eight Mattermost files to the v3 import (2f09714). Only the import path changed, so your commits are untouched and no force-push was needed. CI is green again. The review points above are still yours to decide, of course. Thanks again!

[BoxBoxJason]

Hi @BoxBoxJason, heads-up: master moved to client-go v3 (#419), so I merged master into your branch and switched the eight Mattermost files to the v3 import (2f09714). Only the import path changed, so your commits are untouched and no force-push was needed. CI is green again. The review points above are still yours to decide, of course. Thanks again!

Hey @henrysachs thanks for this very nice review, as always 😁

I started working on implementing everything last week but I didn't finish, I will finish all of that thursday.

Thanks for the upgrade, I won't forget to sync my local branch before continuing 👍

[henrysachs]

Hi @BoxBoxJason, thanks for the update! Almost everything from the review is in, and it looks great. One thing came in with bd2fa8c that needs another look.

Before merging: five newly compared fields never converge

bd2fa8c compares notifyOnlyBrokenPipelines, notifyOnlyDefaultBranch, branchesToBeNotified, labelsToBeNotified and labelsToBeNotifiedBehavior, reading them from the top level of GroupMattermostIntegration (client-go v3.17.0).

I checked against a real GitLab (19.4.2-ee). After a PUT with all five set, GET /groups/:id/integrations/mattermost returns null for all of them at the top level. The values are under properties:

top level:  notify_only_broken_pipelines: null, branches_to_be_notified: null,
            labels_to_be_notified: null, labels_to_be_notified_behavior: null,
            notify_only_default_branch: null
properties: notify_only_broken_pipelines: false, branches_to_be_notified: "all",
            labels_to_be_notified: "foo", labels_to_be_notified_behavior: "match_all"

notify_only_default_branch isn't returned anywhere, neither at the top level nor in properties.

So once any of these fields is set in the spec, the resource is never up to date and gets updated on every poll. The unit tests don't catch it because the mocks put the values at the top level.

Suggestions:

• Read the four fields from properties, e.g. via GetGroupMattermostSettings, whose MattermostIntegrationProperties decodes them there, or by decoding the response yourself.
• Send notifyOnlyDefaultBranch but don't compare it, or drop it, since branchesToBeNotified covers the same thing.
• Add a test that uses the real response shape, with the values under properties.

Smaller things

• The PR description still describes the HMAC secret tracking and the old note about the four excluded fields.
• webhookSecretRef is now optional and only enforced at runtime. A CEL rule on the parameters, such as has(self.webhookSecretRef) || (has(self.useInheritedSettings) && self.useInheritedSettings), would reject a missing ref at apply time.

Checked and correct

• Inheritance: use_inherited_settings is always sent, compared against inherited, and other comparisons are skipped while inheriting. I verified the behavior live with a subgroup: without the parameter, GitLab keeps inheriting and ignores the submitted attributes. With false, it detaches. The late-init to true is a nice touch.
• alertEvents / alertChannel are removed, the empty-webhook error is in, the EE notes and MinLength=1 are there, and so are the 17.9+ and unsupported-options docs.
• 32 "Differs" cases, one per compared field, plus controller tests for inherited, late-init and deletion.
• The secret tracking is cleanly moved out: ProviderConfig, secret_hash.go and the README are back to master.
• Merges cleanly with master. Build, tests and go generate are clean.

Thanks again!

[BoxBoxJason]

Alright @henrysachs I fixed the problems you mentionned + some additional observation issues I found + some missing fields I had (added in client v3).

Live tested against my gitlab instance, everything is still functionnal.

For the follow up about secret change detection, I will test your (easier) suggestion out once this gets merged 😁. This should indeed be way simpler if we can use native kubernetes behaviour for that.

[henrysachs]

LGTM, thanks!

[henrysachs]

Thanks a lot @BoxBoxJason, this is great work! The properties decoder and the wire test with the real response shape are exactly right. I checked the new client live against GitLab 19.4.2-ee: the resource is up to date after an apply, and drift on branchesToBeNotified and notifyOnlyBrokenPipelines is detected.

Merging now. To keep the history accurate, here's what's in and what isn't, since the description still mentions the secret hashing:

• Not in this PR: the opt-in change detection for write-only secrets (spec.secretHashKeySecretRef on the ProviderConfigs, the keyed HMAC-SHA256 annotation, common.SecretHasher). It was removed in 5e4d298 and is planned as a follow-up. Until then, a changed webhookSecretRef value is only applied when another setting of the integration changes, as the field docs say.
• In this PR: the group IntegrationMattermost resource for both scopes, inheritance handling (useInheritedSettings), drift detection for the notification fields read from properties, and the two small fixes to the project-level IntegrationMattermost types.

Looking forward to the secret-tracking follow-up. Happy to review it!