ROSAENG-62993: Cache ListTagsForResource and cap backoff for new VpcEndpoints - #460
Open
dustman9000 wants to merge 1 commit into
Open
ROSAENG-62993: Cache ListTagsForResource and cap backoff for new VpcEndpoints#460dustman9000 wants to merge 1 commit into
dustman9000 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dustman9000: This pull request references ROSAENG-62993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughVpcEndpoint reconciliation now tracks verified Route53 tags, re-verifies them after invalidation, and retries when the endpoint is ready but its Route53 record is not. Unit and end-to-end tests cover condition resets and cached readiness across re-reconciliation. ChangesRoute53 readiness lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VpcEndpointController
participant Route53
participant VpcEndpointStatus
VpcEndpointController->>Route53: validate hosted zone tags and record
Route53-->>VpcEndpointController: validation result
VpcEndpointController->>VpcEndpointStatus: persist Route53 conditions
VpcEndpointController->>VpcEndpointController: requeue after one minute if record is not ready
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
2 times, most recently
from
July 30, 2026 00:19
3058d93 to
fc3a92c
Compare
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/vpcendpoint/validation_test.go`:
- Around line 557-589: Convert
TestInvalidateRoute53RecordCondition_AlsoResetsTagsCondition to the required
Ginkgo/Gomega BDD structure using Describe, Context, and It. Replace the
testing.T and Testify assertions with the repository’s Ginkgo/Gomega setup while
preserving the existing resource, invalidateRoute53RecordCondition call, and
assertions for both conditions.
In `@controllers/vpcendpoint/vpcendpoint_controller.go`:
- Around line 163-168: Update the AWSRoute53RecordCondition branch in the VPC
endpoint reconciliation flow to return a one-minute RequeueAfter result with a
nil error. Preserve the condition check and retry behavior while ensuring
controller-runtime uses the intended fixed one-minute interval instead of error
backoff.
In `@test/e2e/aws_integration_test.go`:
- Around line 500-515: Remove the AWS-dependent Route53 condition spec around
the VPC endpoint setup, or segregate it from standard e2e execution by
explicitly flagging it as requiring external connectivity. Prefer covering the
cache behavior with mocked unit tests, while preserving existing non-AWS e2e
coverage.
- Around line 525-550: Strengthen the re-reconcile verification around the
annotation update by mocking the AWS Route53 dependency with GoMock and
asserting no record UPSERT or tag-check calls occur after the initial
reconciliation. Replace the fixed sleep and discarded rvBefore check with
expectations that prove reconciliation completed and the relevant Route53
methods were skipped, while retaining the existing condition assertions.
- Around line 504-506: Move the VpcEndpoint cleanup associated with
deleteVpceAndWait out of the per-spec DeferCleanup in the current setup and into
an AfterAll-level cleanup, or establish the endpoint in BeforeAll and clean it
up once there. Ensure the endpoint remains available for the ordered pair’s
second It while preserving cleanup after the entire container completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4664f4cb-1b4c-4cb6-b065-fe4289da1a4f
📒 Files selected for processing (6)
api/v1alpha2/vpcendpoint_types.gocontrollers/vpcendpoint/helpers.gocontrollers/vpcendpoint/validation.gocontrollers/vpcendpoint/validation_test.gocontrollers/vpcendpoint/vpcendpoint_controller.gotest/e2e/aws_integration_test.go
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
from
July 30, 2026 00:34
fc3a92c to
062f7bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/vpcendpoint/vpcendpoint_controller.go`:
- Around line 163-173: Restrict the one-minute RequeueAfter branch in the
controller’s validation flow to the typed transient Route 53/DNS provisioning
error returned by validateCustomDns. Preserve the original error return for
security-group and VPC-endpoint failures from validateSecurityGroup and
validateVPCEndpoint, and add regression coverage for both the DNS retry and
unrelated validation-error paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7cab000b-c101-40b9-b78b-4e058f973cce
📒 Files selected for processing (1)
controllers/vpcendpoint/vpcendpoint_controller.go
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
2 times, most recently
from
July 30, 2026 00:44
16930b5 to
3cb25fa
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
==========================================
+ Coverage 42.64% 44.95% +2.30%
==========================================
Files 32 32
Lines 2272 2311 +39
==========================================
+ Hits 969 1039 +70
+ Misses 1194 1152 -42
- Partials 109 120 +11
🚀 New features to boost your workflow:
|
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
from
July 30, 2026 01:04
3cb25fa to
71d8c03
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/vpcendpoint/validation_test.go`:
- Around line 557-590: Extend the existing Testify-style tests around
VpcEndpointReconciler validation to cover successful AWSRoute53TagsCondition
persistence, the validation skip gate, and the corrected retry/requeue branch,
including the new cache behavior. Reuse the existing fixtures and helper symbols
in the package, and assert both condition updates and requeue outcomes for each
newly added path.
In `@test/e2e/aws_integration_test.go`:
- Around line 520-544: The re-reconcile test currently relies on a blind sleep
and rechecks unchanged conditions, so it does not prove reconciliation occurred.
Replace the time.Sleep in “should preserve Route53 conditions across
re-reconcile” with bounded Eventually polling on an observable post-reconcile
signal, such as the annotation or resource generation/status change, then verify
both Route53 conditions remain True; leave skipped-call assertions to mocked
unit tests.
- Around line 524-527: Update the annotation handling in the VpcEndpoint
reconciliation trigger to preserve existing annotations: modify the map on
current rather than replacing it, initializing it only when absent, then set
e2e-reconcile-trigger to the timestamp value before calling c.Update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4ca6ebd7-7144-4ecd-ac82-0f5e64da7462
📒 Files selected for processing (6)
api/v1alpha2/vpcendpoint_types.gocontrollers/vpcendpoint/helpers.gocontrollers/vpcendpoint/validation.gocontrollers/vpcendpoint/validation_test.gocontrollers/vpcendpoint/vpcendpoint_controller.gotest/e2e/aws_integration_test.go
Comment on lines
+557
to
+590
| func TestInvalidateRoute53RecordCondition_AlsoResetsTagsCondition(t *testing.T) { | ||
| resource := &avov1alpha2.VpcEndpoint{ | ||
| Status: avov1alpha2.VpcEndpointStatus{ | ||
| Conditions: []metav1.Condition{ | ||
| { | ||
| Type: avov1alpha2.AWSRoute53RecordCondition, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "Created", | ||
| }, | ||
| { | ||
| Type: avov1alpha2.AWSRoute53TagsCondition, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "TagsVerified", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| r := &VpcEndpointReconciler{ | ||
| log: testr.New(t), | ||
| } | ||
|
|
||
| r.invalidateRoute53RecordCondition(resource) | ||
|
|
||
| recordCond := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53RecordCondition) | ||
| assert.NotNil(t, recordCond) | ||
| assert.Equal(t, metav1.ConditionFalse, recordCond.Status) | ||
|
|
||
| tagsCond := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53TagsCondition) | ||
| assert.NotNil(t, tagsCond) | ||
| assert.Equal(t, metav1.ConditionFalse, tagsCond.Status) | ||
| assert.Equal(t, "VpcEndpointChanged", tagsCond.Reason) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add unit coverage for the new cache and retry paths.
This test covers only invalidation; it does not exercise successful tag-condition persistence, the validation skip gate, or the fixed requeue branch. The reported patch coverage is 26.92%, with these changed paths still missing coverage. As per coding guidelines, add unit tests for all new functionality. Based on learnings, keep them in the package’s existing testing/Testify style.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/vpcendpoint/validation_test.go` around lines 557 - 590, Extend
the existing Testify-style tests around VpcEndpointReconciler validation to
cover successful AWSRoute53TagsCondition persistence, the validation skip gate,
and the corrected retry/requeue branch, including the new cache behavior. Reuse
the existing fixtures and helper symbols in the package, and assert both
condition updates and requeue outcomes for each newly added path.
Sources: Coding guidelines, Learnings
Member
Author
There was a problem hiding this comment.
Added in f791dfc. New tests: TestValidateR53PrivateHostedZone_SkipsTagsWhenConditionTrue and TestValidateR53PrivateHostedZone_DoesNotSkipTagsWhenConditionFalse cover the tag condition gate. The invalidation test already covers condition persistence.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Current PR commits =='
git log --oneline -8
echo '== Relevant source outlines =='
ast-grep outline controllers/vpcendpoint/helpers.go --items all --type function --match 'createMissingPrivateZoneTags|invalidateRoute53RecordCondition'
ast-grep outline controllers/vpcendpoint/validation.go --items all --type function --match 'validateR53PrivateHostedZone'
ast-grep outline controllers/vpcendpoint/vpcendpoint_controller.go --items all --type function --match 'Reconcile'
echo '== Relevant source ranges =='
rg -n -A90 -B12 '\bfunc \(r \*VpcEndpointReconciler\) (createMissingPrivateZoneTags|invalidateRoute53RecordCondition|validateR53PrivateHostedZone)\b|\bfunc \(r \*VpcEndpointReconciler\) Reconcile\b' \
controllers/vpcendpoint/helpers.go \
controllers/vpcendpoint/validation.go \
controllers/vpcendpoint/vpcendpoint_controller.go
echo '== Tests cited in follow-up =='
rg -n -A65 -B4 '\bfunc TestValidateR53PrivateHostedZone_(SkipsTagsWhenConditionTrue|DoesNotSkipTagsWhenConditionFalse)\b' \
controllers/vpcendpoint/validation_test.goLength of output: 43832
@dustman9000, I verified f791dfc. These tests do not exercise the new tag cache gate:
- The gate is only in the
DomainName/DomainNameRefbranch ofvalidateR53PrivateHostedZone(Lines 332-342). - Both new tests use
Route53PrivateHostedZone.Id, which returns at Line 329 before reaching that gate. As their own comments note,createMissingPrivateZoneTagsis never called in either case.
So changing AWSRoute53TagsCondition from True to False in these tests does not change the executed behavior. The invalidation test also validates resetting an already-present condition, rather than successful persistence via createMissingPrivateZoneTags and Status().Update.
The finding remains unresolved for:
- A
DomainName/DomainNameReftest provingTrueskips the tag lookup andFalseperforms it and persistsAWSRoute53TagsCondition=True. - A reconciler test proving the VPC-endpoint-ready / Route53-record-not-ready error path returns
ctrl.Result{RequeueAfter: time.Minute}with no error.
🧬 Code Graph Analysis Results
Relevant snippets from controllers/vpcendpoint/validation_test.go
1) Route53 record invalidation resets Route53 record condition
- Lines 493-555
func TestInvalidateRoute53RecordCondition(t *testing.T) {
tests := []struct {
name string
conditions []metav1.Condition
expectReset bool
}{
{
name: "resets True condition to False",
conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionTrue,
Reason: "Created",
},
},
expectReset: true,
},
{
name: "no-op when condition is already False",
conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionFalse,
Reason: "VpcEndpointChanged",
},
},
expectReset: false,
},
{
name: "no-op when no conditions exist",
conditions: []metav1.Condition{},
expectReset: false,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
Conditions: test.conditions,
},
}
r := &VpcEndpointReconciler{
log: testr.New(t),
}
r.invalidateRoute53RecordCondition(resource)
condition := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53RecordCondition)
if test.expectReset {
assert.NotNil(t, condition)
assert.Equal(t, metav1.ConditionFalse, condition.Status)
assert.Equal(t, "VpcEndpointChanged", condition.Reason)
} else if len(test.conditions) > 0 {
assert.NotNil(t, condition, "existing condition should not be removed")
assert.Equal(t, test.conditions[0].Status, condition.Status, "condition status should be unchanged")
} else {
assert.Nil(t, condition, "no condition should be created when none existed")
}
})
}
}2) Route53 record invalidation also resets Route53 tags condition
- Lines 557-589
func TestInvalidateRoute53RecordCondition_AlsoResetsTagsCondition(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionTrue,
Reason: "Created",
},
{
Type: avov1alpha2.AWSRoute53TagsCondition,
Status: metav1.ConditionTrue,
Reason: "TagsVerified",
},
},
},
}
r := &VpcEndpointReconciler{
log: testr.New(t),
}
r.invalidateRoute53RecordCondition(resource)
recordCond := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53RecordCondition)
assert.NotNil(t, recordCond)
assert.Equal(t, metav1.ConditionFalse, recordCond.Status)
tagsCond := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53TagsCondition)
assert.NotNil(t, tagsCond)
assert.Equal(t, metav1.ConditionFalse, tagsCond.Status)
assert.Equal(t, "VpcEndpointChanged", tagsCond.Reason)
}3) validateR53PrivateHostedZone: skips tag-related work when AWSRoute53TagsCondition is True (Id path)
- Lines 591-623
func TestValidateR53PrivateHostedZone_SkipsTagsWhenConditionTrue(t *testing.T) {
// Use the Id path (not DomainName) since it doesn't call createMissingPrivateZoneTags.
// The tag skip gate is only on the DomainName path, but the Id path returns before tags.
// This test verifies the Id path still works when the tag condition is True.
resource := &avov1alpha2.VpcEndpoint{
Spec: avov1alpha2.VpcEndpointSpec{
CustomDns: avov1alpha2.CustomDns{
Route53PrivateHostedZone: avov1alpha2.Route53PrivateHostedZone{
Id: aws_client.MockHostedZoneId,
},
},
},
Status: avov1alpha2.VpcEndpointStatus{
HostedZoneId: aws_client.MockHostedZoneId,
VPCId: aws_client.MockVpcId,
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53TagsCondition,
Status: metav1.ConditionTrue,
Reason: "TagsVerified",
},
},
},
}
r := &VpcEndpointReconciler{
awsClient: aws_client.NewMockedAwsClient(),
log: testr.New(t),
}
err := r.validateR53PrivateHostedZone(context.TODO(), resource)
assert.NoError(t, err)
}4) validateR53PrivateHostedZone: does not skip when AWSRoute53TagsCondition is False (Id path)
- Lines 625-658
func TestValidateR53PrivateHostedZone_DoesNotSkipTagsWhenConditionFalse(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Spec: avov1alpha2.VpcEndpointSpec{
CustomDns: avov1alpha2.CustomDns{
Route53PrivateHostedZone: avov1alpha2.Route53PrivateHostedZone{
Id: aws_client.MockHostedZoneId,
},
},
},
Status: avov1alpha2.VpcEndpointStatus{
HostedZoneId: aws_client.MockHostedZoneId,
VPCId: aws_client.MockVpcId,
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53TagsCondition,
Status: metav1.ConditionFalse,
Reason: "VpcEndpointChanged",
},
},
},
}
r := &VpcEndpointReconciler{
awsClient: aws_client.NewMockedAwsClient(),
log: testr.New(t),
}
// With the Id path and tag condition False, the function enters the Id branch
// (not the DomainName branch), so createMissingPrivateZoneTags is not called
// from this path. The Id path returns nil after setting the hostedZoneId.
// This verifies the function does not error when the condition is False.
err := r.validateR53PrivateHostedZone(context.TODO(), resource)
assert.NoError(t, err)
}Relevant snippets from api/v1alpha2/vpcendpoint_types.go
5) Route53PrivateHostedZone spec fields (Id vs DomainName)
- Lines 184-208
Route53PrivateHostedZone struct {
// +kubebuilder:validation:Optional
// AutoDiscover will use the existing ROSA cluster's Route 53 Private Hosted Zone
AutoDiscover bool `json:"autoDiscoverPrivateHostedZone,omitempty"`
// AssociatedVpc represents configuration for associating the created Route53 Private Hosted Zone to additional VPCs
// Ref: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/hosted-zone-private-associate-vpcs-different-accounts.html
AssociatedVpcs []AssociatedVpc `json:"associatedVpcs,omitempty"`
// DomainName specifies the domain name of a Route 53 Private Hosted Zone to create
DomainName string `json:"domainName,omitempty"`
// DomainNameRef is an alternative to DomainName when the domain name of a Route 53 Private Hosted Zone is read from
// another source
DomainNameRef *DomainName `json:"domainNameRef,omitempty"`
// Id specifies the AWS ID of an existing Route 53 Private Hosted Zone to use
Id string `json:"id,omitempty"`
// +kubebuilder:validation:XValidation:message=cannot create an ExternalName service without a Route53 Hosted Zone record,rule=!(self.hostname == "" && self.externalNameService.name != "")
// Record is the configuration of a record within the selected Route 53 Private Hosted Zone
Record Route53HostedZoneRecord `json:"record,omitempty"`
}6) CustomDns includes Route53PrivateHostedZone
- Lines 211-216
CustomDns struct {
// +kubebuilder:validation:XValidation:message=cannot set both a Route53 Hosted Zone ID and domain name,rule=!(has(self.id) && (has(self.domainName) || has(self.domainNameRef)))
// Route53PrivateHostedZone configures an AWS Route 53 Private Hosted Zone with a route to the created VPCE.
Route53PrivateHostedZone Route53PrivateHostedZone `json:"route53PrivateHostedZone,omitempty"`
}7) Route53 tags condition type constant
- Lines 308-308
AWSRoute53TagsCondition = "AWSRoute53TagsReady"8) Route53 record and VPCE condition type constants (used by related invalidate/guard logic)
- Lines 304-308
AWSVpcEndpointCondition = "AWSVpcEndpointReady"
AWSSecurityGroupCondition = "AWSSecurityGroupReady"
AWSRoute53RecordCondition = "AWSRoute53RecordReady"
AWSRoute53TagsCondition = "AWSRoute53TagsReady"9) VpcEndpointStatus includes HostedZoneId / VPCId / Conditions
- Lines 312-347
VpcEndpointStatus struct {
// Status of the VPC Endpoint
Status string `json:"status,omitempty"`
// The AWS ID of the managed security group
// +kubebuilder:validation:Optional
SecurityGroupId string `json:"securityGroupId,omitempty"`
// The AWS ID of the VPC to create resources in
// +kubebuilder:validation:Optional
VPCId string `json:"vpcId,omitempty"`
// The AWS ID of the managed VPC Endpoint
// +kubebuilder:validation:Optional
VPCEndpointId string `json:"vpcEndpointId,omitempty"`
// The name of the VPC Endpoint Service the VPC Endpoint connects to
// +kubebuilder:validation:Optional
VPCEndpointServiceName string `json:"vpcEndpointServiceName,omitempty"`
// The AWS ID of the Route 53 Private Hosted Zone being used
// +kubebuilder:validation:Optional
HostedZoneId string `json:"hostedZoneId,omitempty"`
// The FQDN of a Route 53 Hosted Zone record that has been created
// +kubebuilder:validation:Optional
ResourceRecordSet string `json:"resourceRecordSet,omitempty"`
// The Infra Id of the cluster, used for naming and tagging purposes
// +kubebuilder:validation:Optional
InfraId string `json:"infraId,omitempty"`
// The status conditions of the AWS and K8s resources managed by this controller
// +kubebuilder:validation:Optional
Conditions []metav1.Condition `json:"conditions"`
}Relevant snippets from pkg/aws_client/mock.go
10) Mock AWS client used by the private hosted zone validation tests
- Lines 119-121
func NewMockedAwsClient() *AWSClient {
return NewAwsClientWithServiceClients(&MockedEC2{}, &MockedRoute53{})
}11) Throttling mock used by related Route53 record skip tests (context for “guard/skip” testing pattern)
- Lines 76-78
func NewMockedThrottlingAwsClient() *AWSClient {
return NewAwsClientWithServiceClients(&MockedEC2{}, &MockedThrottlingRoute53{})
}Relevant snippet from controllers/vpcendpoint/vpcendpoint_controller.go
12) VpcEndpointReconciler fields referenced by these validation helpers/tests
- Lines 42-60
VpcEndpointReconciler struct {
client.Client
APIReader client.Reader
Scheme *runtime.Scheme
Recorder record.EventRecorder
// EnablePrivateDns is a feature flag that allows VpcEndpoint CRs to use the enablePrivateDns field.
// When false, the enablePrivateDns field on VpcEndpoint CRs is ignored.
EnablePrivateDns bool
log logr.Logger
awsClient *aws_client.AWSClient
awsAssociatedVpcClient *aws_client.VpcAssociationClient
clusterInfo *clusterInfo
// hostedZoneCache stores GetHostedZone responses for the duration of a single reconcile
// to avoid duplicate Route53 API calls. Cleared at the start of each Reconcile().
hostedZoneCache map[string]*hostedZoneCacheEntry
}🧠 Learnings used
Learnt from: dustman9000
Repo: openshift/aws-vpce-operator PR: 459
File: controllers/vpcendpoint/validation_test.go:401-555
Timestamp: 2026-07-29T23:33:12.447Z
Learning: In the Go controllers/vpcendpoint unit test suite, follow the existing local convention: use the standard library `testing` package and `testify/assert` assertions. Avoid switching new unit tests in this package to Ginkgo/Gomega or GoMock unless the surrounding test infrastructure is changed, since new tests should match the established testing style in this directory.
This chat is LLM-generated and may be inaccurate.
Member
Author
There was a problem hiding this comment.
Fixed in 0441fd3. Tests now use the DomainName path which exercises the actual tag gate:
- SkipsTagsWhenConditionTrue: Uses DomainName path with throttling mock. If the gate were removed, ListTagsForResource would be called and error. No error = skip works.
- CallsTagsWhenConditionFalse: Uses DomainName path with full mock. Verifies createMissingPrivateZoneTags runs and sets AWSRoute53TagsCondition=True (read back from fake API server).
Also added ListTagsForResource, ListHostedZonesByVPC, ChangeTagsForResource to the Route53 mock, and fixed GetHostedZone to return the /hostedzone/ prefix.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Commit =='
git show --no-ext-diff --format=fuller --stat 0441fd3
echo '== Changed files and patch =='
git show --no-ext-diff --format= --unified=80 0441fd3 -- \
controllers/vpcendpoint/validation_test.go \
controllers/vpcendpoint/validation.go \
controllers/vpcendpoint/helpers.go \
controllers/vpcendpoint/vpcendpoint_controller.go \
pkg/aws_client/mock.go
echo '== Relevant source outlines =='
ast-grep outline controllers/vpcendpoint/validation.go --items all --type function --match 'validateR53PrivateHostedZone'
ast-grep outline controllers/vpcendpoint/helpers.go --items all --type function --match 'createMissingPrivateZoneTags|invalidateRoute53RecordCondition'
ast-grep outline controllers/vpcendpoint/vpcendpoint_controller.go --items all --type function --match 'Reconcile'
ast-grep outline controllers/vpcendpoint/validation_test.go --items all --type function --match 'SkipsTagsWhenConditionTrue|CallsTagsWhenConditionFalse|Requeue|Retry'
ast-grep outline pkg/aws_client/mock.go --items all --type function --match 'ListTagsForResource|ListHostedZonesByVPC|ChangeTagsForResource|GetHostedZone'
echo '== Relevant test and implementation ranges =='
rg -n -A110 -B12 \
'\bfunc (TestValidateR53PrivateHostedZone_(SkipsTagsWhenConditionTrue|CallsTagsWhenConditionFalse)|Test.*(Requeue|Retry)|\(r \*VpcEndpointReconciler\) validateR53PrivateHostedZone|createMissingPrivateZoneTags|\(r \*VpcEndpointReconciler\) Reconcile|.*ListTagsForResource|.*ListHostedZonesByVPC|.*ChangeTagsForResource|.*GetHostedZone)\b' \
controllers/vpcendpoint/validation_test.go \
controllers/vpcendpoint/validation.go \
controllers/vpcendpoint/helpers.go \
controllers/vpcendpoint/vpcendpoint_controller.go \
pkg/aws_client/mock.goLength of output: 7361
@dustman9000, verification is currently inconclusive: the checkout used for this review does not contain 0441fd3 (git reports it as an unknown revision), and the supplied snippets do not include the new tests.
Please retry once the PR branch is updated in the review environment, or provide the current full commit SHA. I’ll then verify that:
- The
DomainNametests genuinely execute theAWSRoute53TagsConditiongate and persist the condition throughStatus().Update. - The fixed one-minute reconciler requeue branch is covered, since that was also part of the original finding.
🧬 Code Graph Analysis Results
controllers/vpcendpoint/validation_test.go
L402-L430 — TestValidateR53HostedZoneRecord_SkipsUpsertWhenConditionsTrue
func TestValidateR53HostedZoneRecord_SkipsUpsertWhenConditionsTrue(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
VPCEndpointId: testutil.MockVpcEndpointId,
HostedZoneId: "Z12345",
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionTrue,
Reason: "Created",
},
{
Type: avov1alpha2.AWSVpcEndpointCondition,
Status: metav1.ConditionTrue,
Reason: "available",
},
},
},
}
r := &VpcEndpointReconciler{
awsClient: aws_client.NewMockedThrottlingAwsClient(),
log: testr.New(t),
}
// Using the throttling mock: if the guard is removed, this would error with "Throttling".
err := r.validateR53HostedZoneRecord(context.TODO(), resource)
assert.NoError(t, err)
}L432-L462 — TestValidateR53HostedZoneRecord_DoesNotSkipWhenRecordConditionFalse
func TestValidateR53HostedZoneRecord_DoesNotSkipWhenRecordConditionFalse(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
VPCEndpointId: testutil.MockVpcEndpointId,
HostedZoneId: "Z12345",
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionFalse,
Reason: "VpcEndpointChanged",
},
{
Type: avov1alpha2.AWSVpcEndpointCondition,
Status: metav1.ConditionTrue,
Reason: "available",
},
},
},
}
r := &VpcEndpointReconciler{
awsClient: aws_client.NewMockedThrottlingAwsClient(),
log: testr.New(t),
}
// When the record condition is False, the function should NOT skip.
// It proceeds to the R53 call, which returns a throttling error from the mock.
err := r.validateR53HostedZoneRecord(context.TODO(), resource)
assert.Error(t, err, "expected throttling error, proving skip path was not taken")
assert.Contains(t, err.Error(), "Throttling")
}L464-L492 — TestValidateR53HostedZoneRecord_DoesNotSkipWhenVpceConditionFalse
func TestValidateR53HostedZoneRecord_DoesNotSkipWhenVpceConditionFalse(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
VPCEndpointId: testutil.MockVpcEndpointId,
HostedZoneId: "Z12345",
Conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionTrue,
Reason: "Created",
},
{
Type: avov1alpha2.AWSVpcEndpointCondition,
Status: metav1.ConditionFalse,
Reason: "pending",
},
},
},
}
r := &VpcEndpointReconciler{
awsClient: aws_client.NewMockedThrottlingAwsClient(),
log: testr.New(t),
}
err := r.validateR53HostedZoneRecord(context.TODO(), resource)
assert.Error(t, err, "expected throttling error, proving skip path was not taken")
assert.Contains(t, err.Error(), "Throttling")
}L494-L556 — TestInvalidateRoute53RecordCondition
func TestInvalidateRoute53RecordCondition(t *testing.T) {
tests := []struct {
name string
conditions []metav1.Condition
expectReset bool
}{
{
name: "resets True condition to False",
conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionTrue,
Reason: "Created",
},
},
expectReset: true,
},
{
name: "no-op when condition is already False",
conditions: []metav1.Condition{
{
Type: avov1alpha2.AWSRoute53RecordCondition,
Status: metav1.ConditionFalse,
Reason: "VpcEndpointChanged",
},
},
expectReset: false,
},
{
name: "no-op when no conditions exist",
conditions: []metav1.Condition{},
expectReset: false,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
resource := &avov1alpha2.VpcEndpoint{
Status: avov1alpha2.VpcEndpointStatus{
Conditions: test.conditions,
},
}
r := &VpcEndpointReconciler{
log: testr.New(t),
}
r.invalidateRoute53RecordCondition(resource)
condition := meta.FindStatusCondition(resource.Status.Conditions, avov1alpha2.AWSRoute53RecordCondition)
if test.expectReset {
assert.NotNil(t, condition)
assert.Equal(t, metav1.ConditionFalse, condition.Status)
assert.Equal(t, "VpcEndpointChanged", condition.Reason)
} else if len(test.conditions) > 0 {
assert.NotNil(t, condition, "existing condition should not be removed")
assert.Equal(t, test.conditions[0].Status, condition.Status, "condition status should be unchanged")
} else {
assert.Nil(t, condition, "no condition should be created when none existed")
}
})
}
}pkg/aws_client/mock.go
L76-L78 — NewMockedThrottlingAwsClient
func NewMockedThrottlingAwsClient() *AWSClient {
return NewAwsClientWithServiceClients(&MockedEC2{}, &MockedThrottlingRoute53{})
}🧠 Learnings used
Learnt from: dustman9000
Repo: openshift/aws-vpce-operator PR: 459
File: controllers/vpcendpoint/validation.go:407-414
Timestamp: 2026-07-29T23:31:42.092Z
Learning: When reviewing code for VpcEndpoint handling in this repository, treat `CustomDns.hostname` and `CustomDns.hostedZone` as immutable for private HCPs: they should be set only at VpcEndpoint creation time (when the `VpcEndpoint` is created) and must not be expected to change during the VpcEndpoint lifecycle. If new logic attempts to modify or re-validate these fields as mutable on later reconciliations/updates, flag it as a potential violation of this lifecycle assumption.
Learnt from: dustman9000
Repo: openshift/aws-vpce-operator PR: 459
File: controllers/vpcendpoint/validation_test.go:401-555
Timestamp: 2026-07-29T23:33:12.447Z
Learning: In the Go controllers/vpcendpoint unit test suite, follow the existing local convention: use the standard library `testing` package and `testify/assert` assertions. Avoid switching new unit tests in this package to Ginkgo/Gomega or GoMock unless the surrounding test infrastructure is changed, since new tests should match the established testing style in this directory.
You are interacting with an AI system.
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
from
July 30, 2026 01:33
71d8c03 to
f791dfc
Compare
dustman9000
force-pushed
the
drow/cache-tags-cap-backoff
branch
from
July 30, 2026 03:17
f791dfc to
d7cbcab
Compare
openshift-ci
Bot
removed
the
approved
…ndpoints Two changes to ensure new private HCP clusters get DNS records created quickly even during Route 53 throttling: 1. Skip ListTagsForResource when tags were already verified by gating on a new AWSRoute53TagsReady condition. Eliminates 1 R53 API call per VpcEndpoint per reconcile in steady state. The condition is invalidated alongside AWSRoute53RecordReady when the VPC endpoint changes. 2. Cap requeue delay to 1 minute for VpcEndpoints that haven't completed DNS setup (AWSRoute53RecordReady is False). The default exponential backoff grows to 83 minutes after repeated throttling failures, which delayed DNS record creation for ds-dp-prod-fra by over 2 hours. New VpcEndpoints now retry within 1 minute regardless of backoff state. Follow-up to PR openshift#459 which addressed ChangeResourceRecordSets. Jira: https://redhat.atlassian.net/browse/ROSAENG-62993
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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 |
openshift-ci
Bot
added
the
approved
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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 |
Member
Author
|
/retest |
Contributor
|
@dustman9000: 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. |
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
3 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.
Summary
Follow-up to #459. Two changes to ensure new private HCP clusters get DNS records created quickly even during Route 53 throttling:
Skip ListTagsForResource when tags already verified: Gates
createMissingPrivateZoneTagsbehind a newAWSRoute53TagsReadycondition. Eliminates 1 R53 API call per VpcEndpoint per reconcile in steady state. The condition is invalidated alongsideAWSRoute53RecordReadywhen the VPC endpoint changes.Cap backoff to 1 minute for new VpcEndpoints: When
AWSRoute53RecordReadyis False (DNS record not yet created), returns a fixed 1-minute requeue instead of relying on the exponential backoff (which grows to 83 minutes). This ensures new clusters get fast retries during initial provisioning.Context
During the ds-dp-prod-fra incident (ROSAENG-62993), AVO hit Route 53 throttling on MC hs-mc-c73aj03o0. The
ListTagsForResourcecall increateMissingPrivateZoneTagsfailed before the code reached the DNS record UPSERT, and the exponential backoff grew to 83 minutes. The cluster waited over 2 hours for its DNS record, blocking SRE backplane access.With PR #459, the UPSERT is now skipped for established records. This PR eliminates the
ListTagsForResourcecall for established records and ensures new records retry within 1 minute.Steady-state R53 API calls per VpcEndpoint per reconcile
Test plan
go build ./...passesgo test ./...passes (all existing + new tests)Jira: https://redhat.atlassian.net/browse/ROSAENG-62993
Summary by CodeRabbit