Skip to content

openspec: add change for serverless project audit logging - #1067

Open
dimuon wants to merge 5 commits into
elastic:masterfrom
dimuon:add-serverless-project-audit-logging
Open

dimuon wants to merge 5 commits into
elastic:masterfrom
dimuon:add-serverless-project-audit-logging

Conversation

@dimuon

@dimuon dimuon commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Spec-only OpenSpec change for #1065: support audit log configuration on the serverless project resources (ec_elasticsearch_project, ec_observability_project, ec_security_project) via an optional monitoring.logging.audit block.

This PR adds only openspec/changes/add-serverless-project-audit-logging/ (proposal, delta spec, design, tasks). No provider code, docs, or changelog changes.

Key decisions

  • monitoring, logging, and audit are optional; destination is required only when audit is set. enabled defaults to true.
  • ignore_filter_ids is a set of strings, at most 10 (the API maxItems). Null and an empty set both mean no filters; read preserves whichever form the configuration uses.
  • destination.status is computed inside destination and never sent on write (LoggingDestinationCreateRequest has additionalProperties: false).
  • Clearing audit sends "audit":null (never "monitoring":null); clearing filters sends "ignore_filters":null. Both go through Patch*ProjectWithBodyWithResponse because generated *T + omitempty fields cannot encode JSON null. Global nullable-type is not enabled.
  • One Read serves create, update, and refresh. Audit configured outside Terraform is adopted on refresh and then planned for removal; a dropped write surfaces as an inconsistent result after apply.

Implementation gate

The vendored public serverless bundle (public-user-serverless-api-dereferenced.yml) still strips monitoring.logging.* via x-exclude-from-documentation, and so does upstream main. Task 1.1 stops the implementation loop until the public bundle contains monitoring.logging.audit. The raw production user spec was used to author the contract; nothing was vendored in this PR.

Validation

  • openspec validate add-serverless-project-audit-logging --type change --strict passes.
  • Eight rounds of adversarial review; the final round returned approve.

Out of scope

Hosted ec_deployment, a VectorDB project resource, the ignore-filter catalog (list/get only), and log categories other than audit.

Closes nothing yet; tracks #1065.

Spec-only OpenSpec change adding an optional monitoring.logging.audit
block to ec_elasticsearch_project, ec_observability_project, and
ec_security_project.

- ignore_filter_ids is a set (max 10); null and empty both mean no filters
- destination.status is computed, never sent on write
- clearing audit or filters sends JSON null via Patch*ProjectWithBodyWithResponse
- implementation is blocked until the public serverless API bundle
  contains monitoring.logging.audit (task 1.1 stops otherwise)

No provider code changes; no changelog entry for a spec-only PR.
@dimuon
dimuon requested a review from a team as a code owner September 28, 2026 17:55
Copilot AI lite review requested due to automatic review settings September 28, 2026 17:55
@dimuon
dimuon requested a review from tobio September 28, 2026 17:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate review comments must be addressed before approval.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)
What changed in this PR

Spec-only OpenSpec change defining audit logging configuration for three serverless project resources.

Changes:

  • Adds the monitoring.logging.audit schema and lifecycle requirements.
  • Documents API mapping, filtering, null-clearing, and validation behavior.
  • Adds implementation, testing, and documentation tasks.
File Summary
openspec/​changes/​add-serverless-project-audit-logging/​tasks.md Lists implementation and validation work; clarification is needed for the enabled schema default.
openspec/​changes/​add-serverless-project-audit-logging/​specs/​project-audit-logging/​spec.md Defines the audit-logging contract and scenarios.
openspec/​changes/​add-serverless-project-audit-logging/​proposal.md Describes scope and impact; requires corrections for audit retention and compatibility claims.
openspec/​changes/​add-serverless-project-audit-logging/​design.md Documents implementation decisions; the nested destination.project_id requirement must be clarified.
openspec/​changes/​add-serverless-project-audit-logging/​.openspec.yaml Declares OpenSpec change metadata.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread openspec/changes/add-serverless-project-audit-logging/design.md Outdated
Comment thread openspec/changes/add-serverless-project-audit-logging/tasks.md Outdated
Comment thread openspec/changes/add-serverless-project-audit-logging/proposal.md Outdated
Copilot AI lite review requested due to automatic review settings September 30, 2026 08:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Update the task wording to reference the full monitoring.logging.audit path.

Review effort: Lite
Findings: None

Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify audit logging removal condition uses monitoring.logging.audit

openspec/​changes/​add-serverless-project-audit-logging/​tasks.md:18

This task says externally configured audit logging is removed unless configuration includes monitoring, but the specified behavior is conditional on monitoring.logging.audit: an empty monitoring/logging shell still has audit unset and the removal path sends "audit":null (spec.md:202-217). Please use the full attribute path here so the implementation and changelog do not promise that a shell preserves audit logging.

Copilot AI lite review requested due to automatic review settings September 30, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Moderate task gaps remain around the generated default behavior and coverage for all three resources.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Correct migration guidance for omitted audit configuration

openspec/​changes/​add-serverless-project-audit-logging/​tasks.md:18

This changelog instruction says externally configured audit logging is preserved whenever monitoring is present, but the proposed behavior clears audit whenever monitoring.logging.audit is omitted—even for monitoring = {} or monitoring = { logging = {} } (spec.md:84-88 and 210-217). Change the condition to the audit block so the implementation PR does not document the wrong migration behavior.

…curity

project-api rejects an audit destination without project_type (403
"Unable to determine project type") and accepts only observability and
security projects as logging destinations, even though the API schema
marks project_type optional with the full ProjectType enum. Make the
attribute required with a two-value validator so both fail at plan
instead of apply, and drop the "send only when known" clauses and the
unknown/omitted project_type scenarios that no longer apply.
Copilot AI lite review requested due to automatic review settings September 30, 2026 09:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Narrow changelog exception to monitoring.logging.audit configuration

openspec/​changes/​add-serverless-project-audit-logging/​tasks.md:18

This changelog instruction is too broad: a configured monitoring shell with no audit does not preserve audit logging. The read requirements say an API audit object is adopted even when the plan has monitoring set with audit null, after which Terraform still plans the audit removal. State the exception as configuration containing monitoring.logging.audit, not merely monitoring, so the implementation changelog does not promise the wrong behavior.

@tobio tobio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible inconsistent-result error on apply. If the API ever adds filters server-side while the plan has ignore_filter_ids null, apply would fail with an inconsistent result

I'm not sure it's reasonable to expect server injected filters, but this got flagged. Otherwise LGTM.

@dimuon

dimuon commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks Toby! Good catch, and it's deliberate: Read stores whatever ignore_filters the API returns, so server-added filters during an apply would surface as an inconsistent result rather than being silently absorbed. Same rule as for the audit object and linked today.

I agree there's no reason to expect that case. project-controller only writes destination.status or nulls audit when the destination is hard-deleted; nothing server-side touches ignore_filters. Going optional+computed would also change what null means for users (from "no filters" to "whatever the API has"), so I'd rather not do it speculatively. If acceptance testing ever trips on it, that's our evidence and the spec tweak is small.

Copilot AI lite review requested due to automatic review settings October 1, 2026 08:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

@dimuon
dimuon enabled auto-merge (squash) October 1, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants