Audit-event recording: align CRUD actions to one pattern

Ouverte
  • Quelque chose ne va pas ?
#5,246 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub

Les mainteneurs répondent en général sous 3 jours

Personne n'a encore pris cette issue.

Évaluation

Difficulté
5/5
Temps estimé
Plus d'une semaine
Accessibilité débutants
30/100
Type d'issue
Refactorisation
Clarté
À clarifier
Activité
Calme
Stack technique
ruby
Domaine
backend, databases

Piste de recherche

Commencez par lire les prochaines étapes suggérées et les classes référencées sous app/actions/, en particulier les points d’appel de transactions et d’événements d’audit répertoriés dans les tableaux. Sélectionnez un changement de pattern indépendant plutôt que de traiter le catalogue comme un seul patch, puis localisez les specs correspondantes, y compris les assertions sur l’ordre des appels. C’est terminé lorsque les points d’appel sélectionnés suivent le pattern convenu et que leurs tests concernés passent.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Description

This document was generated with AI assistance and may not be 100% accurate. It is intended as a starting point for discussion: whether the call sites that drift from the dominant pattern should be aligned, and whether the resources currently without audit events should gain them.

CRUD actions in cloud_controller_ng record audit events with inconsistent ordering, transaction scope, or not at all. Today, a failure between mutate and record can leave a row mutated with no audit trail. The dominant pattern in app/actions/ already addresses this:

  1. Open a db.transaction do ... end block.
  2. Mutate the model (create / save / destroy).
  3. Record the audit event inside the same transaction, after the mutation.
  4. Run any post-commit side effects (notify Diego, broker calls, blobstore writes, ...) outside the transaction.

This issue catalogs every action-layer (and selected job/lib) audit-event call site that drifts from that pattern, plus actions with no audit event at all. V2 paths (app/controllers/runtime/* and app/actions/v2/*) are out of scope - they are being retired.

Inside transaction, mutate → record (dominant - reference pattern)

26 compliant call sites (click to expand)
Resource Action Reference
App create AppCreate#create
App update AppUpdate#update
Build create BuildCreate#create
Buildpack create BuildpackCreate#create
Buildpack update BuildpackUpdate#update
Deployment create DeploymentCreate#create - records via private record_audit_event in two distinct code paths (stopped-app early-exit and the regular path); both inside the transaction
Droplet copy DropletCopy#copy
Organization update OrganizationUpdate#update
Organization delete OrganizationDelete#delete
OrganizationQuota create OrganizationQuotasCreate#create
OrganizationQuota update OrganizationQuotasUpdate#update
Package create PackageCreate#create (via private record_audit_event)
Process create ProcessCreate#create
Process update ProcessUpdate#update
Revision create RevisionCreate#create (via private record_audit_event)
RoutePolicy create RoutePolicyCreate#create
RoutePolicy update RoutePolicyUpdate#update
RoutePolicy delete RoutePolicyDestroy#delete
ServiceBroker create V3::ServiceBrokerCreate#create
Space create SpaceCreate#create
Space update SpaceUpdate#update and SpaceUpdateIsolationSegment#update
Space delete SpaceDelete#delete
SpaceQuota create SpaceQuotasCreate#create
SpaceQuota update SpaceQuotaUpdate#update
Task create TaskCreate#create

Inside transaction, record → mutate

Audit event written before the mutation. The Sequel transaction rolls back both on failure, so observable behaviour today is equivalent to mutate → record. Re-ordering protects against regressions if either step grows side effects in a future refactor.

Resource Action Reference
App delete AppDelete#delete
Buildpack delete BuildpackDelete#delete
OrganizationQuota delete OrganizationQuotaDelete#delete
Process delete ProcessDelete#delete
Role delete RoleDelete#delete (via private record_event)
Route delete (underlying helper) VCAP::CloudController::RouteDelete#do_delete and #delete_unmapped_route in app/actions/routing/route_delete.rb - records record_route_delete_request before the deletion job runs
SpaceQuota delete SpaceQuotaDelete#delete
Stack delete StackDelete#delete

Outside transaction, mutate → record

A failure between the mutation and Event.create leaves the row mutated with no audit trail. Fix by wrapping the mutate-and-record pair in a db.transaction do ... end block (keep any external side effects - broker HTTP calls, blobstore writes, Diego notifies - outside). The nine groups below expand to ~17 individual call sites once DropletCreate (3), RoleCreate (8) and the two route-delete actions are counted.

Resource Action Reference
Droplet create DropletCreate#create, DropletCreate#create_docker_droplet, DropletCreate#find_or_create_buildpack_droplet - three call sites, all record after the transaction closes
Droplet delete DropletDelete#delete
Organization create OrganizationCreate#create
Package delete PackageDelete#delete
Role create RoleCreate - eight private create_* methods each call record_*_role_add then create the row, with no surrounding transaction
Route create RouteCreate#create
Route delete RouteDeleteAction#delete (V3 action in app/actions/route_delete.rb)
Stack create StackCreate#create
Task delete TaskDelete#delete - records record_task_cancel; cancellation is the only way to delete a task in V3
Special case: async + external side effects

The following actions record audit events outside any DB transaction. Unlike the entries above, a naive transaction wrap is not safe because at least one external side effect - a service-broker HTTP call, a Diego notification, or both - runs between the row write and the audit. Wrapping the side effect inside a DB transaction is incorrect: long-held transactions block other writers, and rolling back can't undo work already performed externally.

Resource Action Reference
ServiceInstance create / update / delete V3::ServiceInstanceCreateManaged, V3::ServiceInstanceUpdateManaged, V3::ServiceInstanceDelete - record_service_instance_event is called outside the transaction(s) that wrap the broker-state writes; broker HTTP calls in between
ServiceBinding (V3) create V3::ServiceBindingCreate#complete_binding_and_save (called from ServiceCredentialBindingAppCreate, ServiceCredentialBindingKeyCreate, ServiceRouteBindingCreate). The row write at save_with_attributes_and_new_operation happens after the broker call has returned, but post_bind_action (e.g. notify_diego for route bindings) sits between the row write and record_create
ServiceCredentialBinding delete ServiceCredentialBindingDelete (extends V3::ServiceBindingDelete) - record_delete and record_start_delete outside any transaction; the broker unbind call happens before perform_delete_actions runs
ServiceRouteBinding delete ServiceRouteBindingDelete#perform_delete_actions - record_delete before binding.destroy, then binding.notify_diego. Diego notification is an external side effect between the row write and post-record state, so the same constraint applies

This family needs its own design pass - a small transaction around the post-side-effect state write + audit, with the side effect clearly outside. Out of scope for the rest of this issue.

Audit events recorded outside app/actions/

Audit calls in jobs and lib/. These bypass the action-layer pattern entirely; listed for completeness, no action proposed.

Where Reference Notes
app/jobs/v3/buildpack_bits.rb record_buildpack_upload Async upload follow-up
lib/cloud_controller/diego/staging_completion_handler.rb record_build_failed, record_build_staged Worker-driven build state transitions

No audit event at all

Actions that mutate persistent state but never call any record_* method. Split by recommended response.

Add an audit event

Resources with no event repository today.

Resource Actions Notes
Domain DomainCreate, DomainUpdate, DomainDelete No domain_event_repository.rb exists. Every other top-level CC resource is audited. Shared domains are platform-wide (no owning org); private domains belong to an org. A new repository would need to decide which space/org an event row belongs to for each flavour, and which roles can read it
SecurityGroup SecurityGroupCreate, SecurityGroupUpdate, SecurityGroupDelete No event repository. Security group mutations directly affect egress networking
IsolationSegment IsolationSegmentCreate, IsolationSegmentUpdate, IsolationSegmentDelete Admin-only, infrequent; candidate for the same reason as Domain
Candidate for existing event repository
Resource Actions Notes
Sidecar SidecarCreate, SidecarUpdate, SidecarDelete Sidecars are subordinate to apps. AppEventRepository already records subordinate events like record_map_route, record_unmap_route, record_app_map_droplet; sidecar events would fit the same pattern
RouteMapping (destination) RouteDestinationUpdate#update Updates the protocol field on a route destination. AppEventRepository already records record_map_route / record_unmap_route for the same association - destination protocol updates would fit alongside
Organization (default isolation segment) SetDefaultIsolationSegment#set Mutates Organization.default_isolation_segment_guid. OrganizationEventRepository.record_organization_update already covers org updates; this could route through that
ServicePlanVisibility V3::ServicePlanVisibilityUpdate, ServicePlanVisibilityDelete Service-plan visibility changes are auditable via ServiceEventRepository.record_service_plan_visibility_event and similar (already used for the legacy controller path). Wire V3 actions through that repo
Needs design discussion
Resource Actions Notes
User UserCreate, UserUpdate, UserDelete user_event_repository.rb exists but only records role add/remove. UserCreate is dual-purpose: it can shadow-sync a UAA user OR register a guid-only stub during permission lookups. The latter is high-frequency and auditing it would flood the events table. Needs a "what counts as user-initiated" rule before adding
ServiceOffering / ServicePlan ServiceOfferingDelete, ServicePlanDelete Admin-initiated catalog changes. Design alongside any future broader catalog-audit work
Intentionally unaudited - leave as-is

Internal state-machine transitions driven by workers, or pure value updates with no security relevance.

Resource Actions Notes
AppFeature AppFeatureUpdate Feature-flag toggles; covered by app update audit
Build BuildUpdate, BuildDelete Build state transitions (STAGING → STAGED / FAILED) are recorded by the worker via record_build_staged / record_build_failed in staging_completion_handler.rb. Build create is audited (user-initiated)
Deployment DeploymentUpdate, DeploymentDelete Worker-driven state transitions. Deployment create is audited
Droplet DropletUpdate Worker-driven state transition. Droplet create and delete are audited
EnvironmentVariableGroup EnvironmentVariableGroupUpdate Affects new app pushes only; no per-app audit value
FeatureFlag FeatureFlagUpdate Global toggle; admin-only and rarely changed
Revision RevisionDelete Revisions are immutable artefacts; deletion is cleanup
Route RouteUpdate V3 path updates route.options (TCP/HTTP routing config) and metadata. Map/unmap surfaces are audited separately via record_map_route / record_unmap_route. Auditing RouteUpdate itself would be a small gain - promote to "high priority" only if the options-change story matters to operators

Suggested next steps

Independent pieces of work; each is its own PR.

  1. Reorder record → mutate in the eight entries listed under "Inside transaction, record → mutate". Pure cleanup, no behaviour change. Specs that assert call order via receive(...).ordered will need adjustment.
  2. Wrap mutate + record in a transaction for the nine groups under "Outside transaction, mutate → record" (~17 call sites; not the "Special case: async + external side effects" section). Keep all external side effects (notify_diego, broker calls, blobstore writes) outside the transaction. A spec asserting the audit row is absent when the model save raises would catch regressions.
  3. Scope out missing audit events - open separate issues per resource:
    • 3a. Sidecar (smallest; likely just extends AppEventRepository)
    • 3b. Domain (needs new event repository and a decision on which space/org an event belongs to, given shared vs. private domains have different ownership)
    • 3c. SecurityGroup (needs new event repository)
    • 3d. IsolationSegment (needs new event repository)
    • 3e. ServicePlanVisibility (route through existing ServiceEventRepository)
    • 3f. RouteDestinationUpdate, SetDefaultIsolationSegment (route through existing parent repos)
Langage dominant
Ruby
Étoiles
207
Forks
373
Merge moyen
2 j 18 h
PR mergées (30 j)
82

Préparer son environnement

Ouvrir dans Codespaces

Lance le conteneur de développement du projet dans votre navigateur, avec votre propre compte GitHub.

Par où commencer

  1. Lisez l'issue en entier, puis le guide de contribution du projet.
  2. Signalez en commentaire que vous la prenez — cela évite que deux personnes fassent le même travail.
  3. Forkez le dépôt et travaillez sur une branche.
  4. Ouvrez une pull request qui référence le numéro de l'issue.

Autres issues de cloudfoundry/cloud_controller_ng

Toutes les issues de cloudfoundry/cloud_controller_ng

Issues similaires

Plus d'issues Ruby

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.