Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Configurator error check #348

Merged
merged 5 commits into from
Aug 24, 2018
Merged

Fix Configurator error check #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e117ace
Fix uncheckedConfigurator errors
Dean-Coakley Aug 10, 2018
dd0ab26
Add Unit tests for Add/Update Ingress
Dean-Coakley Aug 10, 2018
fe6bf81
Fix MergeableIngress unit test. Fix typos
Dean-Coakley Aug 22, 2018
6051d2b
Add unit tests for updating endpoints
Dean-Coakley Aug 23, 2018
00d452f
Fix mergeableIngress typo
Dean-Coakley Aug 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions nginx-controller/controller/controller.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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")
}
}