Skip to content

Commit b27d14d

Browse files
sjbermansjberman
committed
Code review feedback
1 parent cec6dc2 commit b27d14d

File tree

7 files changed

+110
-40
lines changed

7 files changed

+110
-40
lines changed

‎tests/README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@ The tests in this directory are meant to be run on a live Kubernetes environment
44
are similar to the existing [conformance tests](../conformance/README.md), but will verify things such as:
55

66
- NGF-specific functionality
7-
- performance
8-
- scale
7+
- Non-Functional requirements testing (such as performance, scale, etc.)
98

10-
When running, the tests create a port-forward from your NGF Pod to localhost. Traffic is sent over this port.
9+
When running, the tests create a port-forward from your NGF Pod to localhost, using a port chosen by the
10+
test framework. Traffic is sent over this port.
1111

1212
Directory structure is as follows:
1313

‎tests/framework/portforward.go‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,19 @@ func GetNGFPodName(
2727
defer cancel()
2828

2929
var podList core.PodList
30-
if err := k8sClient.List(ctx, &podList, client.InNamespace(namespace)); err != nil {
30+
if err := k8sClient.List(
31+
ctx,
32+
&podList,
33+
client.InNamespace(namespace),
34+
client.MatchingLabels{
35+
"app.kubernetes.io/instance": releaseName,
36+
},
37+
); err != nil {
3138
return "", fmt.Errorf("error getting list of Pods: %w", err)
3239
}
3340

34-
for _, pod := range podList.Items {
35-
if val, ok := pod.Labels["app.kubernetes.io/instance"]; ok && val == releaseName {
36-
return pod.Name, nil
37-
}
41+
if len(podList.Items) > 0 {
42+
return podList.Items[0].Name, nil
3843
}
3944

4045
return "", fmt.Errorf("unable to find NGF Pod")

‎tests/framework/request.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,10 @@ import (
1010
"time"
1111
)
1212

13-
// GET sends a GET request to the specified url.
13+
// Get sends a GET request to the specified url.
1414
// It resolves to localhost (where the NGF port forward is running) instead of using DNS.
15-
// The body of the response is returned, or an error.
16-
func GET(url string, timeout time.Duration) (string, error) {
15+
// The status and body of the response is returned, or an error.
16+
func Get(url string, timeout time.Duration) (int, string, error) {
1717
dialer := &net.Dialer{}
1818

1919
http.DefaultTransport.(*http.Transport).DialContext = func(
@@ -31,20 +31,20 @@ func GET(url string, timeout time.Duration) (string, error) {
3131

3232
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
3333
if err != nil {
34-
return "", err
34+
return 0, "", err
3535
}
3636

3737
resp, err := http.DefaultClient.Do(req)
3838
if err != nil {
39-
return "", err
39+
return 0, "", err
4040
}
4141
defer resp.Body.Close()
4242

4343
body := new(bytes.Buffer)
4444
_, err = body.ReadFrom(resp.Body)
4545
if err != nil {
46-
return "", err
46+
return resp.StatusCode, "", err
4747
}
4848

49-
return body.String(), nil
49+
return resp.StatusCode, body.String(), nil
5050
}

‎tests/framework/resourcemanager.go‎

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,13 @@ import (
3939
"k8s.io/apimachinery/pkg/util/yaml"
4040
"sigs.k8s.io/controller-runtime/pkg/client"
4141
v1 "sigs.k8s.io/gateway-api/apis/v1"
42-
configUtils "sigs.k8s.io/gateway-api/conformance/utils/config"
4342
)
4443

4544
// ResourceManager handles creating/updating/deleting Kubernetes resources.
4645
type ResourceManager struct {
4746
K8sClient client.Client
4847
FS embed.FS
49-
TimeoutConfig configUtils.TimeoutConfig
48+
TimeoutConfig TimeoutConfig
5049
}
5150

5251
// Apply creates or updates Kubernetes resources defined as Go objects.
@@ -55,8 +54,20 @@ func (rm *ResourceManager) Apply(resources []client.Object) error {
5554
defer cancel()
5655

5756
for _, resource := range resources {
58-
if err := rm.K8sClient.Create(ctx, resource); err != nil && !apierrors.IsAlreadyExists(err) {
59-
return fmt.Errorf("error applying resource: %w", err)
57+
if err := rm.K8sClient.Get(ctx, client.ObjectKeyFromObject(resource), resource); err != nil {
58+
if !apierrors.IsNotFound(err) {
59+
return fmt.Errorf("error getting resource: %w", err)
60+
}
61+
62+
if err := rm.K8sClient.Create(ctx, resource); err != nil {
63+
return fmt.Errorf("error creating resource: %w", err)
64+
}
65+
66+
continue
67+
}
68+
69+
if err := rm.K8sClient.Update(ctx, resource); err != nil {
70+
return fmt.Errorf("error updating resource: %w", err)
6071
}
6172
}
6273

@@ -179,6 +190,10 @@ func (rm *ResourceManager) getFileContents(file string) (*bytes.Buffer, error) {
179190
}
180191
defer resp.Body.Close()
181192

193+
if resp.StatusCode != http.StatusOK {
194+
return nil, fmt.Errorf("%d response when getting %s file contents", resp.StatusCode, file)
195+
}
196+
182197
manifests := new(bytes.Buffer)
183198
count, err := manifests.ReadFrom(resp.Body)
184199
if err != nil {
@@ -203,19 +218,28 @@ func (rm *ResourceManager) getFileContents(file string) (*bytes.Buffer, error) {
203218
return bytes.NewBuffer(b), nil
204219
}
205220

206-
// WaitForAppsReady waits for all apps in the specified namespace to be ready, or until the ctx timeout is reached.
207-
func (rm *ResourceManager) WaitForAppsReady(k8sClient client.Client, namespace string) error {
221+
// WaitForAppsToBeReady waits for all apps in the specified namespace to be ready,
222+
// or until the ctx timeout is reached.
223+
224+
func (rm *ResourceManager) WaitForAppsToBeReady(namespace string) error {
208225
ctx, cancel := context.WithTimeout(context.Background(), rm.TimeoutConfig.CreateTimeout)
209226
defer cancel()
210227

228+
if err := rm.waitForPodsToBeReady(ctx, namespace); err != nil {
229+
return err
230+
}
231+
232+
return rm.waitForGatewaysToBeReady(ctx, namespace)
233+
}
234+
235+
func (rm *ResourceManager) waitForPodsToBeReady(ctx context.Context, namespace string) error {
211236
return wait.PollUntilContextCancel(
212237
ctx,
213238
500*time.Millisecond,
214239
true, /* poll immediately */
215240
func(ctx context.Context) (bool, error) {
216-
// first check for Pods to be ready
217241
var podList core.PodList
218-
if err := k8sClient.List(ctx, &podList, client.InNamespace(namespace)); err != nil {
242+
if err := rm.K8sClient.List(ctx, &podList, client.InNamespace(namespace)); err != nil {
219243
return false, err
220244
}
221245

@@ -228,13 +252,23 @@ func (rm *ResourceManager) WaitForAppsReady(k8sClient client.Client, namespace s
228252
}
229253
}
230254

231-
if podsReady != len(podList.Items) {
232-
return false, nil
255+
if podsReady == len(podList.Items) {
256+
return true, nil
233257
}
234258

235-
// now check for Gateway to be programmed
259+
return false, nil
260+
},
261+
)
262+
}
263+
264+
func (rm *ResourceManager) waitForGatewaysToBeReady(ctx context.Context, namespace string) error {
265+
return wait.PollUntilContextCancel(
266+
ctx,
267+
500*time.Millisecond,
268+
true, /* poll immediately */
269+
func(ctx context.Context) (bool, error) {
236270
var gatewayList v1.GatewayList
237-
if err := k8sClient.List(ctx, &gatewayList, client.InNamespace(namespace)); err != nil {
271+
if err := rm.K8sClient.List(ctx, &gatewayList, client.InNamespace(namespace)); err != nil {
238272
return false, err
239273
}
240274

‎tests/framework/timeout.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package framework
2+
3+
import "time"
4+
5+
type TimeoutConfig struct {
6+
// CreateTimeout represents the maximum time for a Kubernetes object to be created.
7+
CreateTimeout time.Duration
8+
9+
// DeleteTimeout represents the maximum time for a Kubernetes object to be deleted.
10+
DeleteTimeout time.Duration
11+
12+
// GetTimeout represents the maximum time to get a Kubernetes object.
13+
GetTimeout time.Duration
14+
15+
// ManifestFetchTimeout represents the maximum time for getting content from a https:// URL.
16+
ManifestFetchTimeout time.Duration
17+
18+
// RequestTimeout represents the maximum time for making an HTTP Request with the roundtripper.
19+
RequestTimeout time.Duration
20+
}
21+
22+
// DefaultTimeoutConfig populates a TimeoutConfig with the default values.
23+
func DefaultTimeoutConfig() TimeoutConfig {
24+
return TimeoutConfig{
25+
CreateTimeout: 60 * time.Second,
26+
DeleteTimeout: 10 * time.Second,
27+
GetTimeout: 10 * time.Second,
28+
ManifestFetchTimeout: 10 * time.Second,
29+
RequestTimeout: 10 * time.Second,
30+
}
31+
}

‎tests/suite/sample_test.go‎

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

33
import (
44
"fmt"
5+
"net/http"
56
"strconv"
67

78
. "github.com/onsi/ginkgo/v2"
@@ -28,7 +29,7 @@ var _ = Describe("Basic test example", func() {
2829
BeforeEach(func() {
2930
Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed())
3031
Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed())
31-
Expect(resourceManager.WaitForAppsReady(k8sClient, ns.Name)).To(Succeed())
32+
Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())
3233
})
3334

3435
AfterEach(func() {
@@ -38,8 +39,9 @@ var _ = Describe("Basic test example", func() {
3839

3940
It("sends traffic", func() {
4041
url := fmt.Sprintf("http://hello.example.com:%s/hello", strconv.Itoa(portFwdPort))
41-
body, err := framework.GET(url, timeoutConfig.RequestTimeout)
42+
status, body, err := framework.Get(url, timeoutConfig.RequestTimeout)
4243
Expect(err).ToNot(HaveOccurred())
44+
Expect(status).To(Equal(http.StatusOK))
4345
Expect(body).To(ContainSubstring("URI: /hello"))
4446
})
4547
})

‎tests/suite/system_suite_test.go‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
ctlr "sigs.k8s.io/controller-runtime"
1818
"sigs.k8s.io/controller-runtime/pkg/client"
1919
v1 "sigs.k8s.io/gateway-api/apis/v1"
20-
configUtils "sigs.k8s.io/gateway-api/conformance/utils/config"
2120

2221
"github.com/nginxinc/nginx-gateway-fabric/tests/framework"
2322
)
@@ -44,12 +43,12 @@ var (
4443

4544
var (
4645
//go:embed manifests/*
47-
manifests embed.FS
48-
k8sClient client.Client
49-
resourceManager framework.ResourceManager
50-
stopCh = make(chan struct{}, 1)
51-
portFwdPort int
52-
timeoutConfig configUtils.TimeoutConfig
46+
manifests embed.FS
47+
k8sClient client.Client
48+
resourceManager framework.ResourceManager
49+
portForwardStopCh = make(chan struct{}, 1)
50+
portFwdPort int
51+
timeoutConfig framework.TimeoutConfig
5352
)
5453

5554
var _ = BeforeSuite(func() {
@@ -68,7 +67,7 @@ var _ = BeforeSuite(func() {
6867
k8sClient, err = client.New(k8sConfig, options)
6968
Expect(err).ToNot(HaveOccurred())
7069

71-
timeoutConfig = configUtils.DefaultTimeoutConfig()
70+
timeoutConfig = framework.DefaultTimeoutConfig()
7271
resourceManager = framework.ResourceManager{
7372
K8sClient: k8sClient,
7473
FS: manifests,
@@ -99,13 +98,12 @@ var _ = BeforeSuite(func() {
9998
podName, err := framework.GetNGFPodName(k8sClient, cfg.Namespace, cfg.ReleaseName, timeoutConfig.CreateTimeout)
10099
Expect(err).ToNot(HaveOccurred())
101100

102-
portFwdPort, err = framework.PortForward(k8sConfig, cfg.Namespace, podName, stopCh)
101+
portFwdPort, err = framework.PortForward(k8sConfig, cfg.Namespace, podName, portForwardStopCh)
103102
Expect(err).ToNot(HaveOccurred())
104103
})
105104

106105
var _ = AfterSuite(func() {
107-
// close the port forward
108-
stopCh <- struct{}{}
106+
portForwardStopCh <- struct{}{}
109107

110108
cfg := framework.InstallationConfig{
111109
ReleaseName: "ngf-test",

0 commit comments

Comments
 (0)