Ensure RabbitMQ Broker reaches synced status when using non-default security group #104

knottnt · 2025-09-16T01:16:59Z

Issue #, if available:

Description of changes:

Add .vscode to .gitignore
Omit unchanged field from update payload
Only set spec fields included in delta in sdkUpdate
late initialize fields to load default values after broker creation
Add delta pre-compare hook to match EngineVersion by prefix instead of exact match
Add e2e test validating creation of RabbitMQ broker with non-default security group becomes synced
Add tests for delta comparison of EngineVersion
Update create/delete e2e test to validate update behavior

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2025-09-16T01:17:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ack-prow · 2025-09-16T01:17:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [knottnt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apis/v1alpha1/ack-generate-metadata.yaml

knottnt · 2025-09-16T01:20:37Z

/test all

knottnt · 2025-09-18T19:37:33Z

/test all

- Add .vscode to .gitignore - Enable late_initialize for fields not set by the Create API's response - Set omit_unchanged_fields to true - Set only_set_unchanged_fields to true

- Add custom hook to check that latest EngineVersion has Spec.EngineVersion as prefix instead of checking for exact match.

- Add e2e test to ensure that the ACK resource syncs when creating a RabbitMQ broker with a non-default security group

- Move creation of broker to fixture - Add test logic for validating update of broker

knottnt · 2025-09-24T23:27:03Z

/test mq-kind-e2e

knottnt · 2025-09-25T03:53:02Z

/test all

knottnt · 2025-09-26T16:42:01Z

/hold
for code-gen change to be merged.

michaelhtm · 2025-09-26T17:45:50Z

apis/v1alpha1/generator.yaml

+          - method: Update
+            ignore: true
+      MaintenanceWindowStartTime:
+        late_initialize: {}


should we add a late_initialize_all field at the Resource level in code-gen?

Hmm, that could be useful. Not sure if I want to block this PR on that code-gen feature though.

pkg/resource/broker/delta_test.go

michaelhtm · 2025-09-26T17:58:32Z

apis/v1alpha1/generator.yaml

+      # UpdateBroker API returns nil for values not set in the request.
+      only_set_unchanged_fields: true


should we consider ignoring the update output completely? and do late_initialize only?

Hmm, the update output does have relevant information, but only for the fields that were non-nil in the input shape. What would the benefit of relying on late_initialize only? Also, for late_initialization don't we only set nil fields? The fields in the delta will already be set so the updated values won't be reflected.

I was thinking late_initialize would just patch all the necessary updated fields from AWS.

If the response you get from update output is the same as the update input, i don't think we need to set those fields again..

There are some cases where we AWS does slightly modify the input value in the output that we want to pick-up to avoid errant drift detection. For example if you send a maintenance window day of "Monday" the Update operation will return MONDAY. We could try catching all of those cases in the delta function, but that would require quite a bit of custom code.

that makes sense

- Add test case for delta when engine version differs by patch version

ack-prow · 2025-09-29T19:49:55Z

@knottnt: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
mq-verify-attribution	`40da041`	link	false	`/test mq-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions 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. I understand the commands that are listed here.

knottnt · 2025-09-29T19:50:24Z

/unhold

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2025

ack-prow bot requested review from michaelhtm and rushmash91 September 16, 2025 01:17

ack-prow bot added the approved label Sep 16, 2025

knottnt commented Sep 16, 2025

View reviewed changes

apis/v1alpha1/ack-generate-metadata.yaml Outdated Show resolved Hide resolved

knottnt mentioned this pull request Sep 16, 2025

Add only_set_unchanged_fields config to UpdateOperationConfig aws-controllers-k8s/code-generator#621

Merged

knottnt added 7 commits September 23, 2025 11:51

Avoid sending unnecessary fields in UpdateBroker call

2459edf

- Add .vscode to .gitignore - Enable late_initialize for fields not set by the Create API's response - Set omit_unchanged_fields to true - Set only_set_unchanged_fields to true

Only compare EngineVersion by prefix

e5cc9c7

- Add custom hook to check that latest EngineVersion has Spec.EngineVersion as prefix instead of checking for exact match.

Ensure that Broker is synced in E2E test

e28fbc2

Add e2e test for RabbitMQ with non-default sg

2041f1e

- Add e2e test to ensure that the ACK resource syncs when creating a RabbitMQ broker with a non-default security group

Test engine comparison logic

fac0f5a

Update created/delete e2e test to verify update

a8ed1c8

- Move creation of broker to fixture - Add test logic for validating update of broker

Fix naming of test resource

d42701a

Fix collision in test secret names

402a132

knottnt force-pushed the rabbitmq-security-groups branch from 7dcabd0 to 402a132 Compare September 25, 2025 03:52

knottnt marked this pull request as ready for review September 25, 2025 04:08

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2025

ack-prow bot requested a review from a-hilaly September 25, 2025 04:08

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2025

michaelhtm reviewed Sep 26, 2025

View reviewed changes

Update delta_test.go

73f4c60

- Add test case for delta when engine version differs by patch version

knottnt requested a review from michaelhtm September 26, 2025 20:07

Rebuild new code-gen version

40da041

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2025

		# UpdateBroker API returns nil for values not set in the request.
		only_set_unchanged_fields: true

Ensure RabbitMQ Broker reaches synced status when using non-default security group #104

Are you sure you want to change the base?

Ensure RabbitMQ Broker reaches synced status when using non-default security group #104

Uh oh!

Conversation

knottnt commented Sep 16, 2025

Uh oh!

ack-prow bot commented Sep 16, 2025

Uh oh!

ack-prow bot commented Sep 16, 2025

Uh oh!

Uh oh!

knottnt commented Sep 16, 2025

Uh oh!

knottnt commented Sep 18, 2025

Uh oh!

knottnt commented Sep 24, 2025

Uh oh!

knottnt commented Sep 25, 2025

Uh oh!

knottnt commented Sep 26, 2025

Uh oh!

michaelhtm Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

knottnt Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelhtm Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

knottnt Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

knottnt Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Sep 29, 2025

Uh oh!

knottnt commented Sep 29, 2025

Uh oh!

Uh oh!