diff --git a/CLAUDE.md b/CLAUDE.md index e9d7899..f01ac17 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,12 +20,15 @@ described there without flagging it to me first. UI-only logic. CLI (`sentryctl`) and Terraform provider are first-class, not afterthoughts. **Status**: `sentryctl` has been built out phase by phase since Phase 3. The Terraform provider (`/terraform`) only exists - as of this note -- one resource (`sentry_dashboard`), built on - HashiCorp's `terraform-plugin-framework`, reusing the exact same REST - contract `sentryctl dashboards apply` and web's dashboard export - already use. Alert rules, notification targets, and tenant/RBAC - resources are real, disclosed future work -- see `/terraform/README.md` - for the full accounting of what is and isn't built, and the same + as of this note -- two resources (`sentry_dashboard`, full CRUD; + `sentry_alert_rule`, create/destroy only -- `alerting` has no + `PUT /rules/{id}` to update against), built on HashiCorp's + `terraform-plugin-framework`, reusing the exact same REST contracts + `sentryctl dashboards apply`/web's dashboard export and + `sentryctl alerts apply` already use. Notification targets and + tenant/RBAC resources are real, disclosed future work -- see + `/terraform/README.md` for the full accounting of what is and isn't + built, and the same "written but not run against a live stack" verification caveat as everything else Docker-gated in this repo. diff --git a/terraform/README.md b/terraform/README.md index c773e81..990a0d7 100644 --- a/terraform/README.md +++ b/terraform/README.md @@ -3,25 +3,30 @@ Sentry's Terraform provider -- `CLAUDE.md`'s "Repo conventions" section names this a first-class deliverable alongside `sentryctl` ("CLI and Terraform provider are first-class, not afterthoughts"), but -no phase before this one had actually built any of it. This is a first -slice, not a finished provider: one resource (`sentry_dashboard`), built -on [HashiCorp's `terraform-plugin-framework`][framework] (the actively- -developed library, not the legacy SDKv2 -- there's no existing provider -code here to migrate, so there's no reason to start on the framework -HashiCorp itself steers new providers away from). +no phase before this one had actually built any of it. Two resources so +far (`sentry_dashboard`, `sentry_alert_rule`), not a finished provider, +built on [HashiCorp's `terraform-plugin-framework`][framework] (the +actively-developed library, not the legacy SDKv2 -- there's no existing +provider code here to migrate, so there's no reason to start on the +framework HashiCorp itself steers new providers away from). [framework]: https://developer.hashicorp.com/terraform/plugin/framework -## Why `sentry_dashboard` first +## Why these two resources first -`cli/README.md` already frames the underlying REST contract this way: +`cli/README.md` already frames the dashboards REST contract this way: `POST /dashboards`, `GET`/`PUT`/`DELETE /dashboards/{id}` are "the seed of a future Terraform provider: one JSON contract, multiple callers (web export, CLI apply, eventually a provider)." This provider is that third caller -- `internal/provider/client.go` talks the exact same JSON shape `sentryctl dashboards apply` and the web UI's Export JSON button already use against `api/dashboards.Handler`, not a new contract invented for -Terraform's sake. +Terraform's sake. `sentry_alert_rule` follows against `alerting`'s own +`POST`/`GET`/`DELETE /rules[/{id}]` -- the natural second resource, and +a real second service (`alerting` is a genuinely separate deployment +from `api`, its own base URL), so building it second exercised that this +provider can talk to more than one Sentry service, not just repeat the +dashboards pattern against the same endpoint. ## What's built @@ -62,19 +67,57 @@ first pass deliberately, not an oversight. A `sentry_dashboard_panel` resource (or a panels list block on this one -- an open design question, not yet decided) is real, disclosed future work. +```hcl +provider "sentry" { + endpoint = "http://localhost:8080" # or $SENTRY_API_ENDPOINT + alerting_endpoint = "http://localhost:8081" # or $SENTRY_ALERTING_API_ENDPOINT -- alerting is a separate service, own base URL + token = var.sentry_api_token # or $SENTRY_API_TOKEN -- shared by both services, same as sentryctl's one $SENTRYCTL_TOKEN +} + +resource "sentry_alert_rule" "example" { + name = "Checkout 5xx spike" + query = "service=checkout status>=500 | stats count" + condition_type = "threshold" + comparator = "gt" + threshold_value = 50 + eval_interval_seconds = 60 + notification_target_id = "target-abc123" # must already exist -- see "Also not built" below +} +``` + +Supports `terraform import sentry_alert_rule.example `. + +**`sentry_alert_rule` is create/destroy only, not update-in-place.** +`alerting`'s REST API has no `PUT /rules/{id}` at all -- confirmed down +to `rulestore.Store`, which has `Create`/`List`/`Get`/`Delete` but no +`Update` method to even wire one to, a real, pre-existing gap in +`alerting`'s own API, not something specific to Terraform. Every +attribute on this resource carries a `RequiresReplace` plan modifier, so +changing anything (the query, the threshold, even just `description`) +destroys and recreates the rule rather than updating it -- which also +resets `alert_state`/delivery-log continuity for that rule, a real +operational side effect worth knowing about before relying on this in +a pipeline that changes rules often. Faking an in-place update via +delete-then-recreate inside the resource itself was considered and +rejected for the same reason: it would hide that side effect instead of +surfacing it in the plan output the way `RequiresReplace` does. Adding a +real `PUT /rules/{id}` to `alerting` would remove this constraint, but +is a change to a different module's REST API, out of scope for this +pass. + **Also not built, all real and disclosed, not attempted here:** -- Alert rules / notification targets (`/alerting`'s REST surface -- - `POST /rules`, etc.) -- a second provider "family" of resources, no - code shared with dashboards beyond this same client-pattern - discipline. +- Notification targets (`/alerting`'s `POST`/`GET`/`DELETE /targets`) -- + a `sentry_alert_rule`'s `notification_target_id` has to name a target + created some other way (`sentryctl`, `curl`, the web UI) until this + exists. - Tenant/RBAC resources (`enterprise-auth`'s tenant/membership/grant surface) -- meaningfully different auth model (offline operator flags today, not a stable REST API a provider could safely drive idempotently -- see `/enterprise/README.md`'s "Bootstrapping a tenant" section) and Phase 4 commercial licensing, so this would need its own design pass, not just "add another resource file." -- A `sentry_dashboard` data source (read-only lookup by ID/name) -- - straightforward given the resource already exists, just not built +- Data sources (read-only lookup by ID/name) for either resource -- + straightforward given the resources already exist, just not built yet. - Publishing to the real Terraform Registry -- `main.go`'s `Address` (`registry.terraform.io/sentry/sentry`) is the address a real @@ -92,29 +135,39 @@ go test ./... `internal/provider/client_test.go` runs real HTTP round trips against a `httptest.Server` (same pattern `cli/cmd/sentryctl`'s own tests use -against the same `api/dashboards` endpoints) -- real request +against the same `api/dashboards`/`alerting` endpoints) -- real request construction (method, path, `Authorization` header, JSON body), real response parsing, including the 404-vs-other-error distinction `Read`/`Delete` need to implement Terraform's "resource deleted -out-of-band" convention correctly. -`internal/provider/provider_test.go` validates the provider and +out-of-band" convention correctly, and (for rules) proving the +`GET /rules/{id}` response's promoted `RuleWithState` fields plus an +extra `"state"` key this client deliberately has no field for still +parse cleanly. +`internal/provider/provider_test.go` validates the provider and both resource schemas are internally well-formed (attribute names, the `Required`/`Computed` split) without needing a Terraform binary or a -live `api` service at all. +live `api`/`alerting` service at all. `internal/provider/dashboard_resource_test.go`'s -`TestAccDashboardResource_basic` is a real acceptance test using +`TestAccDashboardResource_basic` and +`internal/provider/alert_rule_resource_test.go`'s +`TestAccAlertRuleResource_basic` are real acceptance tests using [`terraform-plugin-testing`][testing] -- skipped unless `TF_ACC=1` is set, that framework's own standard convention, the same shape every other live-infrastructure test in this repo uses (`docker`-gated env -vars for Postgres/ClickHouse tests elsewhere). Even with `TF_ACC=1` it -also needs a real running `api` service (Postgres + ClickHouse) to apply -against, which this environment has no Docker access to bring up -- -**not run here**, same disclosed gap as every other live-infra test -across this repo (see `/docs/phase-4-runbook.md`'s "Verification -status" section for the project-wide version of this same caveat). "The -test exists and is correct Go" is not the same claim as "this resource -has been applied for real." +vars for Postgres/ClickHouse tests elsewhere). The alert rule test also +uses a `plancheck.ExpectResourceAction` assertion proving a config +change actually plans a destroy-then-create, not an in-place update -- +the concrete, checked version of the "create/destroy only" design +decision documented above, not just a claim in a comment. Even with +`TF_ACC=1` both tests also need real running `api`/`alerting` services +(Postgres + ClickHouse) to apply against, which this environment has no +Docker access to bring up -- **not run here**, same disclosed gap as +every other live-infra test across this repo (see +`/docs/phase-4-runbook.md`'s "Verification status" section for the +project-wide version of this same caveat). "The test exists and is +correct Go" is not the same claim as "this resource has been applied +for real." [testing]: https://developer.hashicorp.com/terraform/plugin/testing diff --git a/terraform/examples/resources/sentry_alert_rule/import.sh b/terraform/examples/resources/sentry_alert_rule/import.sh new file mode 100644 index 0000000..25175c0 --- /dev/null +++ b/terraform/examples/resources/sentry_alert_rule/import.sh @@ -0,0 +1 @@ +terraform import sentry_alert_rule.checkout_5xx diff --git a/terraform/examples/resources/sentry_alert_rule/resource.tf b/terraform/examples/resources/sentry_alert_rule/resource.tf new file mode 100644 index 0000000..8f4e85c --- /dev/null +++ b/terraform/examples/resources/sentry_alert_rule/resource.tf @@ -0,0 +1,19 @@ +# notification_target_id has to name an already-existing target -- +# no sentry_notification_target resource exists yet (see the provider +# README), so create one via sentryctl/curl/the web UI first and pass +# its id in here. +resource "sentry_alert_rule" "checkout_5xx" { + name = "Checkout 5xx spike" + query = "service=checkout status>=500 | stats count" + condition_type = "threshold" + comparator = "gt" + threshold_value = 50 + eval_interval_seconds = 60 + for_minutes = 5 + notification_target_id = "target-abc123" +} + +# Create/destroy only -- alerting has no PUT /rules/{id} today, so +# changing any attribute above (including this resource block itself) +# destroys and recreates the rule rather than updating it in place. See +# the provider README for why. diff --git a/terraform/internal/provider/alert_rule_resource.go b/terraform/internal/provider/alert_rule_resource.go new file mode 100644 index 0000000..bfaa40d --- /dev/null +++ b/terraform/internal/provider/alert_rule_resource.go @@ -0,0 +1,315 @@ +package provider + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/path" + "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/float64planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" + "github.com/hashicorp/terraform-plugin-framework/types" +) + +var ( + _ resource.Resource = &alertRuleResource{} + _ resource.ResourceWithConfigure = &alertRuleResource{} + _ resource.ResourceWithImportState = &alertRuleResource{} +) + +func newAlertRuleResource() resource.Resource { + return &alertRuleResource{} +} + +// alertRuleResource implements sentry_alert_rule against +// alerting/internal/httpapi's POST/GET/DELETE /rules[/{id}] endpoints. +// +// Deliberately create/destroy only, every attribute RequiresReplace: +// alerting has no PUT /rules/{id} at all -- confirmed down to +// rulestore.Store, which has Create/List/Get/Delete but no Update +// method to even wire one to, a real pre-existing gap in alerting's own +// API. Faking an in-place update via delete-then-recreate inside this +// resource was considered and rejected -- it would silently reset +// alert_state/delivery-log continuity a real operator might care about, +// a behavioral side effect this resource shouldn't paper over. See the +// provider README for the full reasoning and the option (adding a real +// PUT /rules/{id} to alerting) that would remove this constraint. +type alertRuleResource struct { + client *client +} + +type alertRuleResourceModel struct { + ID types.String `tfsdk:"id"` + TenantID types.String `tfsdk:"tenant_id"` + Name types.String `tfsdk:"name"` + Description types.String `tfsdk:"description"` + Query types.String `tfsdk:"query"` + QueryLanguage types.String `tfsdk:"query_language"` + ConditionType types.String `tfsdk:"condition_type"` + Comparator types.String `tfsdk:"comparator"` + ThresholdValue types.Float64 `tfsdk:"threshold_value"` + EvalIntervalSeconds types.Int64 `tfsdk:"eval_interval_seconds"` + ForMinutes types.Int64 `tfsdk:"for_minutes"` + RenotifyIntervalMinutes types.Int64 `tfsdk:"renotify_interval_minutes"` + NotificationTargetID types.String `tfsdk:"notification_target_id"` + Enabled types.Bool `tfsdk:"enabled"` + CreatedBy types.String `tfsdk:"created_by"` +} + +func (r *alertRuleResource) Metadata(_ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_alert_rule" +} + +func (r *alertRuleResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { + replace := []planmodifier.String{stringplanmodifier.RequiresReplace()} + resp.Schema = schema.Schema{ + Description: "A Sentry alert rule. Create/destroy only -- alerting has no update endpoint for " + + "rules today (see this resource's Go doc comment), so every attribute below forces a " + + "destroy-and-recreate on change, never an in-place update.", + Attributes: map[string]schema.Attribute{ + "id": schema.StringAttribute{ + Computed: true, + PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()}, + Description: "Server-generated rule ID.", + }, + "tenant_id": schema.StringAttribute{ + Computed: true, + PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()}, + }, + "name": schema.StringAttribute{ + Required: true, + PlanModifiers: replace, + Description: "Rule name. The API rejects an empty string.", + }, + "description": schema.StringAttribute{ + Optional: true, + Computed: true, + Default: stringdefault.StaticString(""), + PlanModifiers: replace, + }, + "query": schema.StringAttribute{ + Required: true, + PlanModifiers: replace, + Description: "Pipe-syntax or SQL query text (see query_language) -- the same query the evaluator re-runs on every eval_interval_seconds tick. The API rejects an empty string.", + }, + "query_language": schema.StringAttribute{ + Optional: true, + Computed: true, + Default: stringdefault.StaticString(""), + PlanModifiers: replace, + Description: `"" (auto-detect), "sql", or "spl" -- same values the query API itself accepts.`, + }, + "condition_type": schema.StringAttribute{ + Required: true, + PlanModifiers: replace, + Description: `"threshold" (requires comparator + threshold_value) or "absence" (fires when the query returns zero rows).`, + }, + "comparator": schema.StringAttribute{ + Optional: true, + PlanModifiers: replace, + Description: `Required (and only meaningful) when condition_type = "threshold": one of "gt", "gte", "lt", "lte", "eq", "ne".`, + }, + "threshold_value": schema.Float64Attribute{ + Optional: true, + PlanModifiers: []planmodifier.Float64{float64planmodifier.RequiresReplace()}, + Description: `Required (and only meaningful) when condition_type = "threshold".`, + }, + "eval_interval_seconds": schema.Int64Attribute{ + Required: true, + PlanModifiers: []planmodifier.Int64{int64planmodifier.RequiresReplace()}, + Description: "How often the evaluator re-runs this rule's query. The API rejects anything below 30.", + }, + "for_minutes": schema.Int64Attribute{ + Optional: true, + Computed: true, + Default: int64default.StaticInt64(0), + PlanModifiers: []planmodifier.Int64{int64planmodifier.RequiresReplace()}, + Description: "How long the condition must stay true before the rule transitions from pending to firing. 0 means fire immediately on the first true evaluation.", + }, + "renotify_interval_minutes": schema.Int64Attribute{ + Optional: true, + PlanModifiers: []planmodifier.Int64{int64planmodifier.RequiresReplace()}, + Description: "How often to re-send a notification while still firing. Unset means notify once per firing transition only.", + }, + "notification_target_id": schema.StringAttribute{ + Required: true, + PlanModifiers: replace, + Description: "ID of a sentry_notification_target-managed (or manually created) notification target. The API rejects an empty string. No sentry_notification_target resource exists yet -- see the provider README -- so this has to be a target created some other way (sentryctl, curl, or the web UI) for now.", + }, + "enabled": schema.BoolAttribute{ + Optional: true, + Computed: true, + Default: booldefault.StaticBool(true), + PlanModifiers: []planmodifier.Bool{boolplanmodifier.RequiresReplace()}, + Description: "Whether the evaluator considers this rule at all. Defaults to true, matching POST /rules' own default when the field is omitted.", + }, + "created_by": schema.StringAttribute{ + Computed: true, + PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()}, + }, + }, + } +} + +func (r *alertRuleResource) Configure(_ context.Context, req resource.ConfigureRequest, resp *resource.ConfigureResponse) { + if req.ProviderData == nil { + return + } + data, ok := req.ProviderData.(*providerData) + if !ok { + resp.Diagnostics.AddError( + "Unexpected Resource Configure Type", + fmt.Sprintf("Expected *provider.providerData, got: %T. This is a provider bug -- please report it.", req.ProviderData), + ) + return + } + r.client = data.alerting +} + +func alertRuleModelFromAPI(rl *rule) alertRuleResourceModel { + m := alertRuleResourceModel{ + ID: types.StringValue(rl.ID), + TenantID: types.StringValue(rl.TenantID), + Name: types.StringValue(rl.Name), + Description: types.StringValue(rl.Description), + Query: types.StringValue(rl.Query), + QueryLanguage: types.StringValue(rl.QueryLanguage), + ConditionType: types.StringValue(rl.ConditionType), + EvalIntervalSeconds: types.Int64Value(int64(rl.EvalIntervalSeconds)), + ForMinutes: types.Int64Value(int64(rl.ForMinutes)), + NotificationTargetID: types.StringValue(rl.NotificationTargetID), + CreatedBy: types.StringValue(rl.CreatedBy), + } + if rl.Comparator != nil { + m.Comparator = types.StringValue(*rl.Comparator) + } + if rl.ThresholdValue != nil { + m.ThresholdValue = types.Float64Value(*rl.ThresholdValue) + } + if rl.RenotifyIntervalMinutes != nil { + m.RenotifyIntervalMinutes = types.Int64Value(int64(*rl.RenotifyIntervalMinutes)) + } + // Enabled always comes back set from the API (Rule.Enabled is a + // plain bool, not a pointer, in rulestore -- see store.go) -- + // unlike Comparator/ThresholdValue/RenotifyIntervalMinutes above, + // this is never legitimately null in a response. + enabled := true + if rl.Enabled != nil { + enabled = *rl.Enabled + } + m.Enabled = types.BoolValue(enabled) + return m +} + +func alertRuleAPIFromModel(m alertRuleResourceModel) *rule { + rl := &rule{ + Name: m.Name.ValueString(), + Description: m.Description.ValueString(), + Query: m.Query.ValueString(), + QueryLanguage: m.QueryLanguage.ValueString(), + ConditionType: m.ConditionType.ValueString(), + EvalIntervalSeconds: int(m.EvalIntervalSeconds.ValueInt64()), + ForMinutes: int(m.ForMinutes.ValueInt64()), + NotificationTargetID: m.NotificationTargetID.ValueString(), + } + if !m.Comparator.IsNull() { + v := m.Comparator.ValueString() + rl.Comparator = &v + } + if !m.ThresholdValue.IsNull() { + v := m.ThresholdValue.ValueFloat64() + rl.ThresholdValue = &v + } + if !m.RenotifyIntervalMinutes.IsNull() { + v := m.RenotifyIntervalMinutes.ValueInt64() + vInt := int(v) + rl.RenotifyIntervalMinutes = &vInt + } + if !m.Enabled.IsNull() { + v := m.Enabled.ValueBool() + rl.Enabled = &v + } + return rl +} + +func (r *alertRuleResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { + var plan alertRuleResourceModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) + if resp.Diagnostics.HasError() { + return + } + + out, err := r.client.createRule(ctx, alertRuleAPIFromModel(plan)) + if err != nil { + resp.Diagnostics.AddError("Creating Alert Rule", err.Error()) + return + } + + resp.Diagnostics.Append(resp.State.Set(ctx, alertRuleModelFromAPI(out))...) +} + +func (r *alertRuleResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) { + var state alertRuleResourceModel + resp.Diagnostics.Append(req.State.Get(ctx, &state)...) + if resp.Diagnostics.HasError() { + return + } + + out, err := r.client.getRule(ctx, state.ID.ValueString()) + if err != nil { + if isNotFound(err) { + resp.State.RemoveResource(ctx) + return + } + resp.Diagnostics.AddError("Reading Alert Rule", err.Error()) + return + } + + resp.Diagnostics.Append(resp.State.Set(ctx, alertRuleModelFromAPI(out))...) +} + +// Update should be unreachable in practice: every non-Computed +// attribute above carries RequiresReplace, so a real config change +// always plans a destroy-and-recreate instead of an in-place update. +// Still required to satisfy resource.Resource's interface -- implemented +// as a safe passthrough (just re-read the current server state into the +// plan's ID) rather than calling any mutating endpoint, since alerting +// has no PUT /rules/{id} to call in the first place. +func (r *alertRuleResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { + var plan alertRuleResourceModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) + if resp.Diagnostics.HasError() { + return + } + + out, err := r.client.getRule(ctx, plan.ID.ValueString()) + if err != nil { + resp.Diagnostics.AddError("Reading Alert Rule", err.Error()) + return + } + + resp.Diagnostics.Append(resp.State.Set(ctx, alertRuleModelFromAPI(out))...) +} + +func (r *alertRuleResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { + var state alertRuleResourceModel + resp.Diagnostics.Append(req.State.Get(ctx, &state)...) + if resp.Diagnostics.HasError() { + return + } + + if err := r.client.deleteRule(ctx, state.ID.ValueString()); err != nil && !isNotFound(err) { + resp.Diagnostics.AddError("Deleting Alert Rule", err.Error()) + } +} + +func (r *alertRuleResource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { + resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp) +} diff --git a/terraform/internal/provider/alert_rule_resource_test.go b/terraform/internal/provider/alert_rule_resource_test.go new file mode 100644 index 0000000..d902cda --- /dev/null +++ b/terraform/internal/provider/alert_rule_resource_test.go @@ -0,0 +1,89 @@ +package provider + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/plancheck" +) + +// Same skip-gated-not-faked posture as TestAccDashboardResource_basic -- +// see that test's doc comment. notification_target_id below is a +// placeholder: no sentry_notification_target resource exists yet (see +// the provider README), so a real run of this test would need a +// pre-existing target id supplied some other way; not a blocker for +// what this test actually proves, since it has never run against a +// live stack in this environment regardless. +func TestAccAlertRuleResource_basic(t *testing.T) { + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: ` +provider "sentry" { + endpoint = "http://localhost:8080" + alerting_endpoint = "http://localhost:8081" +} + +resource "sentry_alert_rule" "test" { + name = "Acceptance Test Rule" + query = "status>=500 | stats count" + condition_type = "threshold" + comparator = "gt" + threshold_value = 5 + eval_interval_seconds = 60 + notification_target_id = "placeholder-target-id" +} +`, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("sentry_alert_rule.test", "name", "Acceptance Test Rule"), + resource.TestCheckResourceAttr("sentry_alert_rule.test", "comparator", "gt"), + resource.TestCheckResourceAttr("sentry_alert_rule.test", "threshold_value", "5"), + resource.TestCheckResourceAttrSet("sentry_alert_rule.test", "id"), + resource.TestCheckResourceAttrSet("sentry_alert_rule.test", "tenant_id"), + // Left unset in config -- must come back as the + // server's own default (true), same "API default, + // not a duplicated Terraform-side one" reasoning + // sentry_dashboard's default_earliest/default_latest + // use. + resource.TestCheckResourceAttr("sentry_alert_rule.test", "enabled", "true"), + resource.TestCheckResourceAttr("sentry_alert_rule.test", "for_minutes", "0"), + ), + }, + { + // Proves the "create/destroy only" design decision is + // real, not just documented: alerting has no PUT + // /rules/{id}, so every attribute is RequiresReplace, + // and changing one (here, the threshold) must plan a + // destroy-then-create, never an in-place update. + Config: ` +provider "sentry" { + endpoint = "http://localhost:8080" + alerting_endpoint = "http://localhost:8081" +} + +resource "sentry_alert_rule" "test" { + name = "Acceptance Test Rule" + query = "status>=500 | stats count" + condition_type = "threshold" + comparator = "gt" + threshold_value = 10 + eval_interval_seconds = 60 + notification_target_id = "placeholder-target-id" +} +`, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("sentry_alert_rule.test", plancheck.ResourceActionDestroyBeforeCreate), + }, + }, + Check: resource.TestCheckResourceAttr("sentry_alert_rule.test", "threshold_value", "10"), + }, + { + ResourceName: "sentry_alert_rule.test", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} diff --git a/terraform/internal/provider/client.go b/terraform/internal/provider/client.go index d486fd3..75f0d01 100644 --- a/terraform/internal/provider/client.go +++ b/terraform/internal/provider/client.go @@ -141,3 +141,62 @@ func (c *client) updateDashboard(ctx context.Context, id string, d *dashboard) ( func (c *client) deleteDashboard(ctx context.Context, id string) error { return c.do(ctx, http.MethodDelete, "/dashboards/"+id, nil, nil) } + +// rule mirrors alerting/internal/rulestore.Rule's JSON shape, plus the +// request-only `enabled` field POST /rules accepts +// (httpapi.createRuleRequest embeds rulestore.Rule and adds this +// pointer specifically so "omitted" (defaults to enabled) and +// "explicitly false" are distinguishable -- see handleCreateRule's doc +// comment) -- deliberately a local type, not an import of either +// package, same "talk HTTP, not Go imports, to a service that isn't +// yours" posture as dashboard above. GET/POST /rules both return this +// shape flattened (no separate "state" wrapper needed here since this +// resource doesn't manage or expose alert_state -- see the provider +// README on why). +type rule struct { + ID string `json:"id,omitempty"` + TenantID string `json:"tenant_id,omitempty"` + Name string `json:"name"` + Description string `json:"description"` + Query string `json:"query"` + QueryLanguage string `json:"query_language"` + ConditionType string `json:"condition_type"` + Comparator *string `json:"comparator,omitempty"` + ThresholdValue *float64 `json:"threshold_value,omitempty"` + EvalIntervalSeconds int `json:"eval_interval_seconds"` + ForMinutes int `json:"for_minutes"` + RenotifyIntervalMinutes *int `json:"renotify_interval_minutes,omitempty"` + NotificationTargetID string `json:"notification_target_id"` + Enabled *bool `json:"enabled,omitempty"` + CreatedBy string `json:"created_by,omitempty"` +} + +// createRule and getRule are the only mutating/reading calls this +// client makes against /rules -- there is deliberately no updateRule: +// alerting/internal/httpapi has no PUT /rules/{id} at all (confirmed +// down to rulestore.Store, which has Create/List/Get/Delete but no +// Update method to even wire one to) -- a real, pre-existing gap in +// alerting's own API, not something this provider works around by +// faking an update via delete+recreate under the hood. alertRuleResource +// models this honestly: every attribute is RequiresReplace, so +// Terraform destroys and recreates on any change rather than pretending +// an in-place update exists. +func (c *client) createRule(ctx context.Context, r *rule) (*rule, error) { + var out rule + if err := c.do(ctx, http.MethodPost, "/rules", r, &out); err != nil { + return nil, err + } + return &out, nil +} + +func (c *client) getRule(ctx context.Context, id string) (*rule, error) { + var out rule + if err := c.do(ctx, http.MethodGet, "/rules/"+id, nil, &out); err != nil { + return nil, err + } + return &out, nil +} + +func (c *client) deleteRule(ctx context.Context, id string) error { + return c.do(ctx, http.MethodDelete, "/rules/"+id, nil, nil) +} diff --git a/terraform/internal/provider/client_test.go b/terraform/internal/provider/client_test.go index 3ad7bb1..95e6edd 100644 --- a/terraform/internal/provider/client_test.go +++ b/terraform/internal/provider/client_test.go @@ -150,3 +150,94 @@ func TestApiErrorSurfacesPlainTextBodyWhenNotJSON(t *testing.T) { t.Fatalf("err = %v, want it to surface the plain-text body", err) } } + +func TestCreateRuleSendsExpectedRequest(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/rules" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + var body rule + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decoding request body: %v", err) + } + if body.Name != "High Error Rate" || body.ConditionType != "threshold" { + t.Errorf("unexpected request body: %+v", body) + } + if body.Comparator == nil || *body.Comparator != "gt" { + t.Errorf("Comparator = %v, want gt", body.Comparator) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + comparator := "gt" + threshold := 5.0 + _ = json.NewEncoder(w).Encode(rule{ + ID: "rule-1", TenantID: "acme", Name: body.Name, + ConditionType: "threshold", Comparator: &comparator, ThresholdValue: &threshold, + EvalIntervalSeconds: 60, NotificationTargetID: "target-1", + }) + })) + defer srv.Close() + + comparator := "gt" + threshold := 5.0 + c := newClient(srv.URL, "") + out, err := c.createRule(context.Background(), &rule{ + Name: "High Error Rate", Query: "status>=500 | stats count", ConditionType: "threshold", + Comparator: &comparator, ThresholdValue: &threshold, + EvalIntervalSeconds: 60, NotificationTargetID: "target-1", + }) + if err != nil { + t.Fatalf("createRule: %v", err) + } + if out.ID != "rule-1" || out.EvalIntervalSeconds != 60 { + t.Fatalf("unexpected response: %+v", out) + } +} + +func TestGetRuleParsesFlattenedRuleWithStateResponse(t *testing.T) { + // alerting/internal/httpapi's GET /rules/{id} returns + // rulestore.RuleWithState -- Rule's fields promoted to the top + // level via anonymous embedding, plus a "state" object this + // client's rule type deliberately has no field for (see client.go's + // doc comment). This test proves that extra "state" key doesn't + // break parsing the fields this provider does care about. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "id": "rule-1", "tenant_id": "acme", "name": "High Error Rate", + "condition_type": "threshold", "comparator": "gt", "threshold_value": 5, + "eval_interval_seconds": 60, "notification_target_id": "target-1", "enabled": true, + "state": {"rule_id": "rule-1", "state": "ok", "last_eval_status": "ok", "consecutive_errors": 0} + }`)) + })) + defer srv.Close() + + c := newClient(srv.URL, "") + out, err := c.getRule(context.Background(), "rule-1") + if err != nil { + t.Fatalf("getRule: %v", err) + } + if out.Name != "High Error Rate" || out.Comparator == nil || *out.Comparator != "gt" { + t.Fatalf("unexpected response: %+v", out) + } +} + +func TestDeleteRuleSendsToCorrectPath(t *testing.T) { + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + if r.Method != http.MethodDelete || r.URL.Path != "/rules/rule-1" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + c := newClient(srv.URL, "") + if err := c.deleteRule(context.Background(), "rule-1"); err != nil { + t.Fatalf("deleteRule: %v", err) + } + if !called { + t.Fatal("expected the server to receive a DELETE request") + } +} diff --git a/terraform/internal/provider/dashboard_resource.go b/terraform/internal/provider/dashboard_resource.go index a487797..6f75197 100644 --- a/terraform/internal/provider/dashboard_resource.go +++ b/terraform/internal/provider/dashboard_resource.go @@ -111,15 +111,15 @@ func (r *dashboardResource) Configure(_ context.Context, req resource.ConfigureR if req.ProviderData == nil { return } - c, ok := req.ProviderData.(*client) + data, ok := req.ProviderData.(*providerData) if !ok { resp.Diagnostics.AddError( "Unexpected Resource Configure Type", - fmt.Sprintf("Expected *provider.client, got: %T. This is a provider bug -- please report it.", req.ProviderData), + fmt.Sprintf("Expected *provider.providerData, got: %T. This is a provider bug -- please report it.", req.ProviderData), ) return } - r.client = c + r.client = data.api } func dashboardModelFromAPI(d *dashboard) dashboardResourceModel { diff --git a/terraform/internal/provider/provider.go b/terraform/internal/provider/provider.go index 27280ac..d985bbd 100644 --- a/terraform/internal/provider/provider.go +++ b/terraform/internal/provider/provider.go @@ -33,8 +33,21 @@ type sentryProvider struct { } type sentryProviderModel struct { - Endpoint types.String `tfsdk:"endpoint"` - Token types.String `tfsdk:"token"` + Endpoint types.String `tfsdk:"endpoint"` + AlertingEndpoint types.String `tfsdk:"alerting_endpoint"` + Token types.String `tfsdk:"token"` +} + +// providerData is what Configure hands resources/data sources via +// req.ProviderData -- two separate clients, not one, because `alerting` +// is a genuinely separate service with its own base URL (its own +// REST API, its own port, sometimes its own deployment) -- same split +// web/src/lib/api.ts's apiBase/alertingBase and cli/cmd/sentryctl's +// --api/--alerting-api already draw, not something invented for this +// provider. +type providerData struct { + api *client + alerting *client } func (p *sentryProvider) Metadata(_ context.Context, _ provider.MetadataRequest, resp *provider.MetadataResponse) { @@ -44,7 +57,7 @@ func (p *sentryProvider) Metadata(_ context.Context, _ provider.MetadataRequest, func (p *sentryProvider) Schema(_ context.Context, _ provider.SchemaRequest, resp *provider.SchemaResponse) { resp.Schema = schema.Schema{ - Description: "Manages Sentry log-aggregation-platform resources. Dashboards only for now -- alert rules, notification targets, and tenant/RBAC resources are real, disclosed future work, not built in this pass; see the provider README.", + Description: "Manages Sentry log-aggregation-platform resources. Dashboards and alert rules for now -- notification targets and tenant/RBAC resources are real, disclosed future work, not built in this pass; see the provider README.", Attributes: map[string]schema.Attribute{ "endpoint": schema.StringAttribute{ Optional: true, @@ -52,6 +65,14 @@ func (p *sentryProvider) Schema(_ context.Context, _ provider.SchemaRequest, res "$SENTRY_API_ENDPOINT, or \"http://localhost:8080\" if that's unset too -- same " + "default sentryctl's --api/$SENTRYCTL_API_URL uses (cli/cmd/sentryctl/main.go).", }, + "alerting_endpoint": schema.StringAttribute{ + Optional: true, + Description: "Base URL of the alerting service, e.g. \"http://localhost:8081\" -- a " + + "separate service from api, not a path under endpoint above (see " + + "/docs/phase-3-alerting-design.md's component boundary). Defaults to " + + "$SENTRY_ALERTING_API_ENDPOINT, or \"http://localhost:8081\" if that's unset too -- " + + "same default sentryctl's --alerting-api/$SENTRYCTL_ALERTING_API_URL uses.", + }, "token": schema.StringAttribute{ Optional: true, Sensitive: true, @@ -84,19 +105,31 @@ func (p *sentryProvider) Configure(ctx context.Context, req provider.ConfigureRe endpoint = "http://localhost:8080" } + alertingEndpoint := config.AlertingEndpoint.ValueString() + if alertingEndpoint == "" { + alertingEndpoint = os.Getenv("SENTRY_ALERTING_API_ENDPOINT") + } + if alertingEndpoint == "" { + alertingEndpoint = "http://localhost:8081" + } + token := config.Token.ValueString() if token == "" { token = os.Getenv("SENTRY_API_TOKEN") } - c := newClient(endpoint, token) - resp.DataSourceData = c - resp.ResourceData = c + data := &providerData{ + api: newClient(endpoint, token), + alerting: newClient(alertingEndpoint, token), + } + resp.DataSourceData = data + resp.ResourceData = data } func (p *sentryProvider) Resources(_ context.Context) []func() resource.Resource { return []func() resource.Resource{ newDashboardResource, + newAlertRuleResource, } } diff --git a/terraform/internal/provider/provider_test.go b/terraform/internal/provider/provider_test.go index 1c13784..0cb97dd 100644 --- a/terraform/internal/provider/provider_test.go +++ b/terraform/internal/provider/provider_test.go @@ -24,7 +24,7 @@ func TestProviderSchemaValid(t *testing.T) { if resp.Diagnostics.HasError() { t.Fatalf("provider schema has errors: %v", resp.Diagnostics) } - for _, attr := range []string{"endpoint", "token"} { + for _, attr := range []string{"endpoint", "alerting_endpoint", "token"} { if _, ok := resp.Schema.Attributes[attr]; !ok { t.Errorf("provider schema missing expected attribute %q", attr) } @@ -64,3 +64,38 @@ func TestDashboardResourceMetadataSetsTypeName(t *testing.T) { t.Fatalf("TypeName = %q, want sentry_dashboard", resp.TypeName) } } + +func TestAlertRuleResourceSchemaValid(t *testing.T) { + ctx := context.Background() + req := resource.SchemaRequest{} + resp := &resource.SchemaResponse{} + + newAlertRuleResource().Schema(ctx, req, resp) + + if resp.Diagnostics.HasError() { + t.Fatalf("sentry_alert_rule schema has errors: %v", resp.Diagnostics) + } + for _, attr := range []string{ + "id", "tenant_id", "name", "description", "query", "query_language", + "condition_type", "comparator", "threshold_value", "eval_interval_seconds", + "for_minutes", "renotify_interval_minutes", "notification_target_id", "enabled", "created_by", + } { + if _, ok := resp.Schema.Attributes[attr]; !ok { + t.Errorf("sentry_alert_rule schema missing expected attribute %q", attr) + } + } + if !resp.Schema.Attributes["name"].IsRequired() { + t.Error(`"name" must be Required`) + } + if !resp.Schema.Attributes["id"].IsComputed() { + t.Error(`"id" must be Computed`) + } +} + +func TestAlertRuleResourceMetadataSetsTypeName(t *testing.T) { + resp := &resource.MetadataResponse{} + newAlertRuleResource().Metadata(context.Background(), resource.MetadataRequest{ProviderTypeName: "sentry"}, resp) + if resp.TypeName != "sentry_alert_rule" { + t.Fatalf("TypeName = %q, want sentry_alert_rule", resp.TypeName) + } +}