Skip to content
Open
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
118 changes: 54 additions & 64 deletions test/e2e/util_gatewayapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -701,79 +701,69 @@ type testListener struct {
func assertExpectedDNSRecords(t *testing.T, expectations map[expectedDnsRecord]bool) error {
t.Helper()

var expectationsMet bool

err := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 1*time.Minute, false, func(context context.Context) (bool, error) {
haveExpectNotPresent := false
// expectationsMet starts true and gets set to false when some expectation is not met.
expectationsMet = true

dnsRecords := &v1.DNSRecordList{}
if err := kclient.List(context, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
return false, fmt.Errorf("failed to list DNSRecords: %v", err)
}

// Iterate over all expectations.
for exp, shouldBePresent := range expectations {
if !shouldBePresent {
haveExpectNotPresent = true
return wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 1*time.Minute, false,
func(ctx context.Context) (bool, error) {
dnsRecords := &v1.DNSRecordList{}
if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
return false, fmt.Errorf("failed to list DNSRecords: %v", err)
Comment on lines +707 to +708
Copy link
Contributor

Choose a reason for hiding this comment

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

In most of the polling cases we log the error and retry the API operator. Returning an error would break the polling loop.

Suggested change
if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
return false, fmt.Errorf("failed to list DNSRecords: %v", err)
if err := kclient.List(ctx, dnsRecords, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
t.Logf("Failed to list DNSRecords: %v", err)
return false, nil

}

// Reset the found and foundReady flags for each expectation.
found := false
foundReady := false
// Look for a DNSRecord that matches the expected gateway and DNS name.
for _, record := range dnsRecords.Items {
if record.Labels["gateway.networking.k8s.io/gateway-name"] == exp.gatewayName &&
record.Spec.DNSName == exp.dnsName {

if !shouldBePresent {
expectationsMet = false
found = true
t.Logf("DNSRecord %q (%s) found but should not be present.", record.Name, exp.dnsName)
return false, nil
}
found = true
// DNSRecord found and should be present, check if it is published
for _, zone := range record.Status.Zones {
for _, condition := range zone.Conditions {
if condition.Type == v1.DNSRecordPublishedConditionType && condition.Status == string(metav1.ConditionTrue) {
t.Logf("Found DNSRecord %q (%s) %s=%s as expected", record.Name, exp.dnsName, condition.Type, condition.Status)
foundReady = true
}
}
}
if !foundReady {
t.Logf("Found DNSRecord %v but could not determine its readiness; retrying...", record.Name)
expectationsMet = false
return false, nil
}
for exp, shouldBePresent := range expectations {
expectationMet, err := checkDNSRecordExpectation(t, *dnsRecords, exp, shouldBePresent)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to pass by pointer to gain some CPU cycles on copying of all DNSRecords of the cluster.

Suggested change
expectationMet, err := checkDNSRecordExpectation(t, *dnsRecords, exp, shouldBePresent)
expectationMet, err := checkDNSRecordExpectation(t, dnsRecords, exp, shouldBePresent)

Unfortunately, go doesn't have pointers to const (hi, C++!) to benefit from the pointer parameter and ensure no modification on the pointed object. But it's quite common to pass by pointer even in read-only cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

if err != nil {
return false, err
}
Comment on lines +713 to +715
Copy link
Contributor

Choose a reason for hiding this comment

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

Does checkDNSRecordExpectation even return an error? I can only see nils returned. If no error can be returned from checkDNSRecordExpectation then it has to return only 1 value (bool).

Copy link
Contributor

@candita candita May 16, 2025

Choose a reason for hiding this comment

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

I would like to see the messages that are logged here, be returned to the caller in this err variable so the caller can figure out what to do with the message, instead of logging it here. Some messages work great for polling, e.g. "DNSRecord %s found but not yet published, retrying...", but don't make sense if you wanted to reuse this function in the future for a one-time call instead of repeated polling.

So let the caller decide what to do with the values returned.

Copy link
Contributor

@alebedev87 alebedev87 May 20, 2025

Choose a reason for hiding this comment

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

Yes, I overlooked this. Indeed, checkDNSRecordExpectation can be a simple function, not a test helper. The logging is better to be done upper the call stack along with the other logs on the same level.

Even a single error return value can be enough:

if err := checkDNSRecordExpectation(dnsRecords, exp, shouldBePresent); err != nil {
    t.Log(err)
    return false, nil
}

if !expectationMet {
return false, nil
}
}

// If the record is expected but not found, return false to continue polling.
if shouldBePresent && !found {
t.Logf("DNSRecord for hostname %q (gateway: %s) is expected to be present but was not found; retrying...", exp.dnsName, exp.gatewayName)
expectationsMet = false
return false, nil
}
// If the record is not expected but was found, return false to continue polling.
if !shouldBePresent && found {
t.Logf("DNSRecord for hostname %q (gateway: %s) is present but was expected to be absent; retrying...", exp.dnsName, exp.gatewayName)
expectationsMet = false
return false, nil
}
return true, nil
})
}

func checkDNSRecordExpectation(t *testing.T, dnsRecords v1.DNSRecordList, exp expectedDnsRecord, shouldBePresent bool) (bool, error) {
for _, rec := range dnsRecords.Items {
if !dnsRecordMatches(rec, exp) {
continue
}
if haveExpectNotPresent {
t.Logf("Continuing polling to ensure non-expected DNSRecords do not exist...")
if !shouldBePresent {
t.Logf("DNSRecord %s found but expected it absent.", exp.dnsName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Logf("DNSRecord %s found but expected it absent.", exp.dnsName)
t.Logf("DNSRecord %q found but expected to be absent; retrying...", exp.dnsName)

Copy link
Contributor

Choose a reason for hiding this comment

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

return false, nil
}
return !haveExpectNotPresent, nil
})
if !isPublished(rec) {
t.Logf("DNSRecord %s found but not yet published, retrying...", exp.dnsName)
Copy link
Contributor

Choose a reason for hiding this comment

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

return false, nil
}
t.Logf("DNSRecord %s found & published as expected", exp.dnsName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Logf("DNSRecord %s found & published as expected", exp.dnsName)
t.Logf("DNSRecord %q found and published as expected", exp.dnsName)

Copy link
Contributor

Choose a reason for hiding this comment

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

return true, nil
}

if !expectationsMet {
return fmt.Errorf("failed to observe expected DNSRecords: %v", err)
if shouldBePresent {
t.Logf("DNSRecord %s not found, retrying...", exp.dnsName)
Copy link
Contributor

Choose a reason for hiding this comment

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

return false, nil
}
return nil
t.Logf("DNSRecord %s correctly absent", exp.dnsName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Logf("DNSRecord %s correctly absent", exp.dnsName)
t.Logf("DNSRecord %q absent as expected", exp.dnsName)

Copy link
Contributor

Choose a reason for hiding this comment

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

return true, nil
}

// dnsRecordMatches checks if a DNSRecord has the labels and spec matching the expectation.
func dnsRecordMatches(rec v1.DNSRecord, exp expectedDnsRecord) bool {
return rec.Labels["gateway.networking.k8s.io/gateway-name"] == exp.gatewayName &&
rec.Spec.DNSName == exp.dnsName
}

// isPublished returns true if the DNSRecord.Status.Zones contains a Published condition = True.
func isPublished(rec v1.DNSRecord) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent in the naming with dnsRecordMatches:

Suggested change
func isPublished(rec v1.DNSRecord) bool {
func dnsRecordPublished(rec v1.DNSRecord) bool {

for _, zone := range rec.Status.Zones {
for _, cond := range zone.Conditions {
if cond.Type == v1.DNSRecordPublishedConditionType &&
cond.Status == string(metav1.ConditionTrue) {
return true
}
}
}
return false
}

// assertHttpRouteSuccessful checks if the http route was created and has parent conditions that indicate
Expand Down