diff --git a/nginx-controller/nginx/configurator.go b/nginx-controller/nginx/configurator.go index 48d6c10405..f31296cf8d 100644 --- a/nginx-controller/nginx/configurator.go +++ b/nginx-controller/nginx/configurator.go @@ -57,11 +57,10 @@ 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 { - err := cnf.addOrUpdateIngress(ingEx) - if err != nil { + 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 { + if err := cnf.nginx.Reload(); err != nil { return fmt.Errorf("Error reloading NGINX for %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) } return nil @@ -83,17 +82,16 @@ func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error { // AddOrUpdateMergableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types func (cnf *Configurator) AddOrUpdateMergableIngress(mergeableIngs *MergeableIngresses) error { - err := cnf.addOrUpdateMergableIngress(mergeableIngs) - if err != nil { + 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 { + 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) @@ -881,10 +879,7 @@ func upstreamMapToSlice(upstreams map[string]Upstream) []Upstream { func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) error { cnf.addOrUpdateSecret(secret) - kind, err := GetSecretKind(secret) - if err != nil { - return fmt.Errorf("Error secret is unknown") - } + kind, _ := GetSecretKind(secret) if cnf.isPlus() && kind == JWK { return nil } @@ -990,7 +985,7 @@ 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 { - err := 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) } @@ -1154,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 } } diff --git a/nginx-controller/nginx/configurator_test.go b/nginx-controller/nginx/configurator_test.go index c453b04b9d..997362b47e 100644 --- a/nginx-controller/nginx/configurator_test.go +++ b/nginx-controller/nginx/configurator_test.go @@ -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{ @@ -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", @@ -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) @@ -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) @@ -620,40 +652,61 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { } func TestAddOrUpdateIngress(t *testing.T) { - cnf := createTestConfigurator() - validIngress := createCafeIngressEx() - err := cnf.AddOrUpdateIngress(&validIngress) + 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 TestAddOrUpdateMergableIngress(t *testing.T) { + cnf, err := createTestConfigurator() + if err != nil { + t.Errorf("Failed to create a test configurator: %v", err) + } + mergeableIngess := createMergeableCafeIngress() + err = cnf.AddOrUpdateMergableIngress(mergeableIngess) if err != nil { - t.Errorf("Adding ingress failed: %v", err) + t.Errorf("AddOrUpdateMergableIngress returned \n%v, expected \n%v", err, nil) } - expectedIng := cnf.ingresses["default-cafe-ingress"] - if !reflect.DeepEqual(&validIngress, expectedIng) { - t.Errorf("TestAddOrUpdateIngress returned: \n%v, but expected: \n%v", validIngress, expectedIng) + cnfHasMergableIngress := cnf.HasIngress(mergeableIngess.Master.Ingress) + if !cnfHasMergableIngress { + t.Errorf("AddOrUpdateMergableIngress didn't add mergable ingress successfully. HasIngress returned %v, expected %v", cnfHasMergableIngress, true) } } -func TestAddOrUpdateMIngress(t *testing.T) { - cnf := createTestConfigurator() - validMergeableIngress := createMergeableCafeIngress() - err := cnf.AddOrUpdateMergableIngress(validMergeableIngress) +func TestAddOrUpdateIngressFailsWithInvalidIngressTemplate(t *testing.T) { + cnf, err := createTestConfiguratorInvalidIngressTemplate() if err != nil { - t.Errorf("Adding mergable ingress failed: %v", err) + t.Errorf("Failed to create a test configurator: %v", err) } - expectedIng := cnf.ingresses["default-cafe-ingress-master"] - if !reflect.DeepEqual(validMergeableIngress.Master, expectedIng) { - t.Errorf("TestAddOrUpdateMergableIngress returned: \n%v, but expected: \n%v", validMergeableIngress.Master, expectedIng) + ingress := createCafeIngressEx() + err = cnf.AddOrUpdateIngress(&ingress) + if err == nil { + t.Errorf("AddOrUpdateIngressFailsWithInvalidTemplate returned \n%v, but expected \n%v", nil, "template execution error") } } -func TestAddOrUpdateIngressFailsWithInvalidTemplate(t *testing.T) { - t.Skip() // Skip test until TODO is completed - cnf := createTestConfigurator() - validIngress := createCafeIngressEx() - // TODO: Alter template to be invalid - err := cnf.AddOrUpdateIngress(&validIngress) +func TestAddOrUpdateMergableIngressFailsWithInvalidIngressTemplate(t *testing.T) { + cnf, err := createTestConfiguratorInvalidIngressTemplate() + if err != nil { + t.Errorf("Failed to create a test configurator: %v", err) + } + + mergeableIngess := createMergeableCafeIngress() + err = cnf.AddOrUpdateMergableIngress(mergeableIngess) if err == nil { - t.Errorf("TestAddOrUpdateIngressFailsWithInvalidTemplate returned: \n%v, but expected \n template execution error", err) + t.Errorf("AddOrUpdateMergableIngress returned \n%v, expected \n%v", nil, "template execution error") } }