Skip to content

Conversation

@NawafSwe
Copy link

Motivation

Currently, many e2e tests use hardcoded YAML strings and fmt.Sprintf-based helper functions to create test resources. This approach has several drawbacks:

  1. Function name bloat: As noted in #11279, this leads to unwieldy function names like MTLSMeshWithMeshServicesAndMetricsKubernetes when we need to support various field combinations.

  2. Maintainability: Static YAML strings scattered throughout test files are harder to maintain and refactor when resource schemas change.

  3. Type safety: String-based YAML generation lacks compile-time validation and IDE support.

This PR addresses these issues by replacing static YAML strings with the existing builder pattern that's already used in unit tests, following the approach suggested in the issue comments.

Changes

This PR replaces hardcoded YAML strings with builder functions in e2e tests:

  • test/e2e_env/multizone/localityawarelb/meshmultizoneservice.go: Replaced static MeshMultiZoneService YAML with builders.MeshMultiZoneService() builder pattern using method chaining.

  • test/e2e_env/multizone/externalservices/externalservices_multizone_universal.go: Fixed incorrect usage of ResourceUniversal() to properly use the builder pattern.

Benefits

  • Better maintainability: Changes to resource schemas are easier to propagate through tests
  • Type safety: Compile-time validation and IDE autocomplete support
  • Readability: Fluent builder API is more readable than YAML strings
  • Consistency: Aligns e2e tests with the builder pattern already used in unit tests
  • Extensibility: Easy to add new field combinations without creating new function variants

Future work

This PR focuses on MeshMultiZoneService as a starting point. Additional resources (such as MeshLoadBalancingStrategy) still use static YAML and can be migrated in follow-up PRs as builders become available.

Related issues

Addresses #11294 and the concerns raised in #11279.

Changelog: skip

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces hardcoded YAML strings with builder pattern functions in e2e tests to improve maintainability, type safety, and readability. The changes systematically convert static YAML resource definitions to the fluent builder API that is already used in unit tests.

Changes:

  • Migrated static YAML definitions to builder pattern for Mesh, MeshMultiZoneService, MeshService, and HostnameGenerator resources
  • Added new builder methods to MeshBuilder for networking and routing configuration
  • Replaced YamlUniversal() with ResourceUniversal() where appropriate
  • Removed redundant helper functions that are now superseded by builders

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/e2e_env/multizone/zoneegress/internal_services.go Converted Mesh YAML to builder pattern with mTLS and zone egress settings
test/e2e_env/multizone/meshtrafficpermission/meshtrafficpermission.go Replaced inline Mesh YAML helper with builder pattern
test/e2e_env/multizone/meshtimeout/meshtimeout.go Converted MeshMultiZoneService YAML to builder pattern
test/e2e_env/multizone/meshservice/sync.go Migrated MeshMultiZoneService and MeshService YAML definitions to builders
test/e2e_env/multizone/meshservice/connectivity.go Converted HostnameGenerator YAML to builder pattern with helper function
test/e2e_env/multizone/meshmultizoneservice/connectivity.go Replaced MeshMultiZoneService YAML with builder pattern
test/e2e_env/multizone/meshhttproute/meshservice.go Converted MeshMultiZoneService YAML to builder pattern
test/e2e_env/multizone/localityawarelb/meshmultizoneservice.go Migrated MeshMultiZoneService YAML to inline builder pattern
test/e2e_env/multizone/localityawarelb/meshloadbalancingstrategy.go Updated function call to use builder return type instead of YAML
test/e2e_env/multizone/localityawarelb/locality_multizone_hybrid.go Converted Mesh helper function from YAML template to builder pattern
test/e2e_env/multizone/gateway/cross-mesh.go Removed redundant MTLSMeshUniversalEgress helper function
test/e2e_env/multizone/externalservices/externalservices_multizone_universal.go Converted Mesh YAML template to builder pattern with method chaining
test/e2e/zoneegress/externalservices/zoneegress_externalservices_hybrid.go Converted Mesh YAML template to builder function with parameters
pkg/test/resources/builders/mesh_builder.go Added new builder methods for networking passthrough, zone egress, locality-aware load balancing, and mesh external service access

`'{{ .DisplayName }}.{{ .Namespace }}.svc.{{ .Zone }}.not-my-mesh-specific.mesh.local'`,
map[string]string{
"kuma.io/env": "kubernetes",
"kuma.io/mesh": meshName,
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The mesh label should be "non-existent-mesh" to match the original YAML behavior for kube-not-my-mesh-specific-msconnectivity, not meshName.

Suggested change
"kuma.io/mesh": meshName,
"kuma.io/mesh": "non-existent-mesh",

Copilot uses AI. Check for mistakes.
`

meshMTLOnFn := func(name string, enableNetworkOutBound, enableZoneIngress bool) *core_mesh.MeshResource {
return builders.Mesh().WithName(nonDefaultMesh).WithBuiltinMTLSBackend("ca-1").
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The function parameter name is unused; the builder always uses nonDefaultMesh instead. Either remove the parameter or use it in WithName(name).

Suggested change
return builders.Mesh().WithName(nonDefaultMesh).WithBuiltinMTLSBackend("ca-1").
return builders.Mesh().WithName(name).WithBuiltinMTLSBackend("ca-1").

Copilot uses AI. Check for mistakes.

meshMTLOnFn := func(name string, enableNetworkOutBound, enableZoneIngress bool) *core_mesh.MeshResource {
return builders.Mesh().WithName(nonDefaultMesh).WithBuiltinMTLSBackend("ca-1").
WithEgressRoutingEnabled().
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The method WithEgressRoutingEnabled() does not exist in the MeshBuilder API. This will cause a compilation error. Remove this line or verify the correct method name.

Suggested change
WithEgressRoutingEnabled().

Copilot uses AI. Check for mistakes.
NewClusterSetup().
Install(NamespaceWithSidecarInjection(namespace)).
Install(YamlK8s(veryLongNamedService)).
Install(ResourceUniversal(veryLongNamedService)).
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The function should use YamlK8s() instead of ResourceUniversal() for Kubernetes resources. The original code used YamlK8s(veryLongNamedService) which is appropriate for KubeZone2.

Suggested change
Install(ResourceUniversal(veryLongNamedService)).
Install(YamlK8s(veryLongNamedService)).

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +78
m.res.Spec.Networking = &mesh_proto.Networking{
Outbound: &mesh_proto.Networking_Outbound{
Passthrough: &wrapperspb.BoolValue{Value: b},
},
}
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The method only sets the passthrough value when Networking is nil. If Networking already exists, the method silently does nothing, leading to incorrect behavior. The method should always update the passthrough value.

Suggested change
m.res.Spec.Networking = &mesh_proto.Networking{
Outbound: &mesh_proto.Networking_Outbound{
Passthrough: &wrapperspb.BoolValue{Value: b},
},
}
}
m.res.Spec.Networking = &mesh_proto.Networking{}
}
if m.res.Spec.Networking.Outbound == nil {
m.res.Spec.Networking.Outbound = &mesh_proto.Networking_Outbound{}
}
m.res.Spec.Networking.Outbound.Passthrough = &wrapperspb.BoolValue{Value: b}

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Contributor

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

  • Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
  • PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
  • IPv6 is taken into account (.e.g: no string concatenation of host port)
  • Tests (Unit test, E2E tests, manual test on universal and k8s)
    • Don't forget ci/ labels to run additional/fewer tests
  • Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

@NawafSwe
Copy link
Author

@slonka appreicate your review,
I will address the copilot comments 🙌

@NawafSwe
Copy link
Author

Hey @bartsmykla , @lukidzi appreicate your review 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant