-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also assert that "service" does not exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a no-op change or addressing an actual issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comparing a valid instance against null. |
||
|
||
if asslc1.Secret != assl2.Secret { | ||
return false | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'm already doing it at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165L55 to test the other function.
👍 for not exporting this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to make sure we don't have targets in the endpoints. This is the biggest object in the json request
There was a problem hiding this comment.
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.