Commit 4714bcf
authored
feat: add field-level custom_sync generator config (#732)
## Description
Some fields are managed by an AWS API separate from the resource's `Update` operation — tags via `CreateOrUpdateTags`, a logging configuration via its own `Put` call. Controllers reconcile these today by hand-writing an `sdk_update_pre_build_request` hook that invokes a sync function and then short-circuits out of `sdkUpdate` when nothing else differs.
Every controller doing this duplicates the same block. Worse, adding a second such field means widening the existing `DifferentExcept` call by hand, and forgetting to makes the resource silently stop applying legitimate updates, with no error surfaced anywhere.
This PR adds `custom_sync` as a field-level config that generates the boilerplate.
```yaml
resources:
AutoScalingGroup:
fields:
Tags:
custom_sync: {}
```
The sync function itself stays hand-written. It's a method on `resourceManager`, matching every other seam where generated code calls user-supplied code (`custom_implementation`, `set_output_custom_method_name`, and so on):
```go
func (rm *resourceManager) syncTags(
ctx context.Context, desired *resource, latest *resource,
) error
```
The method name is always `sync<FieldName>` and is not configurable, so it's identical across every controller and can be found from the field name alone. `CustomSyncConfig` is an empty struct rather than a bool, so options can be added later without breaking the `generator.yaml` files that adopt `custom_sync: {}` now — promoting a bool to a struct would break under the strict unmarshalling `config.New` performs.
## Generated code
Into `sdkUpdate`:
```go
updatedDesired := desired.DeepCopy()
updatedDesired.SetStatus(latest)
if delta.DifferentAt("Spec.Tags") {
err = rm.syncTags(ctx, desired, latest)
if err != nil {
return nil, err
}
}
if !delta.DifferentExcept("Spec.Tags") {
return rm.concreteResource(updatedDesired), nil
}
```
Into `sdkCreate`:
```go
if ko.Spec.Tags != nil {
msg := "Secondary sync required; resource will be requeued"
ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, &msg, nil)
}
```
A `custom_sync` field is only ever applied in the update path, so immediately after create it exists in Spec but has not been pushed to AWS. Marking the resource unsynced makes the runtime requeue after `requeue.DefaultRequeueAfterDuration` rather than waiting for the full resync period, and the condition message tells the user a further sync is coming without their involvement.
The message is a generic constant rather than a list of the pending field paths. The nil checks guarding it are evaluated at runtime, but a generated message can only be assembled from every configured field, so naming them would over-report whenever a user populates only a subset.
With multiple `custom_sync` fields, the create-path guard collapses into one block, since `SetSynced` overwrites rather than accumulates:
```go
if ko.Spec.LogDeliveryConfigurations != nil || ko.Spec.Tags != nil {
msg := "Secondary sync required; resource will be requeued"
ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, &msg, nil)
}
```
On the update side, all paths land in a single `DifferentExcept` call:
```go
if !delta.DifferentExcept("Spec.LogDeliveryConfigurations", "Spec.Tags") {
```
That auto-widening is the main argument for generating this rather than documenting a snippet.
## Placement
Position in `sdk_update.go.tpl` is load-bearing in both directions, and is recorded in a comment on `CustomSyncUpdate`:
- **After** the `updateable.when` guard, so a resource in a state where mutations aren't allowed is requeued before any out-of-band API call is made, and so the short-circuit can't return success past the guard.
- **Before** the Update operation's `custom_implementation`, which returns from `sdkUpdate` directly and would otherwise skip the sync entirely.
## Validation
Rejected at generation time, since each otherwise produces a controller that compiles but misbehaves at runtime:
| Config | Why it's rejected |
| --- | --- |
| `custom_sync` on a nested field | Emitted code builds a `Spec.<Field>` delta path and a nil check off `ko.Spec` |
| `custom_sync` with `is_read_only` | Field lands in Status, where there's no desired value to sync toward |
| `custom_sync` with `compare.is_ignored` | Field never enters the delta, so the sync never runs and the short-circuit fires every reconcile |
| `custom_sync` with no Update operation | Generated code lives in `sdkUpdate`, so the sync would never be called |
## Testing
`make test` passes — 15 packages, no failures.
New unit tests cover the emitters (single field, two fields, and the no-config inert case) and the model accessors plus all validation errors. Fixtures are added under `pkg/testdata/models/apis/elasticache/0000-00-00/`.
Verified end to end by regenerating `autoscaling-controller`, which has exactly this pattern hand-written today:
- With no `custom_sync` configured, generated `sdk.go` is **byte-identical** to before, confirming the emitters are inert for resources that don't use the feature.
- With `custom_sync` on `Tags` and both hand-written hook templates deleted, the generated code matches what those hooks produced. The controller builds and its tests pass.
- Placement relative to `updateable.when` was confirmed by temporarily adding a guard to `AutoScalingGroup` and inspecting the generated output.
## Notes for reviewers
- Ordering across multiple `custom_sync` fields is alphabetical, which keeps generated output stable. Config-driven ordering is deferred until a resource needs it; note that YAML document order isn't recoverable here, since `ResourceConfig.Fields` is a map and the loader routes through `sigs.k8s.io/yaml`. An explicit `order:` key would be the way to add it.
- `custom_sync` is restricted to top-level Spec fields. Beyond the codegen reasons, `DifferentExcept` matches with `Path.Contains`, so allowing nested paths would let one diff match two except-paths and skew the count.
- No test asserts template placement, since the emitters are unit-tested in isolation. Happy to add a rendered-output assertion if you'd like that ordering pinned.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.1 parent 65d45b2 commit 4714bcf
14 files changed
Lines changed: 731 additions & 0 deletions
File tree
- pkg
- config
- generate
- ack
- code
- model
- testdata/models/apis/elasticache/0000-00-00
- templates/pkg/resource
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
275 | 275 | | |
276 | 276 | | |
277 | 277 | | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
278 | 322 | | |
279 | 323 | | |
280 | 324 | | |
| |||
459 | 503 | | |
460 | 504 | | |
461 | 505 | | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
462 | 511 | | |
463 | 512 | | |
464 | 513 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
164 | 164 | | |
165 | 165 | | |
166 | 166 | | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
167 | 173 | | |
168 | 174 | | |
169 | 175 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
0 commit comments