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.

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

Clean JSON before post request to update configuration #2365

Merged
merged 1 commit into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func (n *NGINXController) syncIngress(interface{}) error {
// it takes time for Nginx to start listening on the port
time.Sleep(1 * time.Second)
}
err := n.ConfigureDynamically(&pcfg)
err := configureDynamically(&pcfg, n.cfg.ListenPorts.Status)
if err == nil {
glog.Infof("dynamic reconfiguration succeeded")
} else {
Expand Down
1 change: 0 additions & 1 deletion internal/ingress/controller/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ func getEndpoints(
targetPort = epPort.Port
}

glog.Infof("TP: %v", targetPort)
// check for invalid port value
if targetPort <= 0 {
continue
Expand Down
38 changes: 28 additions & 10 deletions internal/ingress/controller/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"net/http"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"sync"
Expand All @@ -44,8 +45,6 @@ import (
"k8s.io/client-go/util/flowcontrol"
"k8s.io/kubernetes/pkg/util/filesystem"

"path/filepath"

"k8s.io/ingress-nginx/internal/file"
"k8s.io/ingress-nginx/internal/ingress"
"k8s.io/ingress-nginx/internal/ingress/annotations"
Expand Down Expand Up @@ -752,24 +751,43 @@ func (n *NGINXController) setupSSLProxy() {

// IsDynamicConfigurationEnough decides if the new configuration changes can be dynamically applied without reloading
func (n *NGINXController) IsDynamicConfigurationEnough(pcfg *ingress.Configuration) bool {
var copyOfRunningConfig ingress.Configuration = *n.runningConfig
var copyOfPcfg ingress.Configuration = *pcfg
copyOfRunningConfig := *n.runningConfig
copyOfPcfg := *pcfg

copyOfRunningConfig.Backends = []*ingress.Backend{}
copyOfPcfg.Backends = []*ingress.Backend{}

return copyOfRunningConfig.Equal(&copyOfPcfg)
}

// ConfigureDynamically JSON encodes new Backends and POSTs it to an internal HTTP endpoint
// configureDynamically JSON encodes new Backends and POSTs it to an internal HTTP endpoint
// that is handled by Lua
func (n *NGINXController) ConfigureDynamically(pcfg *ingress.Configuration) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you changing this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be able to test it without creating a new NGINXController instance.
Also removes an export we should not expose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be able to test it without creating a new NGINXController instance.

Is this a big deal though? I'm already doing it at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165L55 to test the other function.

Also removes an export we should not expose.

👍 for not exporting this function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a big deal though? I'

I just want to make sure we don't have targets in the endpoints. This is the biggest object in the json request

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, I was commenting on "to be able to test it without creating a new NGINXController instance." - it is a few lines of code to create a new NGINXController instance in test.

func configureDynamically(pcfg *ingress.Configuration, port int) error {
backends := make([]*ingress.Backend, len(pcfg.Backends))

for i, backend := range pcfg.Backends {
cleanedupBackend := *backend
cleanedupBackend.Service = nil
backends[i] = &cleanedupBackend
luaBackend := &ingress.Backend{
Name: backend.Name,
Port: backend.Port,
Secure: backend.Secure,
SSLPassthrough: backend.SSLPassthrough,
SessionAffinity: backend.SessionAffinity,
UpstreamHashBy: backend.UpstreamHashBy,
LoadBalancing: backend.LoadBalancing,
}

var endpoints []ingress.Endpoint
for _, endpoint := range backend.Endpoints {
endpoints = append(endpoints, ingress.Endpoint{
Address: endpoint.Address,
FailTimeout: endpoint.FailTimeout,
MaxFails: endpoint.MaxFails,
Port: endpoint.Port,
})
}

luaBackend.Endpoints = endpoints
backends[i] = luaBackend
}

buf, err := json.Marshal(backends)
Expand All @@ -779,7 +797,7 @@ func (n *NGINXController) ConfigureDynamically(pcfg *ingress.Configuration) erro

glog.V(2).Infof("posting backends configuration: %s", buf)

url := fmt.Sprintf("http://localhost:%d/configuration/backends", n.cfg.ListenPorts.Status)
url := fmt.Sprintf("http://localhost:%d/configuration/backends", port)
resp, err := http.Post(url, "application/json", bytes.NewReader(buf))
if err != nil {
return err
Expand Down
86 changes: 86 additions & 0 deletions internal/ingress/controller/nginx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,15 @@ limitations under the License.
package controller

import (
"io"
"io/ioutil"
"net"
"net/http"
"net/http/httptest"
"strings"
"testing"

apiv1 "k8s.io/api/core/v1"
"k8s.io/ingress-nginx/internal/ingress"
)

Expand Down Expand Up @@ -89,6 +96,85 @@ func TestIsDynamicConfigurationEnough(t *testing.T) {
}
}

func TestConfigureDynamically(t *testing.T) {
target := &apiv1.ObjectReference{}

backends := []*ingress.Backend{{
Name: "fakenamespace-myapp-80",
Service: &apiv1.Service{},
Endpoints: []ingress.Endpoint{
{
Address: "10.0.0.1",
Port: "8080",
Target: target,
},
{
Address: "10.0.0.2",
Port: "8080",
Target: target,
},
},
}}

servers := []*ingress.Server{{
Hostname: "myapp.fake",
Locations: []*ingress.Location{
{
Path: "/",
Backend: "fakenamespace-myapp-80",
Service: &apiv1.Service{},
},
},
}}

commonConfig := &ingress.Configuration{
Backends: backends,
Servers: servers,
}

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusCreated)

if r.Method != "POST" {
t.Errorf("expected a 'POST' request, got '%s'", r.Method)
}

b, err := ioutil.ReadAll(r.Body)
if err != nil && err != io.EOF {
t.Fatal(err)
}
body := string(b)
if strings.Index(body, "target") != -1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also assert that "service" does not exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

t.Errorf("unexpected target reference in JSON content: %v", body)
}

if strings.Index(body, "service") != -1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you include a dummy service object for the backend at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165R99 as well? so that this assertion becomes more meaningful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

t.Errorf("unexpected service reference in JSON content: %v", body)
}

}))

listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Errorf("unexpected error listening on a random port: %v", err)
}
defer listener.Close()

port := listener.Addr().(*net.TCPAddr).Port

ts.Listener = listener
defer ts.Close()

err = configureDynamically(commonConfig, port)
if err != nil {
t.Errorf("unexpected error posting dynamic configuration: %v", err)
}

if commonConfig.Backends[0].Endpoints[0].Target != target {
t.Errorf("unexpected change in the configuration object after configureDynamically invocation")
}
}

func TestNginxHashBucketSize(t *testing.T) {
tests := []struct {
n int
Expand Down
7 changes: 7 additions & 0 deletions internal/ingress/resolver/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ type AuthSSLCert struct {

// Equal tests for equality between two AuthSSLCert types
func (asslc1 *AuthSSLCert) Equal(assl2 *AuthSSLCert) bool {
if asslc1 == assl2 {
return true
}
if asslc1 == nil || assl2 == nil {
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a no-op change or addressing an actual issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressing an actual issue?

Comparing a valid instance against null.


if asslc1.Secret != assl2.Secret {
return false
}
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/lua/dynamic_configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
Expect(err).NotTo(HaveOccurred())

// give some time for Lua to sync the backend
time.Sleep(2 * time.Second)
time.Sleep(5 * time.Second)

resp, _, errs := gorequest.New().
Get(f.IngressController.HTTPURL).
Expand Down