Skip to content

Commit 2785b47

Browse files
sjbermansjberman
committed
Address code review suggestions
1 parent a9c9bcf commit 2785b47

File tree

3 files changed

+74
-74
lines changed

3 files changed

+74
-74
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ const (
4949
)
5050

5151
// ConfigFolders is a list of folders where NGINX configuration files are stored.
52+
// Volumes here also need to be added to our crossplane ephemeral test container.
5253
var ConfigFolders = []string{httpFolder, secretsFolder, includesFolder, modulesIncludesFolder, streamFolder}
5354

5455
// Generator generates NGINX configuration files.

‎tests/framework/crossplane.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package framework
22

33
import (
44
"context"
5+
"encoding/json"
56
"fmt"
67
"net/http"
78
"strings"
@@ -18,9 +19,10 @@ import (
1819
// ExpectedNginxField contains an nginx directive key and value,
1920
// and the expected file, server, and location block that it should exist in.
2021
type ExpectedNginxField struct {
21-
// Key is the directive name.
22-
Key string
23-
// Value is the value for the directive. Can be the full value or a substring.
22+
// Directive is the directive name.
23+
Directive string
24+
// Value is the value for the directive. Can be the full value or a substring. If it's a substring,
25+
// then ValueSubstringAllowed should be true.
2426
Value string
2527
// File is the file name that should contain the directive. Can be a full filename or a substring.
2628
File string
@@ -36,7 +38,7 @@ type ExpectedNginxField struct {
3638

3739
// ValidateNginxFieldExists accepts the nginx config and the configuration for the expected field,
3840
// and returns whether or not that field exists where it should.
39-
func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) bool {
41+
func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) error {
4042
for _, config := range conf.Config {
4143
if !strings.Contains(config.File, expFieldCfg.File) {
4244
continue
@@ -45,7 +47,7 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) boo
4547
for _, directive := range config.Parsed {
4648
if len(expFieldCfg.Servers) == 0 {
4749
if expFieldCfg.fieldFound(directive) {
48-
return true
50+
return nil
4951
}
5052
continue
5153
}
@@ -54,18 +56,23 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) boo
5456
if directive.Directive == "server" && getServerName(directive.Block) == serverName {
5557
for _, serverDirective := range directive.Block {
5658
if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) {
57-
return true
59+
return nil
5860
} else if serverDirective.Directive == "location" &&
5961
fieldExistsInLocation(serverDirective, expFieldCfg) {
60-
return true
62+
return nil
6163
}
6264
}
6365
}
6466
}
6567
}
6668
}
6769

68-
return false
70+
b, err := json.Marshal(conf)
71+
if err != nil {
72+
return fmt.Errorf("error marshaling nginx config: %w", err)
73+
}
74+
75+
return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, string(b))
6976
}
7077

7178
func getServerName(serverBlock Directives) string {
@@ -86,15 +93,15 @@ func (e ExpectedNginxField) fieldFound(directive *Directive) bool {
8693
valueMatch = strings.Contains(arg, e.Value)
8794
}
8895

89-
return directive.Directive == e.Key && valueMatch
96+
return directive.Directive == e.Directive && valueMatch
9097
}
9198

92-
func fieldExistsInLocation(serverDirective *Directive, expFieldCfg ExpectedNginxField) bool {
99+
func fieldExistsInLocation(locationDirective *Directive, expFieldCfg ExpectedNginxField) bool {
93100
// location could start with '=', so get the last element which is the path
94-
loc := serverDirective.Args[len(serverDirective.Args)-1]
101+
loc := locationDirective.Args[len(locationDirective.Args)-1]
95102
if loc == expFieldCfg.Location {
96-
for _, locDirective := range serverDirective.Block {
97-
if expFieldCfg.fieldFound(locDirective) {
103+
for _, directive := range locationDirective.Block {
104+
if expFieldCfg.fieldFound(directive) {
98105
return true
99106
}
100107
}

‎tests/suite/client_settings_test.go

Lines changed: 53 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy"
9393

9494
Context("nginx config", func() {
9595
var conf *framework.Payload
96+
filePrefix := fmt.Sprintf("/etc/nginx/includes/ClientSettingsPolicy_%s", namespace)
9697

9798
BeforeAll(func() {
9899
podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout)
@@ -108,105 +109,96 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy"
108109
DescribeTable("is set properly for",
109110
func(expCfgs []framework.ExpectedNginxField) {
110111
for _, expCfg := range expCfgs {
111-
failureMsg := fmt.Sprintf(
112-
"directive '%s' with value '%s' not found in expected place",
113-
expCfg.Key, expCfg.Value,
114-
)
115-
Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(BeTrue(), failureMsg)
112+
Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed())
116113
}
117114
},
118115
Entry("gateway policy", []framework.ExpectedNginxField{
119116
{
120-
Key: "include",
121-
Value: "gw-csp.conf",
122-
ValueSubstringAllowed: true,
123-
File: "http.conf",
124-
Servers: []string{"*.example.com", "cafe.example.com"},
117+
Directive: "include",
118+
Value: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
119+
File: "http.conf",
120+
Servers: []string{"*.example.com", "cafe.example.com"},
125121
},
126122
{
127-
Key: "client_max_body_size",
128-
Value: "1000",
129-
File: "gw-csp.conf",
123+
Directive: "client_max_body_size",
124+
Value: "1000",
125+
File: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
130126
},
131127
{
132-
Key: "client_body_timeout",
133-
Value: "30s",
134-
File: "gw-csp.conf",
128+
Directive: "client_body_timeout",
129+
Value: "30s",
130+
File: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
135131
},
136132
{
137-
Key: "keepalive_requests",
138-
Value: "100",
139-
File: "gw-csp.conf",
133+
Directive: "keepalive_requests",
134+
Value: "100",
135+
File: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
140136
},
141137
{
142-
Key: "keepalive_time",
143-
Value: "5s",
144-
File: "gw-csp.conf",
138+
Directive: "keepalive_time",
139+
Value: "5s",
140+
File: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
145141
},
146142
{
147-
Key: "keepalive_timeout",
148-
Value: "2s 1s",
149-
File: "gw-csp.conf",
143+
Directive: "keepalive_timeout",
144+
Value: "2s 1s",
145+
File: fmt.Sprintf("%s_gw-csp.conf", filePrefix),
150146
},
151147
}),
152148
Entry("coffee route policy", []framework.ExpectedNginxField{
153149
{
154-
Key: "include",
155-
Value: "coffee-route-csp.conf",
156-
ValueSubstringAllowed: true,
157-
File: "http.conf",
158-
Servers: []string{"cafe.example.com"},
159-
Location: "/coffee",
150+
Directive: "include",
151+
Value: fmt.Sprintf("%s_coffee-route-csp.conf", filePrefix),
152+
File: "http.conf",
153+
Servers: []string{"cafe.example.com"},
154+
Location: "/coffee",
160155
},
161156
{
162-
Key: "client_max_body_size",
163-
Value: "2000",
164-
File: "coffee-route-csp.conf",
157+
Directive: "client_max_body_size",
158+
Value: "2000",
159+
File: fmt.Sprintf("%s_coffee-route-csp.conf", filePrefix),
165160
},
166161
}),
167162
Entry("tea route policy", []framework.ExpectedNginxField{
168163
{
169-
Key: "include",
170-
Value: "tea-route-csp.conf",
171-
ValueSubstringAllowed: true,
172-
File: "http.conf",
173-
Servers: []string{"cafe.example.com"},
174-
Location: "/tea",
164+
Directive: "include",
165+
Value: fmt.Sprintf("%s_tea-route-csp.conf", filePrefix),
166+
File: "http.conf",
167+
Servers: []string{"cafe.example.com"},
168+
Location: "/tea",
175169
},
176170
{
177-
Key: "keepalive_requests",
178-
Value: "200",
179-
File: "tea-route-csp.conf",
171+
Directive: "keepalive_requests",
172+
Value: "200",
173+
File: fmt.Sprintf("%s_tea-route-csp.conf", filePrefix),
180174
},
181175
}),
182176
Entry("soda route policy", []framework.ExpectedNginxField{
183177
{
184-
Key: "include",
185-
Value: "soda-route-csp.conf",
186-
ValueSubstringAllowed: true,
187-
File: "http.conf",
188-
Servers: []string{"cafe.example.com"},
189-
Location: "/soda",
178+
Directive: "include",
179+
Value: fmt.Sprintf("%s_soda-route-csp.conf", filePrefix),
180+
File: "http.conf",
181+
Servers: []string{"cafe.example.com"},
182+
Location: "/soda",
190183
},
191184
{
192-
Key: "client_max_body_size",
193-
Value: "3000",
194-
File: "soda-route-csp.conf",
185+
Directive: "client_max_body_size",
186+
Value: "3000",
187+
File: fmt.Sprintf("%s_soda-route-csp.conf", filePrefix),
195188
},
196189
}),
197190
Entry("grpc route policy", []framework.ExpectedNginxField{
198191
{
199-
Key: "include",
200-
Value: "grpc-route-csp.conf",
201-
ValueSubstringAllowed: true,
202-
File: "http.conf",
203-
Servers: []string{"*.example.com"},
204-
Location: "/helloworld.Greeter/SayHello",
192+
Directive: "include",
193+
Value: fmt.Sprintf("%s_grpc-route-csp.conf", filePrefix),
194+
File: "http.conf",
195+
Servers: []string{"*.example.com"},
196+
Location: "/helloworld.Greeter/SayHello",
205197
},
206198
{
207-
Key: "client_max_body_size",
208-
Value: "0",
209-
File: "grpc-route-csp.conf",
199+
Directive: "client_max_body_size",
200+
Value: "0",
201+
File: fmt.Sprintf("%s_grpc-route-csp.conf", filePrefix),
210202
},
211203
}),
212204
)

0 commit comments

Comments
 (0)