Repository navigation
feat(instance): add securityGroupRefs - #406
ack-prow[bot] merged 3 commits into
Conversation
SecurityGroupIds on Instance now carries a reference to SecurityGroup (Status.ID) the way VpcEndpoint does, so an Instance can point at a SecurityGroup CR by name instead of waiting for its ID. Generated with code-generator v0.67.0: spec field, resolver, deepcopy and CRD. The e2e launches an Instance that references a SecurityGroup CR and checks the group ID on both sides. Signed-off-by: Youssef Omar Bouden <[email protected]>
ack-prow
Bot
added
the
needs-ok-to-test
|
Hi @blacksoxx. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
gustavodiaz7722
left a comment
gustavodiaz7722
left a comment
There was a problem hiding this comment.
Thanks, the reference works as intended. I ran a controller built from this branch (rebased onto main, code-generator and runtime v0.68.0) against real EC2. The Instance launched with the referenced group, adding and removing a ref updated the instance's groups through ModifyInstanceAttribute, and setting both securityGroupIDs and securityGroupRefs was rejected.
Three things before merge:
- The e2e assertion on
spec.securityGroupIDsfails (inline comment). The runtime clears resolved reference IDs before it patches the spec, so withsecurityGroupRefsset,spec.securityGroupIDsis never persisted. That is what aws-controllers-k8s/community#3132 asks for. In my run the same check raisedKeyError: 'securityGroupIDs'.test_references.pyalready asserts this convention for the route table'sgatewayID. - The branch conflicts with #408 in
apis/v1alpha1/ack-generate-metadata.yaml. Please rebase onto main and rerunmake build-controllerwith code-generator v0.68.0. Regenerating this change on top of main with v0.68.0 gave the same generated files as this PR, so only the metadata file should change. - Tide uses the PR description as the squash commit message. After the fixes, please update what it says the e2e checks and the code-generator version.
| assert k8s.wait_on_condition(ref, "ACK.ReferencesResolved", "True", wait_periods=5) | ||
|
|
||
| cr = k8s.get_resource(ref) | ||
| assert cr["spec"]["securityGroupIDs"] == [sg_id] |
There was a problem hiding this comment.
With securityGroupRefs set, spec.securityGroupIDs is never written back (the runtime runs ClearResolvedReferences before the spec patch), so this line raises KeyError. Suggest asserting that the ref is kept and the IDs are not:
| assert cr["spec"]["securityGroupIDs"] == [sg_id] | |
| assert "securityGroupIDs" not in cr["spec"] | |
| assert cr["spec"]["securityGroupRefs"][0]["from"]["name"] == sg_name |
|
/ok-to-test |
ack-prow
Bot
added
ok-to-test
…roup-refs # Conflicts: # apis/v1alpha1/ack-generate-metadata.yaml
…isted The runtime clears resolved reference IDs before it patches the spec, so with securityGroupRefs set spec.securityGroupIDs never lands in the CR; the e2e now checks the ref stays and the IDs field is absent. Regenerated with code-generator v0.68.0 on top of main: only the metadata changed. Signed-off-by: Youssef Omar Bouden <[email protected]>
|
/LGTM |
|
/label release/minor |
ack-prow
Bot
added
the
release/minor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blacksoxx, gustavodiaz7722 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Releasing changes: * #406
Issue #, if available: aws-controllers-k8s/community#3132
Description of changes:
InstancehadsecurityGroupIDsandsecurityGroupsbut no way to point at aSecurityGroupCR, so a group managed by ACK had to be created first and its ID copied into the Instance manifest.SecurityGroupIdsnow carries a reference toSecurityGroup(Status.ID), the wayVpcEndpointalready does, which generatesspec.securityGroupRefswith an optional namespace per entry, the resolver and the "either IDs or refs" validation.Generated with code-generator v0.68.0 and controller-gen v0.19.0: the spec field, references, deepcopy and the CRD (config and helm copies).
go build ./...andgo test ./pkg/...pass. The new e2etest_security_group_refcreates aSecurityGroupCR in the shared VPC, launches an Instance that references it by name, and checks that the instance's groups in EC2 hold the group's ID while the CR keeps the reference and nosecurityGroupIDsis persisted, since the runtime clears resolved IDs before the spec patch. Not run locally.networkInterfaces[].groupsis left as is, as the issue allows.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.