8000 Update from review feedback · nginx/kubernetes-ingress@99f9c70 · GitHub
[go: up one dir, main page]
Skip to content

Commit 99f9c70

Browse files
soneillf5soneillf5
committed
Update from review feedback
1 parent fa537f4 commit 99f9c70

File tree

7 files changed

+89
-151
lines changed

7 files changed

+89
-151
lines changed

‎internal/configs/configurator.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,16 +1182,6 @@ func (cnf *Configurator) GetVirtualServerCounts() (vsCount int, vsrCount int) {
11821182
return vsCount, vsrCount
11831183
}
11841184

1185-
func (cnf *Configurator) CheckIfListenerExists(transportServerListener *conf_v1alpha1.TransportServerListener) bool {
1186-
listener, exists := cnf.globalCfgParams.Listeners[transportServerListener.Name]
1187-
1188-
if !exists {
1189-
return false
1190-
}
1191-
1192-
return transportServerListener.Protocol == listener.Protocol
1193-
}
1194-
11951185
// AddOrUpdateSpiffeCerts writes Spiffe certs and keys to disk and reloads NGINX
11961186
func (cnf *Configurator) AddOrUpdateSpiffeCerts(svidResponse *workload.X509SVIDs) error {
11971187
svid := svidResponse.Default()

‎internal/configs/configurator_test.go‎

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -234,58 +234,6 @@ func TestGetFileNameForVirtualServerFromKey(t *testing.T) {
234234
}
235235
}
236236

237-
func TestCheckIfListenerExists(t *testing.T) {
238-
tests := []struct {
239-
listener conf_v1alpha1.TransportServerListener
240-
expected bool
241-
msg string
242-
}{
243-
{
244-
listener: conf_v1alpha1.TransportServerListener{
245-
Name: "tcp-listener",
246-
Protocol: "TCP",
247-
},
248-
expected: true,
249-
msg: "name and protocol match",
250-
},
251-
{
252-
listener: conf_v1alpha1.TransportServerListener{
253-
Name: "some-listener",
254-
Protocol: "TCP",
255-
},
256-
expected: false,
257-
msg: "only protocol matches",
258-
},
259-
{
260-
listener: conf_v1alpha1.TransportServerListener{
261-
Name: "tcp-listener",
262-
Protocol: "UDP",
263-
},
264-
expected: false,
265-
msg: "only name matches",
266-
},
267-
}
268-
269-
cnf, err := createTestConfigurator()
270-
if err != nil {
271-
t.Errorf("Failed to create a test configurator: %v", err)
272-
}
273-
274-
cnf.globalCfgParams.Listeners = map[string]Listener{
275-
"tcp-listener": {
276-
Port: 53,
277-
Protocol: "TCP",
278-
},
279-
}
280-
281-
for _, test := range tests {
282-
result := cnf.CheckIfListenerExists(&test.listener)
283-
if result != test.expected {
284-
t.Errorf("CheckIfListenerExists() returned %v but expected %v for the case of %q", result, test.expected, test.msg)
285-
}
286-
}
287-
}
288-
289237
func TestGetFileNameForTransportServer(t *testing.T) {
290238
transportServer := &conf_v1alpha1.TransportServer{
291239
ObjectMeta: meta_v1.ObjectMeta{

‎internal/k8s/controller.go‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -870,26 +870,13 @@ func (lbc *LoadBalancerController) syncTransportServer(task task) {
870870
} else {
871871
glog.V(2).Infof("Adding or Updating TransportServer: %v\n", key)
872872
ts := obj.(*conf_v1alpha1.TransportServer)
873-
changes, problems = lbc.addOrUpdateTransportServer(key, ts)
873+
changes, problems = lbc.configuration.AddOrUpdateTransportServer(ts)
874874
}
875875

876876
lbc.processChanges(changes)
877877
lbc.processProblems(problems)
878878
}
879879

880-
func (lbc *LoadBalancerController) addOrUpdateTransportServer(key string, ts *conf_v1alpha1.TransportServer) ([]ResourceChange, []ConfigurationProblem) {
881-
validationErr := lbc.transportServerValidator.ValidateTransportServer(ts)
882-
if validationErr != nil {
883-
return lbc.configuration.DeleteTransportServer(key)
884-
}
885-
886-
if !lbc.configurator.CheckIfListenerExists(&ts.Spec.Listener) {
887-
return lbc.configuration.DeleteTransportServer(key)
888-
}
889-
890-
return lbc.configuration.AddOrUpdateTransportServer(ts)
891-
}
892-
893880
func (lbc *LoadBalancerController) syncGlobalConfiguration(task task) {
894881
key := task.Key
895882
obj, gcExists, err := lbc.globalConfigurationLister.GetByKey(key)

‎internal/k8s/status.go‎

Lines changed: 65 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -27,23 +27,24 @@ import (
2727
// API. For external information, it primarily reports the IP or host of the LoadBalancer Service exposing the
2828
// Ingress Controller, or an external IP specified in the ConfigMap.
2929
type statusUpdater struct {
30-
client kubernetes.Interface
31-
namespace string
32-
externalServiceName string
33-
externalStatusAddress string
34-
externalServiceAddresses []string
35-
externalServicePorts string
36-
bigIPAddress string
37-
bigIPPorts string
38-
externalEndpoints []v1.ExternalEndpoint
39-
status []api_v1.LoadBalancerIngress
40-
keyFunc func(obj interface{}) (string, error)
41-
ingressLister *storeToIngressLister
42-
virtualServerLister cache.Store
43-
virtualServerRouteLister cache.Store
44-
transportServerLister cache.Store
45-
policyLister cache.Store
46-
confClient k8s_nginx.Interface
30+
client kubernetes.Interface
31+
namespace string
32+
externalServiceName string
33+
externalStatusAddress string
34+
externalServiceAddresses []string
35+
externalServicePorts string
36+
bigIPAddress string
37+
bigIPPorts string
38+
externalEndpoints []v1.ExternalEndpoint
39+
externalListenerEndpoints []conf_v1alpha1.ExternalEndpoint
40+
status []api_v1.LoadBalancerIngress
41+
keyFunc func(obj interface{}) (string, error)
42+
ingressLister *storeToIngressLister
43+
virtualServerLister cache.Store
44+
virtualServerRouteLister cache.Store
45+
transportServerLister cache.Store
46+
policyLister cache.Store
47+
confClient k8s_nginx.Interface
4748
}
4849

4950
func (su *statusUpdater) UpdateExternalEndpointsForResources(resource []Resource) error {
@@ -92,6 +93,11 @@ func (su *statusUpdater) UpdateExternalEndpointsForResource(r Resource) error {
9293
if failed {
9394
return fmt.Errorf("not all Resources updated")
9495
}
96+
case *TransportServerConfiguration:
97+
err := su.updateTransportServerExternalEndpoints(impl.TransportServer)
98+
if err != nil {
99+
return fmt.Errorf("not all Resources updated")
100+
}
95101
}
96102

97103
return nil
@@ -284,6 +290,7 @@ func (su *statusUpdater) SaveStatusFromExternalStatus(externalStatusAddress stri
284290
ips = append(ips, su.externalStatusAddress)
285291
su.saveStatus(ips)
286292
su.externalEndpoints = su.generateExternalEndpointsFromStatus(su.status)
293+
su.externalListenerEndpoints = su.generateExternalListenerEndpointsFromStatus(su.status)
287294
}
288295

289296
// ClearStatusFromExternalService clears the saved status from the External Service
@@ -304,6 +311,7 @@ func (su *statusUpdater) SaveStatusFromExternalService(svc *api_v1.Service) {
304311
}
305312
su.saveStatus(ips)
306313
su.externalEndpoints = su.generateExternalEndpointsFromStatus(su.status)
314+
su.externalListenerEndpoints = su.generateExternalListenerEndpointsFromStatus(su.status)
307315
}
308316

309317
func (su *statusUpdater) SaveStatusFromIngressLink(ip string) {
@@ -318,6 +326,7 @@ func (su *statusUpdater) SaveStatusFromIngressLink(ip string) {
318326
ips := []string{su.bigIPAddress}
319327
su.saveStatus(ips)
320328
su.externalEndpoints = su.generateExternalEndpointsFromStatus(su.status)
329+
su.externalListenerEndpoints = su.generateExternalListenerEndpointsFromStatus(su.status)
321330
}
322331

323332
func (su *statusUpdater) ClearStatusFromIngressLink() {
@@ -332,6 +341,7 @@ func (su *statusUpdater) ClearStatusFromIngressLink() {
332341
ips := []string{}
333342
su.saveStatus(ips)
334343
su.externalEndpoints = su.generateExternalEndpointsFromStatus(su.status)
344+
su.externalListenerEndpoints = su.generateExternalListenerEndpointsFromStatus(su.status)
335345
}
336346

337347
func (su *statusUpdater) retryUpdateTransportServerStatus(tsCopy *conf_v1alpha1.TransportServer) error {
@@ -415,7 +425,7 @@ func (su *statusUpdater) UpdateTransportServerStatus(ts *conf_v1alpha1.Transport
415425
tsCopy.Status.State = state
416426
tsCopy.Status.Reason = reason
417427
tsCopy.Status.Message = message
418-
tsCopy.Status.ExternalEndpoints = toAlphaV1(su.externalEndpoints)
428+
tsCopy.Status.ExternalEndpoints = su.externalListenerEndpoints
419429

420430
_, err = su.confClient.K8sV1alpha1().TransportServers(tsCopy.Namespace).UpdateStatus(context.TODO(), tsCopy, metav1.UpdateOptions{})
421431
if err != nil {
@@ -425,18 +435,6 @@ func (su *statusUpdater) UpdateTransportServerStatus(ts *conf_v1alpha1.Transport
425435
return err
426436
}
427437

428-
func toAlphaV1(endpoints []v1.ExternalEndpoint) []conf_v1alpha1.ExternalEndpoint {
429-
var alphaEndpoints []conf_v1alpha1.ExternalEndpoint
430-
for _, endpoint := range endpoints {
431-
alphaEndpoint := conf_v1alpha1.ExternalEndpoint{
432-
IP: endpoint.IP,
433-
Ports: endpoint.Ports,
434-
}
435-
alphaEndpoints = append(alphaEndpoints, alphaEndpoint)
436-
}
437-
return alphaEndpoints
438-
}
439-
440438
func hasTsStatusChanged(ts *conf_v1alpha1.TransportServer, state string, reason string, message string) bool {
441439
if ts.Status.State != state {
442440
return true
@@ -571,6 +569,28 @@ func (su *statusUpdater) UpdateVirtualServerRouteStatus(vsr *conf_v1.VirtualServ
571569
return err
572570
}
573571

572+
func (su *statusUpdater) updateTransportServerExternalEndpoints(ts *conf_v1alpha1.TransportServer) error {
573+
tsLatest, exists, err := su.transportServerLister.Get(ts)
574+
if err != nil {
575+
glog.V(3).Infof("error getting TransportServer from Store: %v", err)
576+
return err
577+
}
578+
if !exists {
579+
glog.V(3).Infof("TransportServer doesn't exist in Store")
580+
return nil
581+
}
582+
583+
tsCopy := tsLatest.(*conf_v1alpha1.TransportServer).DeepCopy()
584+
tsCopy.Status.ExternalEndpoints = su.externalListenerEndpoints
585+
586+
_, err = su.confClient.K8sV1alpha1().TransportServers(ts.Namespace).UpdateStatus(context.TODO(), tsCopy, metav1.UpdateOptions{})
587+
if err != nil {
588+
glog.V(3).Infof("error setting TransportServer %v/%v status, retrying: %v", tsCopy.Namespace, tsCopy.Name, err)
589+
return su.retryUpdateTransportServerStatus(tsCopy)
590+
}
591+
return err
592+
}
593+
574594
func (su *statusUpdater) updateVirtualServerExternalEndpoints(vs *conf_v1.VirtualServer) error {
575595
// Get a pristine VirtualServer from the Store
576596
vsLatest, exists, err := su.virtualServerLister.Get(vs)
@@ -632,6 +652,21 @@ func (su *statusUpdater) generateExternalEndpointsFromStatus(status []api_v1.Loa
632652
return externalEndpoints
633653
}
634654

655+
func (su *statusUpdater) generateExternalListenerEndpointsFromStatus(status []api_v1.LoadBalancerIngress) []conf_v1alpha1.ExternalEndpoint {
656+
var externalEndpoints []conf_v1alpha1.ExternalEndpoint
657+
for _, lb := range status {
658+
port := su.externalServicePorts
659+
if su.bigIPPorts != "" {
660+
port = su.bigIPPorts
661+
}
662+
663+
endpoint := conf_v1alpha1.ExternalEndpoint{IP: lb.IP, Port: port}
664+
externalEndpoints = append(externalEndpoints, endpoint)
665+
}
666+
667+
return externalEndpoints
668+
}
669+
635670
func hasPolicyStatusChanged(pol *v1.Policy, state string, reason string, message string) bool {
636671
return pol.Status.State != state || pol.Status.Reason != reason || pol.Status.Message != message
637672
}

‎internal/k8s/status_test.go‎

Lines changed: 21 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"reflect"
66
"testing"
77

8+
"github.com/google/go-cmp/cmp"
89
conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
910
conf_v1alpha1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1alpha1"
1011
fake_v1alpha1 "github.com/nginxinc/kubernetes-ingress/pkg/client/clientset/versioned/fake"
@@ -36,31 +37,20 @@ func TestUpdateTransportServerStatus(t *testing.T) {
3637
*ts,
3738
}})
3839

39-
tsLister := storeToTransportServerLister{}
40-
tsLister.Store, _ = cache.NewInformer(
41-
cache.NewListWatchFromClient(
42-
fakeClient.K8sV1alpha1().RESTClient(),
43-
"transportservers",
44-
"nginx-ingress",
45-
fields.Everything(),
46-
),
47-
&conf_v1alpha1.TransportServer{},
48-
2,
49-
nil,
50-
)
40+
tsLister := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc)
5141

52-
err := tsLister.Store.Add(ts)
42+
err := tsLister.Add(ts)
5343
if err != nil {
5444
t.Errorf("Error adding TransportServer to the transportserver lister: %v", err)
5545
}
5646
su := statusUpdater{
57-
transportServerLister: &tsLister,
47+
transportServerLister: tsLister,
5848
confClient: fakeClient,
5949
keyFunc: cache.DeletionHandlingMetaNamespaceKeyFunc,
60-
externalEndpoints: []conf_v1.ExternalEndpoint{
50+
externalListenerEndpoints: []conf_v1alpha1.ExternalEndpoint{
6151
{
62-
IP: "123.123.123.123",
63-
Ports: "1234",
52+
IP: "123.123.123.123",
53+
Port: "1234",
6454
},
6555
},
6656
}
@@ -71,20 +61,15 @@ func TestUpdateTransportServerStatus(t *testing.T) {
7161
}
7262
updatedTs, _ := fakeClient.K8sV1alpha1().TransportServers(ts.Namespace).Get(context.TODO(), ts.Name, meta_v1.GetOptions{})
7363

74-
if updatedTs.Status.State != "after status" {
75-
t.Errorf("expected: %v actual: %v", "after status", updatedTs.Status.State)
64+
expectedStatus := conf_v1alpha1.TransportServerStatus{
65+
State: "after status",
66+
Reason: "after reason",
67+
Message: "after message",
68+
ExternalEndpoints: []conf_v1alpha1.ExternalEndpoint{{IP: "123.123.123.123", Port: "1234"}},
7669
}
77-
if updatedTs.Status.Message != "after message" {
78-
t.Errorf("expected: %v actual: %v", "after message", updatedTs.Status.Message)
79-
}
80-
if updatedTs.Status.Reason != "after reason" {
81-
t.Errorf("expected: %v actual: %v", "after reason", updatedTs.Status.Reason)
82-
}
83-
if updatedTs.Status.ExternalEndpoints[0].Ports != "1234" {
84-
t.Errorf("expected: %v actual: %v", "1234", updatedTs.Status.ExternalEndpoints[0].Ports)
85-
}
86-
if updatedTs.Status.ExternalEndpoints[0].IP != "123.123.123.123" {
87-
t.Errorf("expected: %v actual: %v", "123.123.123.123", updatedTs.Status.ExternalEndpoints[0].IP)
70+
71+
if diff := cmp.Diff(expectedStatus, updatedTs.Status); diff != "" {
72+
t.Errorf("Unexpected status (-want +got):\n%s", diff)
8873
}
8974
}
9075

@@ -107,8 +92,7 @@ func TestUpdateTransportServerStatusIgnoreNoChange(t *testing.T) {
10792
*ts,
10893
}})
10994

110-
tsLister := storeToTransportServerLister{}
111-
tsLister.Store, _ = cache.NewInformer(
95+
tsLister, _ := cache.NewInformer(
11296
cache.NewListWatchFromClient(
11397
fakeClient.K8sV1alpha1().RESTClient(),
11498
"transportservers",
@@ -120,12 +104,12 @@ func TestUpdateTransportServerStatusIgnoreNoChange(t *testing.T) {
120104
nil,
121105
)
122106

123-
err := tsLister.Store.Add(ts)
107+
err := tsLister.Add(ts)
124108
if err != nil {
125109
t.Errorf("Error adding TransportServer to the transportserver lister: %v", err)
126110
}
127111
su := statusUpdater{
128-
transportServerLister: &tsLister,
112+
transportServerLister: tsLister,
129113
confClient: fakeClient,
130114
keyFunc: cache.DeletionHandlingMetaNamespaceKeyFunc,
131115
}
@@ -164,8 +148,7 @@ func TestUpdateTransportServerStatusMissingTransportServer(t *testing.T) {
164148
&conf_v1alpha1.TransportServerList{
165149
Items: []conf_v1alpha1.TransportServer{}})
166150

167-
tsLister := storeToTransportServerLister{}
168-
tsLister.Store, _ = cache.NewInformer(
151+
tsLister, _ := cache.NewInformer(
169152
cache.NewListWatchFromClient(
170153
fakeClient.K8sV1alpha1().RESTClient(),
171154
"transportservers",
@@ -178,7 +161,7 @@ func TestUpdateTransportServerStatusMissingTransportServer(t *testing.T) {
178161
)
179162

180163
su := statusUpdater{
181-
transportServerLister: &tsLister,
164+
transportServerLister: tsLister,
182165
confClient: fakeClient,
183166
keyFunc: cache.DeletionHandlingMetaNamespaceKeyFunc,
184167
externalEndpoints: []conf_v1.ExternalEndpoint{
@@ -196,7 +179,7 @@ func TestUpdateTransportServerStatusMissingTransportServer(t *testing.T) {
196179

197180
updatedTs, _ := fakeClient.K8sV1alpha1().TransportServers(ts.Namespace).Get(context.TODO(), ts.Name, meta_v1.GetOptions{})
198181
if updatedTs != nil {
199-
t.Errorf("expected TransportServer Store would be empty as provided TransportServer was not found")
182+
t.Errorf("expected TransportServer Store would be empty as provided TransportServer was not found. Unexpected updated TransportServer: %v", updatedTs)
200183
}
201184
}
202185

0 commit comments

Comments
 (0)
0