Skip to content

Commit dfc23a2

Browse files
sjbermansjberman
committed
Code review
1 parent bcaab14 commit dfc23a2

File tree

15 files changed

+127
-86
lines changed

15 files changed

+127
-86
lines changed

‎charts/nginx-gateway-fabric/templates/deployment.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,8 @@ spec:
118118
volumeMounts:
119119
- name: nginx-conf
120120
mountPath: /etc/nginx/conf.d
121-
- name: nginx-includes
122-
mountPath: /etc/nginx/modules-includes
121+
- name: module-includes
122+
mountPath: /etc/nginx/module-includes
123123
- name: nginx-secrets
124124
mountPath: /etc/nginx/secrets
125125
- name: nginx-run
@@ -151,8 +151,8 @@ spec:
151151
volumeMounts:
152152
- name: nginx-conf
153153
mountPath: /etc/nginx/conf.d
154-
- name: nginx-includes
155-
mountPath: /etc/nginx/modules-includes
154+
- name: module-includes
155+
mountPath: /etc/nginx/module-includes
156156
- name: nginx-secrets
157157
mountPath: /etc/nginx/secrets
158158
- name: nginx-run
@@ -185,7 +185,7 @@ spec:
185185
volumes:
186186
- name: nginx-conf
187187
emptyDir: {}
188-
- name: nginx-includes
188+
- name: module-includes
189189
emptyDir: {}
190190
- name: nginx-secrets
191191
emptyDir: {}

‎conformance/provisioner/static-deployment.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ spec:
7070
volumeMounts:
7171
- name: nginx-conf
7272
mountPath: /etc/nginx/conf.d
73-
- name: nginx-includes
74-
mountPath: /etc/nginx/modules-includes
73+
- name: module-includes
74+
mountPath: /etc/nginx/module-includes
7575
- name: nginx-secrets
7676
mountPath: /etc/nginx/secrets
7777
- name: nginx-run
@@ -96,8 +96,8 @@ spec:
9696
volumeMounts:
9797
- name: nginx-conf
9898
mountPath: /etc/nginx/conf.d
99-
- name: nginx-includes
100-
mountPath: /etc/nginx/modules-includes
99+
- name: module-includes
100+
mountPath: /etc/nginx/module-includes
101101
- name: nginx-secrets
102102
mountPath: /etc/nginx/secrets
103103
- name: nginx-run
@@ -115,7 +115,7 @@ spec:
115115
volumes:
116116
- name: nginx-conf
117117
emptyDir: {}
118-
- name: nginx-includes
118+
- name: module-includes
119119
emptyDir: {}
120120
- name: nginx-secrets
121121
emptyDir: {}

‎deploy/manifests/nginx-gateway-experimental.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -220,8 +220,8 @@ spec:
220220
volumeMounts:
221221
- name: nginx-conf
222222
mountPath: /etc/nginx/conf.d
223-
- name: nginx-includes
224-
mountPath: /etc/nginx/modules-includes
223+
- name: module-includes
224+
mountPath: /etc/nginx/module-includes
225225
- name: nginx-secrets
226226
mountPath: /etc/nginx/secrets
227227
- name: nginx-run
@@ -246,8 +246,8 @@ spec:
246246
volumeMounts:
247247
- name: nginx-conf
248248
mountPath: /etc/nginx/conf.d
249-
- name: nginx-includes
250-
mountPath: /etc/nginx/modules-includes
249+
- name: module-includes
250+
mountPath: /etc/nginx/module-includes
251251
- name: nginx-secrets
252252
mountPath: /etc/nginx/secrets
253253
- name: nginx-run
@@ -265,7 +265,7 @@ spec:
265265
volumes:
266266
- name: nginx-conf
267267
emptyDir: {}
268-
- name: nginx-includes
268+
- name: module-includes
269269
emptyDir: {}
270270
- name: nginx-secrets
271271
emptyDir: {}

‎deploy/manifests/nginx-gateway.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,8 @@ spec:
216216
volumeMounts:
217217
- name: nginx-conf
218218
mountPath: /etc/nginx/conf.d
219-
- name: nginx-includes
220-
mountPath: /etc/nginx/modules-includes
219+
- name: module-includes
220+
mountPath: /etc/nginx/module-includes
221221
- name: nginx-secrets
222222
mountPath: /etc/nginx/secrets
223223
- name: nginx-run
@@ -242,8 +242,8 @@ spec:
242242
volumeMounts:
243243
- name: nginx-conf
244244
mountPath: /etc/nginx/conf.d
245-
- name: nginx-includes
246-
mountPath: /etc/nginx/modules-includes
245+
- name: module-includes
246+
mountPath: /etc/nginx/module-includes
247247
- name: nginx-secrets
248248
mountPath: /etc/nginx/secrets
249249
- name: nginx-run
@@ -261,7 +261,7 @@ spec:
261261
volumes:
262262
- name: nginx-conf
263263
emptyDir: {}
264-
- name: nginx-includes
264+
- name: module-includes
265265
emptyDir: {}
266266
- name: nginx-secrets
267267
emptyDir: {}

‎deploy/manifests/nginx-plus-gateway-experimental.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -227,8 +227,8 @@ spec:
227227
volumeMounts:
228228
- name: nginx-conf
229229
mountPath: /etc/nginx/conf.d
230-
- name: nginx-includes
231-
mountPath: /etc/nginx/modules-includes
230+
- name: module-includes
231+
mountPath: /etc/nginx/module-includes
232232
- name: nginx-secrets
233233
mountPath: /etc/nginx/secrets
234234
- name: nginx-run
@@ -253,8 +253,8 @@ spec:
253253
volumeMounts:
254254
- name: nginx-conf
255255
mountPath: /etc/nginx/conf.d
256-
- name: nginx-includes
257-
mountPath: /etc/nginx/modules-includes
256+
- name: module-includes
257+
mountPath: /etc/nginx/module-includes
258258
- name: nginx-secrets
259259
mountPath: /etc/nginx/secrets
260260
- name: nginx-run
@@ -272,7 +272,7 @@ spec:
272272
volumes:
273273
- name: nginx-conf
274274
emptyDir: {}
275-
- name: nginx-includes
275+
- name: module-includes
276276
emptyDir: {}
277277
- name: nginx-secrets
278278
emptyDir: {}

‎deploy/manifests/nginx-plus-gateway.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,8 @@ spec:
223223
volumeMounts:
224224
- name: nginx-conf
225225
mountPath: /etc/nginx/conf.d
226-
- name: nginx-includes
227-
mountPath: /etc/nginx/modules-includes
226+
- name: module-includes
227+
mountPath: /etc/nginx/module-includes
228228
- name: nginx-secrets
229229
mountPath: /etc/nginx/secrets
230230
- name: nginx-run
@@ -249,8 +249,8 @@ spec:
249249
volumeMounts:
250250
- name: nginx-conf
251251
mountPath: /etc/nginx/conf.d
252-
- name: nginx-includes
253-
mountPath: /etc/nginx/modules-includes
252+
- name: module-includes
253+
mountPath: /etc/nginx/module-includes
254254
- name: nginx-secrets
255255
mountPath: /etc/nginx/secrets
256256
- name: nginx-run
@@ -268,7 +268,7 @@ spec:
268268
volumes:
269269
- name: nginx-conf
270270
emptyDir: {}
271-
- name: nginx-includes
271+
- name: module-includes
272272
emptyDir: {}
273273
- name: nginx-secrets
274274
emptyDir: {}

‎internal/mode/static/nginx/conf/nginx-plus.conf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
load_module /usr/lib/nginx/modules/ngx_http_js_module.so;
2-
include /etc/nginx/modules-includes/*.conf;
2+
include /etc/nginx/module-includes/*.conf;
33

44
worker_processes auto;
55

‎internal/mode/static/nginx/conf/nginx.conf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
load_module /usr/lib/nginx/modules/ngx_http_js_module.so;
2-
include /etc/nginx/modules-includes/*.conf;
2+
include /etc/nginx/module-includes/*.conf;
33

44
worker_processes auto;
55

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ const (
1717
httpFolder = configFolder + "/conf.d"
1818

1919
// modulesIncludesFolder is the folder where the included "load_module" file is stored.
20-
modulesIncludesFolder = configFolder + "/modules-includes"
20+
modulesIncludesFolder = configFolder + "/module-includes"
2121

2222
// secretsFolder is the folder where secrets (like TLS certs/keys) are stored.
2323
secretsFolder = configFolder + "/secrets"

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,6 @@ func TestGenerate(t *testing.T) {
110110
certBundle := string(files[3].Content)
111111
g.Expect(certBundle).To(Equal("test-cert"))
112112

113-
g.Expect(files[4].Path).To(Equal("/etc/nginx/modules-includes/load-modules.conf"))
113+
g.Expect(files[4].Path).To(Equal("/etc/nginx/module-includes/load-modules.conf"))
114114
g.Expect(files[4].Content).To(Equal([]byte("load_module modules/ngx_otel_module.so;")))
115115
}

‎internal/mode/static/state/dataplane/configuration_test.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2585,17 +2585,22 @@ func TestBuildTelemetry(t *testing.T) {
25852585
Interval: helpers.GetPointer(ngfAPI.Duration("5s")),
25862586
},
25872587
ServiceName: helpers.GetPointer("my-svc"),
2588+
SpanAttributes: []ngfAPI.SpanAttribute{
2589+
{Key: "key", Value: "value"},
2590+
},
25882591
},
25892592
},
25902593
}
25912594

25922595
expTelemetryConfigured := Telemetry{
2593-
Endpoint: "my-otel.svc:4563",
2594-
ServiceName: "ngf:ns:gw:my-svc",
2595-
Interval: "5s",
2596-
BatchSize: 512,
2597-
BatchCount: 4,
2598-
SpanAttributes: []SpanAttribute{},
2596+
Endpoint: "my-otel.svc:4563",
2597+
ServiceName: "ngf:ns:gw:my-svc",
2598+
Interval: "5s",
2599+
BatchSize: 512,
2600+
BatchCount: 4,
2601+
SpanAttributes: []SpanAttribute{
2602+
{Key: "key", Value: "value"},
2603+
},
25992604
}
26002605

26012606
tests := []struct {

‎internal/mode/static/state/dataplane/types.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,6 @@ type Telemetry struct {
272272
type SpanAttribute struct {
273273
// Key is the key for a span attribute.
274274
Key string
275-
276275
// Value is the value for a span attribute.
277276
Value string
278277
}

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,7 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced
105105
return exists
106106
// Similar to Namespace above, NginxProxy reference exists if it once was or currently is linked to a GatewayClass.
107107
case *ngfAPI.NginxProxy:
108-
var existed bool
109-
if g.NginxProxy != nil {
110-
existed = client.ObjectKeyFromObject(obj) == client.ObjectKeyFromObject(g.NginxProxy)
111-
}
112-
exists := isNginxProxyReferenced(obj, g.GatewayClass)
113-
return existed || exists
108+
return isNginxProxyReferenced(nsname, g)
114109
default:
115110
return false
116111
}

‎internal/mode/static/state/graph/nginxproxy.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package graph
33
import (
44
"k8s.io/apimachinery/pkg/types"
55
"k8s.io/apimachinery/pkg/util/validation/field"
6+
"sigs.k8s.io/controller-runtime/pkg/client"
67
v1 "sigs.k8s.io/gateway-api/apis/v1"
78

89
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
@@ -22,9 +23,11 @@ func getNginxProxy(
2223
}
2324

2425
// isNginxProxyReferenced returns whether or not a specific NginxProxy is referenced in the GatewayClass.
25-
func isNginxProxyReferenced(np *ngfAPI.NginxProxy, gc *GatewayClass) bool {
26-
return np != nil && gc != nil &&
27-
gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == np.Name
26+
func isNginxProxyReferenced(npNSName types.NamespacedName, g *Graph) bool {
27+
existed := g.NginxProxy != nil && npNSName == client.ObjectKeyFromObject(g.NginxProxy)
28+
gc := g.GatewayClass
29+
exists := gc != nil && gcReferencesAnyNginxProxy(gc.Source) && gc.Source.Spec.ParametersRef.Name == npNSName.Name
30+
return existed || exists
2831
}
2932

3033
// gcReferencesNginxProxy returns whether a GatewayClass references any NginxProxy resource.

0 commit comments

Comments
 (0)