Skip to content

Use ServiceID to match VPC Endpoint Service in ReadMany operation #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
*.swp
*~
.idea
.vscode
/docs/site
bin
build
4 changes: 2 additions & 2 deletions apis/v1alpha1/ack-generate-metadata.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
ack_generate_info:
build_date: "2025-07-22T22:13:44Z"
build_date: "2025-07-24T18:21:24Z"
build_hash: b2dc0f44e0b08f041de14c3944a5cc005ba97c8f
go_version: go1.24.5
version: v0.50.0
api_directory_checksum: d162a6e9df2d4861d6c01d42047402b51f341293
api_version: v1alpha1
aws_sdk_go_version: v1.32.6
generator_config_info:
file_checksum: e6a6ff840d55df735215ac04826122ebf89eb79a
file_checksum: c1b1b2a01e8d9f0971cc42f937b92fc9bb0d2987
original_file_name: generator.yaml
last_modification:
reason: API generation
15 changes: 8 additions & 7 deletions apis/v1alpha1/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,6 @@ operations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
CreateVpcEndpointServiceConfiguration:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed duplicate operation objects

output_wrapper_field_path: ServiceConfiguration
DeleteVpcEndpointServiceConfigurations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
CreateFlowLogs:
operation_type:
- Create
Expand Down Expand Up @@ -1068,7 +1062,10 @@ resources:
when:
- path: Status.ServiceState
in:
- available
- Available
Copy link
Author

@knottnt knottnt Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource never reached ACK.ResourceSynced=True due to case mismatch.

list_operation:
match_fields:
- ServiceId
Comment on lines +1066 to +1068
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we declare ServiceID as primaryKey instead? This would ensure it is assigned to DescribeVpcEndpointServiceConfigurationsInput

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ServiceID is already marked as the primary key. Maybe there's an issue with how that field is being handled in code-gen. But I think that might be a better solution since it will be caught in the requiredFieldsMissingFromReadManyInput and skip the API call.

Copy link
Author

@knottnt knottnt Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code-gen checkRequiredFieldsMissingFromShapeReadMany function it seems that fields marked with is_primary_key aren't actually included in the generated check. FindPluralizedIdentifiersInShape uses GetIdentifiers which only doesn't include primary key fields in when searching the target shape. Do you know if there's a reason primary keys aren't included in these checks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see..It seems like we should have renamed the field ID...We may need to add a functionality in code-gen that lets us define which field is the identifier. if it is not defined, the fallback could be GetIdentifiers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the same functionality ended up using a similar pattern to Elastic IP Address to include the check via hooks. The improvement code-gen to handle this automatically would good, but I don't know if I want it to block this PR.

hooks:
sdk_delete_post_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_delete_post_build_request.go.tpl
Expand All @@ -1078,6 +1075,10 @@ resources:
template_path: hooks/vpc_endpoint_service_configuration/sdk_update_pre_build_request.go.tpl
sdk_read_many_post_set_output:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_post_set_output.go.tpl
sdk_read_many_pre_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_pre_build_request.go.tpl
sdk_read_many_post_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_post_build_request.go.tpl
VpcPeeringConnection:
fields:
VpcId:
Expand Down
15 changes: 8 additions & 7 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,6 @@ operations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
CreateVpcEndpointServiceConfiguration:
output_wrapper_field_path: ServiceConfiguration
DeleteVpcEndpointServiceConfigurations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
CreateFlowLogs:
operation_type:
- Create
Expand Down Expand Up @@ -1068,7 +1062,10 @@ resources:
when:
- path: Status.ServiceState
in:
- available
- Available
list_operation:
match_fields:
- ServiceId
hooks:
sdk_delete_post_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_delete_post_build_request.go.tpl
Expand All @@ -1078,6 +1075,10 @@ resources:
template_path: hooks/vpc_endpoint_service_configuration/sdk_update_pre_build_request.go.tpl
sdk_read_many_post_set_output:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_post_set_output.go.tpl
sdk_read_many_pre_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_pre_build_request.go.tpl
sdk_read_many_post_build_request:
template_path: hooks/vpc_endpoint_service_configuration/sdk_read_many_post_build_request.go.tpl
VpcPeeringConnection:
fields:
VpcId:
Expand Down
2 changes: 1 addition & 1 deletion pkg/resource/vpc_endpoint_service_configuration/manager.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions pkg/resource/vpc_endpoint_service_configuration/sdk.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
if r.ko.Status.ServiceID != nil {
input.ServiceIds = []string{*r.ko.Status.ServiceID}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
if r.ko.Status.ServiceID == nil {
return nil, ackerr.NotFound
}
2 changes: 2 additions & 0 deletions test/e2e/bootstrap_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from acktest.bootstrapping.s3 import Bucket
from acktest.bootstrapping.vpc import VPC
from acktest.bootstrapping.vpc import TransitGateway
from acktest.bootstrapping.vpc_endpoint_service import VpcEndpointServiceConfiguration
from e2e import bootstrap_directory

@dataclass
Expand All @@ -28,6 +29,7 @@ class BootstrapResources(Resources):
AdoptedVPC: VPC
NetworkLoadBalancer: NetworkLoadBalancer
TestTransitGateway: TransitGateway
AdoptedVpcEndpointService: VpcEndpointServiceConfiguration

_bootstrap_resources = None

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/requirements.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
acktest @ git+https://github.com/aws-controllers-k8s/test-infra.git@72e9d798ad4f22e0e1ff4e227cfd69f7e301479a
acktest @ git+https://github.com/knottnt/ack-test-infra.git@93adae31a050a11a67567e1e8d3c440b47f4044e
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to replace this once this PR is merged

9 changes: 9 additions & 0 deletions test/e2e/resources/vpc_endpoint_service_adoption.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
apiVersion: ec2.services.k8s.aws/v1alpha1
kind: VPCEndpointServiceConfiguration
metadata:
name: $VPC_ENDPOINT_SERVICE_ADOPTED_NAME
annotations:
services.k8s.aws/adoption-policy: $ADOPTION_POLICY
services.k8s.aws/adoption-fields: "$ADOPTION_FIELDS"
services.k8s.aws/deletion-policy: retain
spec: {}
4 changes: 3 additions & 1 deletion test/e2e/service_bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from acktest.bootstrapping.elbv2 import NetworkLoadBalancer
from acktest.bootstrapping.vpc import VPC
from acktest.bootstrapping.vpc import TransitGateway
from acktest.bootstrapping.vpc_endpoint_service import VpcEndpointServiceConfiguration
from acktest.bootstrapping.s3 import Bucket
from e2e import bootstrap_directory
from e2e.bootstrap_resources import BootstrapResources
Expand All @@ -37,7 +38,8 @@ def service_bootstrap() -> Resources:
),
NetworkLoadBalancer=NetworkLoadBalancer("e2e-vpc-ep-service-test"),
AdoptedVPC=VPC(name_prefix="e2e-adopted-vpc", num_public_subnet=1, num_private_subnet=0),
TestTransitGateway=TransitGateway()
TestTransitGateway=TransitGateway(),
AdoptedVpcEndpointService=VpcEndpointServiceConfiguration(name_prefix="e2e-adopted-vpc-es"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue with bootstrapping this..but should we instead create the resource using k8s?
Something like this: https://github.com/aws-controllers-k8s/kafka-controller/blob/main/test/e2e/tests/test_configuration.py#L66

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit to using k8s vs bootstapping? Is it that we don't have to maintain the python library code for the VpcEndpointServiceConfiguration bootstappable class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's one case. Another one is, I don't see a case where we need to reuse VPCEndpointService in any other controller, so i don't see the need for it being bootstrapped. It's your call though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm leaning towards leaving it as a bootstrappable resource for now since it matches the pattern already being used elsewhere in this controller. It also helps manage the sub-resources needed to create the VPC Endpoint Service.

)

try:
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/tests/test_vpc_endpoint_service_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ def test_vpc_endpoint_service_configuration_create_delete(self, ec2_client, simp
# Check VPC Endpoint Service exists in AWS
ec2_validator = EC2Validator(ec2_client)
ec2_validator.assert_vpc_endpoint_service_configuration(resource_id)
assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=5)


# Check that the allowedPrincipal is properly set
allowed_principals = ec2_validator.get_vpc_endpoint_service_permissions(resource_id)
Expand Down
129 changes: 129 additions & 0 deletions test/e2e/tests/test_vpc_endpoint_service_configuration_adoption.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You may
# not use this file except in compliance with the License. A copy of the
# License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is distributed
# on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
# express or implied. See the License for the specific language governing
# permissions and limitations under the License.

"""Integration tests for the Vpc Endpoint Service Configuraion Adoption.
"""

# Default to us-west-2 since that's where prow is deployed
import logging
from os import environ
import time

import pytest

from e2e import service_marker
from e2e import CRD_GROUP, CRD_VERSION, load_ec2_resource
from e2e.bootstrap_resources import get_bootstrap_resources
from e2e.replacement_values import REPLACEMENT_VALUES
from acktest.resources import random_suffix_name
from acktest.k8s import resource as k8s
from acktest import tags

from e2e.tests.helper import EC2Validator


REGION = "us-west-2" if environ.get('AWS_DEFAULT_REGION') is None else environ.get('AWS_DEFAULT_REGION')
RESOURCE_PLURAL = "vpcendpointserviceconfigurations"

CREATE_WAIT_AFTER_SECONDS = 10
DELETE_WAIT_AFTER_SECONDS = 10
MODIFY_WAIT_AFTER_SECONDS = 5

@pytest.fixture
def vpc_endpoint_service_adoption():
replacements = REPLACEMENT_VALUES.copy()
resource_name = random_suffix_name("vpc-es-adoption", 24)
service_id = get_bootstrap_resources().AdoptedVpcEndpointService.service_id
assert service_id is not None

replacements["VPC_ENDPOINT_SERVICE_ADOPTED_NAME"] = resource_name
replacements["ADOPTION_POLICY"] = "adopt"
replacements["ADOPTION_FIELDS"] = f"{{\\\"serviceID\\\": \\\"{service_id}\\\"}}"

resource_data = load_ec2_resource(
"vpc_endpoint_service_adoption",
additional_replacements=replacements,
)
logging.debug(resource_data)

ref = k8s.CustomResourceReference(
CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
resource_name, namespace="default",
)

k8s.create_custom_resource(ref, resource_data)
time.sleep(CREATE_WAIT_AFTER_SECONDS)

cr = k8s.wait_resource_consumed_by_controller(ref)
assert cr is not None
assert k8s.get_resource_exists(ref)

yield (ref, cr)

_, deleted = k8s.delete_custom_resource(ref, DELETE_WAIT_AFTER_SECONDS)
assert deleted

@service_marker
@pytest.mark.canary
class TestVpcAdoption:

def test_vpc_endpoint_service_configuration_adopt_update(self, ec2_client, vpc_endpoint_service_adoption):
(ref, cr) = vpc_endpoint_service_adoption

time.sleep(CREATE_WAIT_AFTER_SECONDS)

assert cr is not None
assert 'status' in cr
assert 'serviceID' in cr['status']
resource_id = cr['status']['serviceID']

assert 'spec' in cr
assert 'tags' in cr['spec']

# Check VPC Endpoint Service exists in AWS
ec2_validator = EC2Validator(ec2_client)
ec2_validator.assert_vpc_endpoint_service_configuration(resource_id)

updated_endpoint_service_config = ec2_validator.get_vpc_endpoint_service_configuration(resource_id)

actual_tags = updated_endpoint_service_config['Tags']
tags.assert_ack_system_tags(actual_tags)

name_tag = next((tag for tag in actual_tags if tag['Key'] == 'Name'), None)
assert name_tag is not None

name_tag = {'key': 'Name', 'value': name_tag['Value']}
new_tag = {'key': 'TestName', 'value': 'test-value'}
updates = {
"spec": {"tags": [name_tag, new_tag]}
}

k8s.patch_custom_resource(ref, updates)
time.sleep(MODIFY_WAIT_AFTER_SECONDS)

assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=5)

updated_endpoint_service_config = ec2_validator.get_vpc_endpoint_service_configuration(resource_id)
assert updated_endpoint_service_config is not None
assert 'Tags' in updated_endpoint_service_config

expected_tags = [{"Key": name_tag['key'], "Value": name_tag['value']}, {"Key": new_tag['key'], "Value": new_tag['value']}]
tags.assert_equal_without_ack_tags(
actual=updated_endpoint_service_config['Tags'],
expected=expected_tags,
)