Skip to content

Commit 01a7db3

Browse files
Dean-CoakleyDean-Coakley
authored
Fix Configurator error checking (#348)
* Fixed unchecked Configurator errors * Added unit tests for AddOrUpdateIngress and AddOrUpdateIngressMergeable * Added unit tests for UpdateEndpoints and UpdateEndpointsMergeableIngress * Fixed mergeableIngress typo
1 parent 7366989 commit 01a7db3

File tree

3 files changed

+189
-24
lines changed

3 files changed

+189
-24
lines changed

‎nginx-controller/controller/controller.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -679,7 +679,7 @@ func (lbc *LoadBalancerController) syncIng(task Task) {
679679
}
680680
return
681681
}
682-
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngExs)
682+
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngExs)
683683
if err != nil {
684684
lbc.recorder.Eventf(ing, api_v1.EventTypeWarning, "AddedOrUpdatedWithError", "Configuration for %v(Master) was added or updated, but not applied: %v", key, err)
685685
for _, minion := range mergeableIngExs.Minions {
@@ -804,7 +804,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) {
804804
glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err)
805805
continue
806806
}
807-
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngress)
807+
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngress)
808808
if err != nil {
809809
glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err)
810810
}
@@ -864,7 +864,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) {
864864
glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err)
865865
continue
866866
}
867-
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngress)
867+
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngress)
868868
if err != nil {
869869
glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err)
870870
}

‎nginx-controller/nginx/configurator.go

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,11 @@ func (cnf *Configurator) AddOrUpdateDHParam(content string) (string, error) {
5757

5858
// AddOrUpdateIngress adds or updates NGINX configuration for the Ingress resource
5959
func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) error {
60-
cnf.addOrUpdateIngress(ingEx)
61-
60+
if err := cnf.addOrUpdateIngress(ingEx); err != nil {
61+
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
62+
}
6263
if err := cnf.nginx.Reload(); err != nil {
63-
return fmt.Errorf("Error when adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
64+
return fmt.Errorf("Error reloading NGINX for %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
6465
}
6566
return nil
6667
}
@@ -79,17 +80,18 @@ func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error {
7980
return nil
8081
}
8182

82-
// AddOrUpdateMergableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types
83-
func (cnf *Configurator) AddOrUpdateMergableIngress(mergeableIngs *MergeableIngresses) error {
84-
cnf.addOrUpdateMergableIngress(mergeableIngs)
85-
86-
if err := cnf.nginx.Reload(); err != nil {
83+
// AddOrUpdateMergeableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types
84+
func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error {
85+
if err := cnf.addOrUpdateMergeableIngress(mergeableIngs); err != nil {
8786
return fmt.Errorf("Error when adding or updating ingress %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
8887
}
88+
if err := cnf.nginx.Reload(); err != nil {
89+
return fmt.Errorf("Error reloading NGINX for %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
90+
}
8991
return nil
9092
}
9193

92-
func (cnf *Configurator) addOrUpdateMergableIngress(mergeableIngs *MergeableIngresses) error {
94+
func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error {
9395
nginxCfg := cnf.generateNginxCfgForMergeableIngresses(mergeableIngs)
9496
name := objectMetaToFileName(&mergeableIngs.Master.Ingress.ObjectMeta)
9597
content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg)
@@ -962,10 +964,13 @@ func (cnf *Configurator) DeleteIngress(key string) error {
962964

963965
// UpdateEndpoints updates endpoints in NGINX configuration for the Ingress resource
964966
func (cnf *Configurator) UpdateEndpoints(ingEx *IngressEx) error {
965-
cnf.addOrUpdateIngress(ingEx)
967+
err := cnf.addOrUpdateIngress(ingEx)
968+
if err != nil {
969+
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
970+
}
966971

967972
if cnf.isPlus() {
968-
err := cnf.updatePlusEndpoints(ingEx)
973+
err = cnf.updatePlusEndpoints(ingEx)
969974
if err == nil {
970975
return nil
971976
}
@@ -980,10 +985,12 @@ func (cnf *Configurator) UpdateEndpoints(ingEx *IngressEx) error {
980985

981986
// UpdateEndpointsMergeableIngress updates endpoints in NGINX configuration for a mergeable Ingress resource
982987
func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergeableIngs *MergeableIngresses) error {
983-
cnf.addOrUpdateMergableIngress(mergeableIngs)
988+
err := cnf.addOrUpdateMergeableIngress(mergeableIngs)
989+
if err != nil {
990+
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
991+
}
984992

985993
if cnf.isPlus() {
986-
var err error
987994
for _, ing := range mergeableIngs.Minions {
988995
err = cnf.updatePlusEndpoints(ing)
989996
if err != nil {
@@ -1142,7 +1149,7 @@ func (cnf *Configurator) UpdateConfig(config *Config, ingExes []*IngressEx, merg
11421149
}
11431150
}
11441151
for _, mergeableIng := range mergeableIngs {
1145-
if err := cnf.addOrUpdateMergableIngress(mergeableIng); err != nil {
1152+
if err := cnf.addOrUpdateMergeableIngress(mergeableIng); err != nil {
11461153
return err
11471154
}
11481155
}

‎nginx-controller/nginx/configurator_test.go

Lines changed: 165 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -482,16 +482,39 @@ func createExpectedConfigForMergeableCafeIngress() IngressNginxConfig {
482482

483483
}
484484

485-
func createTestConfigurator() *Configurator {
486-
templateExecutor, _ := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
485+
func createTestConfigurator() (*Configurator, error) {
486+
templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
487+
if err != nil {
488+
return nil, err
489+
}
490+
ngxc := NewNginxController("/etc/nginx", true)
491+
apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true)
492+
if err != nil {
493+
return nil, err
494+
}
495+
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor), nil
496+
}
497+
498+
func createTestConfiguratorInvalidIngressTemplate() (*Configurator, error) {
499+
templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
500+
if err != nil {
501+
return nil, err
502+
}
503+
invalidIngressTemplate := "{{.Upstreams.This.Field.Does.Not.Exist}}"
504+
if err := templateExecutor.UpdateIngressTemplate(&invalidIngressTemplate); err != nil {
505+
return nil, err
506+
}
487507
ngxc := NewNginxController("/etc/nginx", true)
488508
apiCtrl, _ := plus.NewNginxAPIController(&http.Client{}, "", true)
489-
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor)
509+
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor), nil
490510
}
491511

492512
func TestGenerateNginxCfg(t *testing.T) {
493513
cafeIngressEx := createCafeIngressEx()
494-
cnf := createTestConfigurator()
514+
cnf, err := createTestConfigurator()
515+
if err != nil {
516+
t.Errorf("Failed to create a test configurator: %v", err)
517+
}
495518
expected := createExpectedConfigForCafeIngressEx()
496519

497520
pems := map[string]string{
@@ -514,7 +537,10 @@ func TestGenerateNginxCfgForJWT(t *testing.T) {
514537
cafeIngressEx.Ingress.Annotations["nginx.com/jwt-login-url"] = "https://login.example.com"
515538
cafeIngressEx.JWTKey = &api_v1.Secret{}
516539

517-
cnf := createTestConfigurator()
540+
cnf, err := createTestConfigurator()
541+
if err != nil {
542+
t.Errorf("Failed to create a test configurator: %v", err)
543+
}
518544
expected := createExpectedConfigForCafeIngressEx()
519545
expected.Servers[0].JWTAuth = &JWTAuth{
520546
Key: "/etc/nginx/secrets/default-cafe-jwk",
@@ -547,7 +573,10 @@ func TestGenerateNginxCfgForMergeableIngresses(t *testing.T) {
547573
mergeableIngresses := createMergeableCafeIngress()
548574
expected := createExpectedConfigForMergeableCafeIngress()
549575

550-
cnf := createTestConfigurator()
576+
cnf, err := createTestConfigurator()
577+
if err != nil {
578+
t.Errorf("Failed to create a test configurator: %v", err)
579+
}
551580

552581
result := cnf.generateNginxCfgForMergeableIngresses(mergeableIngresses)
553582

@@ -604,7 +633,10 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) {
604633
},
605634
}
606635

607-
cnf := createTestConfigurator()
636+
cnf, err := createTestConfigurator()
637+
if err != nil {
638+
t.Errorf("Failed to create a test configurator: %v", err)
639+
}
608640

609641
result := cnf.generateNginxCfgForMergeableIngresses(mergeableIngresses)
610642

@@ -618,3 +650,129 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) {
618650
t.Errorf("generateNginxCfgForMergeableIngresses returned \n%v, but expected \n%v", result.Servers[0].JWTRedirectLocations, expected.Servers[0].JWTRedirectLocations)
619651
}
620652
}
653+
654+
func TestAddOrUpdateIngress(t *testing.T) {
655+
cnf, err := createTestConfigurator()
656+
if err != nil {
657+
t.Errorf("Failed to create a test configurator: %v", err)
658+
}
659+
ingress := createCafeIngressEx()
660+
err = cnf.AddOrUpdateIngress(&ingress)
661+
if err != nil {
662+
t.Errorf("AddOrUpdateIngress returned: \n%v, but expected: \n%v", err, nil)
663+
}
664+
665+
cnfHasIngress := cnf.HasIngress(ingress.Ingress)
666+
if !cnfHasIngress {
667+
t.Errorf("AddOrUpdateIngress didn't add ingress successfully. HasIngress returned %v, expected %v", cnfHasIngress, true)
668+
}
669+
}
670+
671+
func TestAddOrUpdateMergeableIngress(t *testing.T) {
672+
cnf, err := createTestConfigurator()
673+
if err != nil {
674+
t.Errorf("Failed to create a test configurator: %v", err)
675+
}
676+
mergeableIngess := createMergeableCafeIngress()
677+
err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
678+
if err != nil {
679+
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, expected \n%v", err, nil)
680+
}
681+
682+
cnfHasMergeableIngress := cnf.HasIngress(mergeableIngess.Master.Ingress)
683+
if !cnfHasMergeableIngress {
684+
t.Errorf("AddOrUpdateMergeableIngress didn't add mergeable ingress successfully. HasIngress returned %v, expected %v", cnfHasMergeableIngress, true)
685+
}
686+
}
687+
688+
func TestAddOrUpdateIngressFailsWithInvalidIngressTemplate(t *testing.T) {
689+
cnf, err := createTestConfiguratorInvalidIngressTemplate()
690+
if err != nil {
691+
t.Errorf("Failed to create a test configurator: %v", err)
692+
}
693+
694+
ingress := createCafeIngressEx()
695+
err = cnf.AddOrUpdateIngress(&ingress)
696+
if err == nil {
697+
t.Errorf("AddOrUpdateIngressFailsWithInvalidTemplate returned \n%v, but expected \n%v", nil, "template execution error")
698+
}
699+
}
700+
701+
func TestAddOrUpdateMergeableIngressFailsWithInvalidIngressTemplate(t *testing.T) {
702+
cnf, err := createTestConfiguratorInvalidIngressTemplate()
703+
if err != nil {
704+
t.Errorf("Failed to create a test configurator: %v", err)
705+
}
706+
707+
mergeableIngess := createMergeableCafeIngress()
708+
err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
709+
if err == nil {
710+
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error")
711+
}
712+
}
713+
714+
func TestUpdateEndpoints(t *testing.T) {
715+
cnf, err := createTestConfigurator()
716+
if err != nil {
717+
t.Errorf("Failed to create a test configurator: %v", err)
718+
}
719+
720+
ingress := createCafeIngressEx()
721+
err = cnf.UpdateEndpoints(&ingress)
722+
if err != nil {
723+
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", err, nil)
724+
}
725+
726+
// test with OSS Configurator
727+
cnf.nginxAPI = nil
728+
err = cnf.UpdateEndpoints(&ingress)
729+
if err != nil {
730+
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", err, nil)
731+
}
732+
}
733+
734+
func TestUpdateEndpointsMergeableIngress(t *testing.T) {
735+
cnf, err := createTestConfigurator()
736+
if err != nil {
737+
t.Errorf("Failed to create a test configurator: %v", err)
738+
}
739+
740+
mergeableIngress := createMergeableCafeIngress()
741+
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
742+
if err != nil {
743+
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", err, nil)
744+
}
745+
746+
// test with OSS Configurator
747+
cnf.nginxAPI = nil
748+
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
749+
if err != nil {
750+
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", err, nil)
751+
}
752+
}
753+
754+
func TestUpdateEndpointsFailsWithInvalidTemplate(t *testing.T) {
755+
cnf, err := createTestConfiguratorInvalidIngressTemplate()
756+
if err != nil {
757+
t.Errorf("Failed to create a test configurator: %v", err)
758+
}
759+
760+
ingress := createCafeIngressEx()
761+
err = cnf.UpdateEndpoints(&ingress)
762+
if err == nil {
763+
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", nil, "template execution error")
764+
}
765+
}
766+
767+
func TestUpdateEndpointsMergeableIngressFailsWithInvalidTemplate(t *testing.T) {
768+
cnf, err := createTestConfiguratorInvalidIngressTemplate()
769+
if err != nil {
770+
t.Errorf("Failed to create a test configurator: %v", err)
771+
}
772+
773+
mergeableIngress := createMergeableCafeIngress()
774+
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
775+
if err == nil {
776+
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error")
777+
}
778+
}

0 commit comments

Comments
 (0)