Conversation
k1ranpk
added a commit
to k1ranpk/kafka-controller
that referenced
this pull request
Jun 15, 2026
Adding the EC2 SecurityGroup reference to the shared BrokerNodeGroupInfo type surfaced a securityGroupRefs field on the ServerlessCluster CRD, but ServerlessCluster had no resolver for it, so the field was silently ignored (asymmetric with the functional clientSubnetRefs). Add the matching reference to ServerlessCluster's Provisioned.BrokerNodeGroupInfo.SecurityGroups (preserving its existing late_initialize) and regenerate, producing resolveReferenceForProvisioned_BrokerNodeGroupInfo_SecurityGroups. Addresses reviewer MUST FIX on PR aws-controllers-k8s#104.
Member
|
Should we also add e2e test for serverless cluster? |
Adds an EC2 SecurityGroup reference for Cluster.BrokerNodeGroupInfo.SecurityGroups, generating a securityGroupRefs field that resolves to Status.ID, mirroring the existing clientSubnetRefs support. Resolves aws-controllers-k8s/community#2752
Adds a negative test asserting that an unresolved securityGroupRefs reference blocks MSK Cluster creation (no AWS cost), and a positive test that creates a Cluster referencing an ACK-managed ec2 SecurityGroup and verifies the resolved security group is attached. The positive test is skipped when the ec2 SecurityGroup CRD is not installed. Also ignores test/e2e/.venv/. Resolves aws-controllers-k8s/community#2752
The positive test (creates a live MSK Cluster referencing an ACK-managed ec2 SecurityGroup and verifies attachment) has not been verified end-to-end. Skip it so an unverified test does not run in CI, and leave a TODO to re-enable after a successful run. The no-cost negative test (unresolved ref blocks create) remains active. Re: aws-controllers-k8s/community#2752
Adding the EC2 SecurityGroup reference to the shared BrokerNodeGroupInfo type surfaced a securityGroupRefs field on the ServerlessCluster CRD, but ServerlessCluster had no resolver for it, so the field was silently ignored (asymmetric with the functional clientSubnetRefs). Add the matching reference to ServerlessCluster's Provisioned.BrokerNodeGroupInfo.SecurityGroups (preserving its existing late_initialize) and regenerate, producing resolveReferenceForProvisioned_BrokerNodeGroupInfo_SecurityGroups. Addresses reviewer MUST FIX on PR aws-controllers-k8s#104.
Adds a negative test asserting that an unresolved securityGroupRefs reference on a provisioned ServerlessCluster (ClusterV2) blocks creation (no AWS cost), and a positive test that creates a provisioned cluster referencing an ACK-managed ec2 SecurityGroup and verifies the resolved security group is attached under Provisioned.BrokerNodeGroupInfo. SecurityGroups. The positive test is skipped pending end-to-end verification against live MSK, mirroring the Cluster references suite. Addresses reviewer comment on PR aws-controllers-k8s#104 requesting e2e coverage for the ServerlessCluster securityGroupRefs resolver. Re: aws-controllers-k8s/community#2752
Rebasing onto upstream/main pulled in the ACK runtime/code-generator v0.60.0 bump (aws-controllers-k8s#108). Regenerate the kafka controller with code-generator v0.60.0 so the generated reference resolvers and ack-generate-metadata checksums are consistent with the merged generator.yaml (which adds the BrokerNodeGroupInfo.SecurityGroups reference for both Cluster and ServerlessCluster).
k1ranpk
force-pushed
the
feat/add-securitygroup-refs-to-cluster
branch
from
June 24, 2026 21:23
b830dce to
034e95a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: k1ranpk 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 |
Remove the @pytest.mark.skip decorators and the _require_ec2_securitygroup_crd runtime skip guard from the positive securityGroupRefs e2e tests for both Cluster and ServerlessCluster. The positive tests now always run as canary tests instead of being skipped. These tests require the ec2-controller (SecurityGroup CRD) to be installed in the test cluster via cluster.configuration.additional_controllers. If it is absent the SecurityGroup fixture now fails loudly rather than silently skipping, so the missing dependency is visible. Re: aws-controllers-k8s/community#2752
|
@k1ranpk: 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. |
michaelhtm
reviewed
Jun 25, 2026
Member
There was a problem hiding this comment.
we can't test cross controller references in e2e. the test infra only runs the local controller
Member
There was a problem hiding this comment.
we may encounter issues like these for nested references: aws-controllers-k8s/community#1898
Contributor
There was a problem hiding this comment.
@gustavodiaz7722 I believe this issue should addressed generally once runtime#267 and code-generator#738 have been merged and released to all service controllers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Adds support for adopting/referencing ACK-managed EC2
SecurityGroupresources from the MSKClusterresource, resolving aws-controllers-k8s/community#2752.Code generation
generator.yaml: adds a reference forBrokerNodeGroupInfo.SecurityGroupsto the ec2SecurityGroupresource (path: Status.ID), mirroring the existingclientSubnetRefssupport for subnets.make build-controller: API types + deepcopy, CRDs (config/crd,helm/crds), RBAC (addssecuritygroupsget/list), andpkg/resource/cluster/references.go.This produces a
securityGroupRefsfield onCluster.spec.brokerNodeGroupInfothat resolves to the referenced SecurityGroup'sStatus.ID. If the reference can't be resolved, the controller blocks creation (standard ACK reference behavior).Tests
test/e2e/tests/test_cluster_references.py:securityGroupRefsreference blocks Cluster creation. No MSK cluster is ever created, so it is fast and incurs no AWS cost.SecurityGroupand verify the resolved security group is attached inBrokerNodeGroupInfo.SecurityGroups. This test has not yet been verified end-to-end against a live MSK cluster, so it is skipped via@pytest.mark.skipto avoid landing an unverified test that runs in CI. ATODO(community#2752)documents how to re-enable it: install the ec2-controller in the test cluster (addec2-controller@<version>tocluster.configuration.additional_controllersintest_config.yaml), run it successfully, then remove the skip.New test resource templates:
cluster_security_group_ref.yaml,security_group.yaml.Verification
go build ./cmd/controllerpasses.check-breaking-changesclean (additive field only).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.