Skip to content

Commit fbf535b

Browse files
ciarams87ciarams87
committed
Review feedback round 2
1 parent d7bcd4b commit fbf535b

18 files changed

+666
-590
lines changed

‎conformance/Makefile

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ ENABLE_EXPERIMENTAL ?= false
1919
.DEFAULT_GOAL := help
2020

2121
ifeq ($(ENABLE_EXPERIMENTAL),true)
22-
SUPPORTED_FEATURES +=,GRPCExactMethodMatching,GRPCRouteListenerHostnameMatching
22+
SUPPORTED_FEATURES +=,GRPCExactMethodMatching,GRPCRouteListenerHostnameMatching,GRPCRouteHeaderMatching
2323
endif
2424

2525
.PHONY: help

‎internal/mode/static/nginx/config/servers.go

+4-5
Original file line numberDiff line numberDiff line change
@@ -519,13 +519,12 @@ func createMatchLocation(path string) http.Location {
519519
}
520520

521521
func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.Header {
522-
if grpc {
523-
return []http.Header{}
522+
var headers []http.Header
523+
if !grpc {
524+
headers = make([]http.Header, len(baseHeaders))
525+
copy(headers, baseHeaders)
524526
}
525527

526-
headers := make([]http.Header, len(baseHeaders))
527-
copy(headers, baseHeaders)
528-
529528
if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil {
530529
for i, header := range headers {
531530
if header.Name == "Host" {

‎internal/mode/static/nginx/config/servers_test.go

+51-25
Original file line numberDiff line numberDiff line change
@@ -821,7 +821,7 @@ func TestCreateServers(t *testing.T) {
821821
Path: "= /grpc/method",
822822
ProxyPass: "grpc://test_foo_80",
823823
GRPC: true,
824-
ProxySetHeaders: []http.Header{},
824+
ProxySetHeaders: nil,
825825
},
826826
{
827827
Path: "= /grpc-with-backend-tls-policy/method",
@@ -831,7 +831,7 @@ func TestCreateServers(t *testing.T) {
831831
TrustedCertificate: "/etc/nginx/secrets/test-btp.crt",
832832
},
833833
GRPC: true,
834-
ProxySetHeaders: []http.Header{},
834+
ProxySetHeaders: nil,
835835
},
836836
}
837837
}
@@ -1085,7 +1085,7 @@ func TestCreateLocationsRootPath(t *testing.T) {
10851085
},
10861086
}
10871087

1088-
getPathRules := func(rootPath bool) []dataplane.PathRule {
1088+
getPathRules := func(rootPath bool, grpc bool) []dataplane.PathRule {
10891089
rules := []dataplane.PathRule{
10901090
{
10911091
Path: "/path-1",
@@ -1119,17 +1119,54 @@ func TestCreateLocationsRootPath(t *testing.T) {
11191119
})
11201120
}
11211121

1122+
if grpc {
1123+
rules = append(rules, dataplane.PathRule{
1124+
Path: "/grpc",
1125+
GRPC: true,
1126+
MatchRules: []dataplane.MatchRule{
1127+
{
1128+
Match: dataplane.Match{},
1129+
BackendGroup: fooGroup,
1130+
},
1131+
},
1132+
})
1133+
}
1134+
11221135
return rules
11231136
}
11241137

11251138
tests := []struct {
11261139
name string
11271140
pathRules []dataplane.PathRule
11281141
expLocations []http.Location
1142+
grpc bool
11291143
}{
11301144
{
11311145
name: "path rules with no root path should generate a default 404 root location",
1132-
pathRules: getPathRules(false /* rootPath */),
1146+
pathRules: getPathRules(false /* rootPath */, false /* grpc */),
1147+
expLocations: []http.Location{
1148+
{
1149+
Path: "/path-1",
1150+
ProxyPass: "http://test_foo_80$request_uri",
1151+
ProxySetHeaders: baseHeaders,
1152+
},
1153+
{
1154+
Path: "/path-2",
1155+
ProxyPass: "http://test_foo_80$request_uri",
1156+
ProxySetHeaders: baseHeaders,
1157+
},
1158+
{
1159+
Path: "/",
1160+
Return: &http.Return{
1161+
Code: http.StatusNotFound,
1162+
},
1163+
},
1164+
},
1165+
},
1166+
{
1167+
name: "path rules with grpc & with no root path should generate a default 404 root location and GRPC true",
1168+
pathRules: getPathRules(false /* rootPath */, true /* grpc */),
1169+
grpc: true,
11331170
expLocations: []http.Location{
11341171
{
11351172
Path: "/path-1",
@@ -1141,6 +1178,11 @@ func TestCreateLocationsRootPath(t *testing.T) {
11411178
ProxyPass: "http://test_foo_80$request_uri",
11421179
ProxySetHeaders: baseHeaders,
11431180
},
1181+
{
1182+
Path: "/grpc",
1183+
ProxyPass: "grpc://test_foo_80",
1184+
GRPC: true,
1185+
},
11441186
{
11451187
Path: "/",
11461188
Return: &http.Return{
@@ -1151,7 +1193,7 @@ func TestCreateLocationsRootPath(t *testing.T) {
11511193
},
11521194
{
11531195
name: "path rules with a root path should not generate a default 404 root path",
1154-
pathRules: getPathRules(true /* rootPath */),
1196+
pathRules: getPathRules(true /* rootPath */, false /* grpc */),
11551197
expLocations: []http.Location{
11561198
{
11571199
Path: "/path-1",
@@ -1188,8 +1230,9 @@ func TestCreateLocationsRootPath(t *testing.T) {
11881230
t.Run(test.name, func(t *testing.T) {
11891231
g := NewWithT(t)
11901232

1191-
locs, _ := createLocations(test.pathRules, 80)
1233+
locs, grpc := createLocations(test.pathRules, 80)
11921234
g.Expect(locs).To(Equal(test.expLocations))
1235+
g.Expect(grpc).To(Equal(test.grpc))
11931236
})
11941237
}
11951238
}
@@ -1880,25 +1923,8 @@ func TestGenerateProxySetHeaders(t *testing.T) {
18801923
},
18811924
},
18821925
{
1883-
msg: "header filter",
1884-
filters: &dataplane.HTTPFilters{
1885-
RequestHeaderModifiers: &dataplane.HTTPHeaderFilter{
1886-
Add: []dataplane.HTTPHeader{
1887-
{
1888-
Name: "Authorization",
1889-
Value: "my-auth",
1890-
},
1891-
},
1892-
Set: []dataplane.HTTPHeader{
1893-
{
1894-
Name: "Accept-Encoding",
1895-
Value: "gzip",
1896-
},
1897-
},
1898-
Remove: []string{"my-header"},
1899-
},
1900-
},
1901-
expectedHeaders: []http.Header{},
1926+
msg: "grpc",
1927+
expectedHeaders: nil,
19021928
GRPC: true,
19031929
},
19041930
}

‎internal/mode/static/state/change_processor_test.go

+16-24
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ func createRouteBackendRefs(refs []v1.HTTPBackendRef) []graph.RouteBackendRef {
175175
for _, ref := range refs {
176176
rbr := graph.RouteBackendRef{
177177
BackendRef: ref.BackendRef,
178-
Filters: []any{},
179178
}
180179
rbrs = append(rbrs, rbr)
181180
}
@@ -299,7 +298,6 @@ var _ = Describe("ChangeProcessor", func() {
299298
refGrant1, refGrant2 *v1beta1.ReferenceGrant
300299
expGraph *graph.Graph
301300
expRouteHR1, expRouteHR2 *graph.L7Route
302-
hr1Name, hr2Name types.NamespacedName
303301
gatewayAPICRD, gatewayAPICRDUpdated *metav1.PartialObjectMetadata
304302
routeKey1, routeKey2 graph.RouteKey
305303
)
@@ -319,23 +317,15 @@ var _ = Describe("ChangeProcessor", func() {
319317
}
320318

321319
hr1 = createRoute("hr-1", "gateway-1", "foo.example.com", crossNsBackendRef)
322-
hr1Name = types.NamespacedName{Namespace: hr1.Namespace, Name: hr1.Name}
323320

324-
routeKey1 = graph.RouteKey{
325-
NamespacedName: hr1Name,
326-
RouteType: graph.RouteTypeHTTP,
327-
}
321+
routeKey1 = graph.CreateRouteKey(hr1)
328322

329323
hr1Updated = hr1.DeepCopy()
330324
hr1Updated.Generation++
331325

332326
hr2 = createRoute("hr-2", "gateway-2", "bar.example.com")
333-
hr2Name = types.NamespacedName{Namespace: "test", Name: "hr-2"}
334327

335-
routeKey2 = graph.RouteKey{
336-
NamespacedName: hr2Name,
337-
RouteType: graph.RouteTypeHTTP,
338-
}
328+
routeKey2 = graph.CreateRouteKey(hr2)
339329

340330
refGrant1 = &v1beta1.ReferenceGrant{
341331
ObjectMeta: metav1.ObjectMeta{
@@ -428,24 +418,25 @@ var _ = Describe("ChangeProcessor", func() {
428418
})
429419
BeforeEach(func() {
430420
expRouteHR1 = &graph.L7Route{
431-
Source: hr1,
432-
RouteType: graph.RouteTypeHTTP,
433-
SrcParentRefs: hr1.Spec.ParentRefs,
421+
Source: hr1,
422+
RouteType: graph.RouteTypeHTTP,
434423
ParentRefs: []graph.ParentRef{
435424
{
436425
Attachment: &graph.ParentRefAttachmentStatus{
437426
AcceptedHostnames: map[string][]string{"listener-80-1": {"foo.example.com"}},
438427
Attached: true,
439428
},
440-
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
429+
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
430+
SectionName: hr1.Spec.ParentRefs[0].SectionName,
441431
},
442432
{
443433
Attachment: &graph.ParentRefAttachmentStatus{
444434
AcceptedHostnames: map[string][]string{"listener-443-1": {"foo.example.com"}},
445435
Attached: true,
446436
},
447-
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
448-
Idx: 1,
437+
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"},
438+
Idx: 1,
439+
SectionName: hr1.Spec.ParentRefs[1].SectionName,
449440
},
450441
},
451442
Spec: graph.L7RouteSpec{
@@ -475,24 +466,25 @@ var _ = Describe("ChangeProcessor", func() {
475466
}
476467

477468
expRouteHR2 = &graph.L7Route{
478-
Source: hr2,
479-
RouteType: graph.RouteTypeHTTP,
480-
SrcParentRefs: hr2.Spec.ParentRefs,
469+
Source: hr2,
470+
RouteType: graph.RouteTypeHTTP,
481471
ParentRefs: []graph.ParentRef{
482472
{
483473
Attachment: &graph.ParentRefAttachmentStatus{
484474
AcceptedHostnames: map[string][]string{"listener-80-1": {"bar.example.com"}},
485475
Attached: true,
486476
},
487-
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"},
477+
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"},
478+
SectionName: hr2.Spec.ParentRefs[0].SectionName,
488479
},
489480
{
490481
Attachment: &graph.ParentRefAttachmentStatus{
491482
AcceptedHostnames: map[string][]string{"listener-443-1": {"bar.example.com"}},
492483
Attached: true,
493484
},
494-
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"},
495-
Idx: 1,
485+
Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"},
486+
Idx: 1,
487+
SectionName: hr2.Spec.ParentRefs[1].SectionName,
496488
},
497489
},
498490
Spec: graph.L7RouteSpec{

0 commit comments

Comments
 (0)