Skip to content

Commit

Permalink
Fix Configurator error checking (#348)
Browse files Browse the repository at this point in the history 
* Fixed unchecked Configurator errors
* Added unit tests for AddOrUpdateIngress and AddOrUpdateIngressMergeable
* Added unit tests for UpdateEndpoints and UpdateEndpointsMergeableIngress
* Fixed mergeableIngress typo
  • Loading branch information
@Dean-Coakley
Dean-Coakley authored and ismael_serrano committed Sep 5, 2018
1 parent 7ffe30e commit 8a02653
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 24 deletions.
6 changes: 3 additions & 3 deletions nginx-controller/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ func (lbc *LoadBalancerController) syncIng(task Task) {
}
return
}
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngExs)
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngExs)
if err != nil {
lbc.recorder.Eventf(ing, api_v1.EventTypeWarning, "AddedOrUpdatedWithError", "Configuration for %v(Master) was added or updated, but not applied: %v", key, err)
for _, minion := range mergeableIngExs.Minions {
Expand Down Expand Up @@ -804,7 +804,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) {
glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err)
continue
}
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngress)
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngress)
if err != nil {
glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err)
}
Expand Down Expand Up @@ -864,7 +864,7 @@ func (lbc *LoadBalancerController) syncSecret(task Task) {
glog.Errorf("Ignoring Ingress %v(Minion): %v", minion.Name, err)
continue
}
err = lbc.cnf.AddOrUpdateMergableIngress(mergeableIngress)
err = lbc.cnf.AddOrUpdateMergeableIngress(mergeableIngress)
if err != nil {
glog.Errorf("Failed to update Ingress %v(Master) of %v(Minion): %v", master.Name, minion.Name, err)
}
Expand Down
35 changes: 21 additions & 14 deletions nginx-controller/nginx/configurator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,11 @@ func (cnf *Configurator) AddOrUpdateDHParam(content string) (string, error) {

// AddOrUpdateIngress adds or updates NGINX configuration for the Ingress resource
func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) error {
cnf.addOrUpdateIngress(ingEx)

if err := cnf.addOrUpdateIngress(ingEx); err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
if err := cnf.nginx.Reload(); err != nil {
return fmt.Errorf("Error when adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
return fmt.Errorf("Error reloading NGINX for %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
return nil
}
Expand All @@ -79,17 +80,18 @@ func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error {
return nil
}

// AddOrUpdateMergableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types
func (cnf *Configurator) AddOrUpdateMergableIngress(mergeableIngs *MergeableIngresses) error {
cnf.addOrUpdateMergableIngress(mergeableIngs)

if err := cnf.nginx.Reload(); err != nil {
// AddOrUpdateMergeableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types
func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error {
if err := cnf.addOrUpdateMergeableIngress(mergeableIngs); err != nil {
return fmt.Errorf("Error when adding or updating ingress %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
}
if err := cnf.nginx.Reload(); err != nil {
return fmt.Errorf("Error reloading NGINX for %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
}
return nil
}

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

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

if cnf.isPlus() {
err := cnf.updatePlusEndpoints(ingEx)
err = cnf.updatePlusEndpoints(ingEx)
if err == nil {
return nil
}
Expand All @@ -980,10 +985,12 @@ func (cnf *Configurator) UpdateEndpoints(ingEx *IngressEx) error {

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

if cnf.isPlus() {
var err error
for _, ing := range mergeableIngs.Minions {
err = cnf.updatePlusEndpoints(ing)
if err != nil {
Expand Down Expand Up @@ -1142,7 +1149,7 @@ func (cnf *Configurator) UpdateConfig(config *Config, ingExes []*IngressEx, merg
}
}
for _, mergeableIng := range mergeableIngs {
if err := cnf.addOrUpdateMergableIngress(mergeableIng); err != nil {
if err := cnf.addOrUpdateMergeableIngress(mergeableIng); err != nil {
return err
}
}
Expand Down
172 changes: 165 additions & 7 deletions nginx-controller/nginx/configurator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,16 +482,39 @@ func createExpectedConfigForMergeableCafeIngress() IngressNginxConfig {

}

func createTestConfigurator() *Configurator {
templateExecutor, _ := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
func createTestConfigurator() (*Configurator, error) {
templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
if err != nil {
return nil, err
}
ngxc := NewNginxController("/etc/nginx", true)
apiCtrl, err := plus.NewNginxAPIController(&http.Client{}, "", true)
if err != nil {
return nil, err
}
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor), nil
}

func createTestConfiguratorInvalidIngressTemplate() (*Configurator, error) {
templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080)
if err != nil {
return nil, err
}
invalidIngressTemplate := "{{.Upstreams.This.Field.Does.Not.Exist}}"
if err := templateExecutor.UpdateIngressTemplate(&invalidIngressTemplate); err != nil {
return nil, err
}
ngxc := NewNginxController("/etc/nginx", true)
apiCtrl, _ := plus.NewNginxAPIController(&http.Client{}, "", true)
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor)
return NewConfigurator(ngxc, NewDefaultConfig(), apiCtrl, templateExecutor), nil
}

func TestGenerateNginxCfg(t *testing.T) {
cafeIngressEx := createCafeIngressEx()
cnf := createTestConfigurator()
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}
expected := createExpectedConfigForCafeIngressEx()

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

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

cnf := createTestConfigurator()
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

result := cnf.generateNginxCfgForMergeableIngresses(mergeableIngresses)

Expand Down Expand Up @@ -604,7 +633,10 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) {
},
}

cnf := createTestConfigurator()
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

result := cnf.generateNginxCfgForMergeableIngresses(mergeableIngresses)

Expand All @@ -618,3 +650,129 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) {
t.Errorf("generateNginxCfgForMergeableIngresses returned \n%v, but expected \n%v", result.Servers[0].JWTRedirectLocations, expected.Servers[0].JWTRedirectLocations)
}
}

func TestAddOrUpdateIngress(t *testing.T) {
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}
ingress := createCafeIngressEx()
err = cnf.AddOrUpdateIngress(&ingress)
if err != nil {
t.Errorf("AddOrUpdateIngress returned: \n%v, but expected: \n%v", err, nil)
}

cnfHasIngress := cnf.HasIngress(ingress.Ingress)
if !cnfHasIngress {
t.Errorf("AddOrUpdateIngress didn't add ingress successfully. HasIngress returned %v, expected %v", cnfHasIngress, true)
}
}

func TestAddOrUpdateMergeableIngress(t *testing.T) {
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}
mergeableIngess := createMergeableCafeIngress()
err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
if err != nil {
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, expected \n%v", err, nil)
}

cnfHasMergeableIngress := cnf.HasIngress(mergeableIngess.Master.Ingress)
if !cnfHasMergeableIngress {
t.Errorf("AddOrUpdateMergeableIngress didn't add mergeable ingress successfully. HasIngress returned %v, expected %v", cnfHasMergeableIngress, true)
}
}

func TestAddOrUpdateIngressFailsWithInvalidIngressTemplate(t *testing.T) {
cnf, err := createTestConfiguratorInvalidIngressTemplate()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

ingress := createCafeIngressEx()
err = cnf.AddOrUpdateIngress(&ingress)
if err == nil {
t.Errorf("AddOrUpdateIngressFailsWithInvalidTemplate returned \n%v, but expected \n%v", nil, "template execution error")
}
}

func TestAddOrUpdateMergeableIngressFailsWithInvalidIngressTemplate(t *testing.T) {
cnf, err := createTestConfiguratorInvalidIngressTemplate()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

mergeableIngess := createMergeableCafeIngress()
err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
if err == nil {
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error")
}
}

func TestUpdateEndpoints(t *testing.T) {
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

ingress := createCafeIngressEx()
err = cnf.UpdateEndpoints(&ingress)
if err != nil {
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", err, nil)
}

// test with OSS Configurator
cnf.nginxAPI = nil
err = cnf.UpdateEndpoints(&ingress)
if err != nil {
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", err, nil)
}
}

func TestUpdateEndpointsMergeableIngress(t *testing.T) {
cnf, err := createTestConfigurator()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

mergeableIngress := createMergeableCafeIngress()
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
if err != nil {
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", err, nil)
}

// test with OSS Configurator
cnf.nginxAPI = nil
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
if err != nil {
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", err, nil)
}
}

func TestUpdateEndpointsFailsWithInvalidTemplate(t *testing.T) {
cnf, err := createTestConfiguratorInvalidIngressTemplate()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

ingress := createCafeIngressEx()
err = cnf.UpdateEndpoints(&ingress)
if err == nil {
t.Errorf("UpdateEndpoints returned\n%v, but expected \n%v", nil, "template execution error")
}
}

func TestUpdateEndpointsMergeableIngressFailsWithInvalidTemplate(t *testing.T) {
cnf, err := createTestConfiguratorInvalidIngressTemplate()
if err != nil {
t.Errorf("Failed to create a test configurator: %v", err)
}

mergeableIngress := createMergeableCafeIngress()
err = cnf.UpdateEndpointsMergeableIngress(mergeableIngress)
if err == nil {
t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error")
}
}

0 comments on commit 8a02653

Please sign in to comment.