Add streamResourcePolicy - #176
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: purkhusid 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 |
ack-prow
Bot
added
the
needs-ok-to-test
|
Hi @purkhusid. 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 Regular contributors should join the org to skip this step. 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-sigs/prow repository. |
## What? Fixes aws-controllers-k8s/community#2948 ## How? Add a new `streamResourcePolicy` attribute to the Table CRD. I would have preferred adding the property to the `streamSpecification` but I couldn't figure out how to add a custom nested field.
f487729 to
3eb4a86
Compare
|
I'm running this patch in our clusters and it is working as expected. Would greatly appreciate a review. |
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
Thanks @purkhusid. Left a few comments.
| // Stream policy is desired but the stream ARN isn't available yet; | ||
| // requeue until DynamoDB Streams is enabled and the stream exists. |
There was a problem hiding this comment.
Q: If the user hasn't enabled a stream for the table will this cause the table to perpetually requeue? Wondering if we should validate that spec.streamSpecificationEnabled is true when StreamResourcePolicy is not nil.
| operation: PutResourcePolicy | ||
| path: Policy | ||
| compare: | ||
| is_ignored: true |
There was a problem hiding this comment.
We've actually added an is_iam_policy config to instruct the code-generator to inject delta comparison logic similar to the compareResourcePolicyDocument hook. I believe you should be able to use that config here and for ResourcePolicy and remove the custom comparison hooks.
| is_ignored: true | |
| is_iam_policy: true |
| streamResourcePolicyDeferred := false | ||
| if delta.DifferentAt("Spec.StreamResourcePolicy") { | ||
| if latest.ko.Status.LatestStreamARN != nil && *latest.ko.Status.LatestStreamARN != "" { | ||
| if err = rm.syncStreamResourcePolicy(ctx, desired, latest); err != nil { |
There was a problem hiding this comment.
Q: When disabling the stream for an existing table should we defer removal of the stream's resource policy until after the stream has been disabled? If we remove the resource policy before disabling the stream and then the controller fails to disable to stream we'd leave the resource in an exposed state.
| # PutResourcePolicy against the stream ARN (Status.LatestStreamARN). | ||
| # There is no native input field for a stream policy, so this is a | ||
| # custom field synced entirely through hooks. | ||
| StreamResourcePolicy: |
There was a problem hiding this comment.
It looks like the from config currently only supports top level fields. However, I believe the below could be used to inject the field into the `Spec.StreamSpecification object.
StreamSpecification.ResourcePolicy:
type: "*string"
set:
- ignore: "all"
There was a problem hiding this comment.
The above would no longer inject the description from the API model for PutResourcePolicy, but we could use documentation.yaml to add a description for the field. You can find an example of that file in the rds-controller here.
2 participants
What?
Fixes aws-controllers-k8s/community#2948
How?
Add a new
streamResourcePolicyattribute to the Table CRD.I would have preferred adding the property to the
streamSpecificationbut I couldn't figure out how to add a custom nested field.