Add StateMachineVersion resource - #104
bjorn-stange-expel wants to merge 5 commits into
Conversation
ack-prow
Bot
added
the
do-not-merge/work-in-progress
ack-prow
Bot
added
the
needs-ok-to-test
|
Hi @bjorn-stange-expel. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
bjorn-stange-expel
changed the title
ack-prow
Bot
removed
the
do-not-merge/work-in-progress
|
/test all |
ack-prow
Bot
added
the
do-not-merge/work-in-progress
2d0963e to
a85d727
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bjorn-stange-expel The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Add StateMachineVersion CRD for managing immutable point-in-time snapshots of state machines via PublishStateMachineVersion. Uses custom hooks for find (DescribeStateMachine with the version ARN) and update (terminal error since versions are immutable). Supports a stateMachineRef K8s reference. Regenerated against code-generator v0.59.1 (commit 2970ca9) and controller-gen v0.19.0 to match current upstream main, and moved generator config into the root generator.yaml per review feedback on aws-controllers-k8s#103. The e2e create test now asserts the resource reaches ACK.ResourceSynced=True. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebased onto upstream main (runtime/code-generator v0.60.0). The regenerated references.go now resolves the StateMachine reference via the cross-namespace helper introduced in code-generator v0.60.0, consistent with the other resources on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a85d727 to
9f841fc
Compare
ack-prow
Bot
removed
the
do-not-merge/work-in-progress
There is no DescribeStateMachineVersion API, so the controller reads versions via DescribeStateMachine with the version ARN through a custom hook. Cover both branches: the successful read that syncs the CR, and the StateMachineDoesNotExist-to-NotFound mapping that lets a CR be deleted cleanly after the version is removed out-of-band. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
michaelhtm
left a comment
michaelhtm
left a comment
There was a problem hiding this comment.
Hey @bjorn-stange-expel
the changes lgtm. One question I have is, Would it make sense to manage StateMachineVersions using the StateMachine resource?
Maybe using this Publish field?
/test all
|
@bjorn-stange-expel: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@shabbskagalwala Do we have any thoughts on using the existing Publish field to manage state machine versions? I put together an alternate implementation that doesn't create a new CRD #122 |
@gustavodiaz7722 thank you! I think this should work and your explanation on the PR #122 helps explain this choice as well |
Adds `spec.publish` so a StateMachine can cut immutable versions. `CreateStateMachineInput.Publish` and `CreateStateMachineOutput.StateMachineVersionArn` were both in the ignore list, so there was no way to opt into versioning and no way to learn a version's ARN once one existed.
| Field | Purpose |
|---|---|
| `spec.publish` | Cut a version whenever configuration is pushed to AWS |
| `status.stateMachineVersionARN` | The version holding the configuration last pushed |
| `status.revisionID` | Which revision is currently live (from `DescribeStateMachine`) |
### Behaviour
Publishing is a parameter of Create/Update rather than a separate call, so a version is cut whenever the update payload differs from the current revision. `PublishStateMachineVersion` is idempotent per revision, so repeat reconciles of an unchanged spec cut nothing.
`publish` is excluded from the generated comparison: it modifies a request, it is not state of the state machine, so its value changing is not by itself a reason to call Update.
That leaves one case the comparison cannot see — turning `publish` on for an already-settled state machine produces no spec difference. `status.stateMachineVersionARN` carries that signal, since a nil ARN under `publish: true` means no version holds the configuration last pushed. The read path clears the ARN when the version it names is gone, and `customPreCompare` turns a nil ARN into a `Spec.Publish` delta, registered under a `Spec` path because the runtime only calls Update when `delta.DifferentAt("Spec")` holds. One condition covers both a state machine that has never published and one whose version was deleted out of band. The probe is confined to `clearDeletedVersionARN`, clears only on `StateMachineDoesNotExist`, and is skipped when `publish` is false.
### Why a field rather than a StateMachineVersion resource
#104 proposes the resource shape, and it is worth discussing these side by side rather than one in place of the other. Two reasons I went this way:
- **Publishing stays atomic with the content.** The version is cut by the same `UpdateStateMachine` call that sets the definition, so it cannot hold a different revision. A separate CR publishes whatever revision is current when it reconciles, and ACK gives no ordering guarantee between independent CRs.
- **No manifest accumulation.** Versions are immutable, so a per-version CR means a new, uneditable object per release, with the count tracking the 1000-version cap.
CloudFormation's `AWS::StepFunctions::StateMachineVersion` avoids both, but only because every property is `Update requires: Replacement` — one template block, replaced each deploy, which both sequences the publish and cleans up the old version. ACK has no replacement primitive, so that mechanism does not port.
### Known gap
No retention. Versions accumulate and AWS caps them at 1000 with manual deletion, so this wants a follow-up such as `keepLastVersions` on the parent. Flagging rather than hiding it.
### Testing
E2E in `test/e2e/tests/test_state_machine.py` covers publish at create, publish absent, a definition change cutting exactly one further version, enabling `publish` on an already-settled state machine, and republishing after a version is deleted outside ACK. Each case then forces a reconcile and asserts nothing further is published, since the failure mode of a delta that never settles is publishing on every pass. The nudge patches `spec.tags`: ACK filters events with `GenerationChangedPredicate` so an annotation is never delivered, and a tags-only delta short-circuits before `UpdateStateMachine` so it cannot publish by itself.
Confirmed against the live API: publish is idempotent per revision; an update with an identical payload and `publish: true` does publish when no version exists for the current revision; a missing version returns `StateMachineDoesNotExist`; version numbers are never reused after deletion.
`versionDescription` is left out deliberately — it participates in the revision fingerprint but is unreadable from the state machine, so it needs its own comparison story.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
|
@shabbskagalwala We've merged release 1.7.0 that supports new Publish field #123 |
5 participants
Addresses aws-controllers-k8s/community#2797
Adds a StateMachineVersion resource for publishing immutable point-in-time snapshots of state machines.
PublishStateMachineVersionDescribeStateMachinewith the version ARN (no dedicated describe API exists)is_immutable(also enforced via CRDx-kubernetes-validations)DeleteStateMachineVersionstateMachineRefresolves the parent StateMachine ARNOnly
hooks.gois hand-written; everything else is generated with code-generatorv0.60.0(branch is rebased on main after #115). Follows the Lambda controller's Version pattern.Test plan
go build ./...passestest_state_machine_version.py— 3/3 passed (create/delete, custom find + out-of-band delete, immutable-update-rejected)🤖 Generated with Claude Code