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

Jump to bottom

Use whitelist-source-range from configmap when no annotation on ingress. #517

Merged
merged 1 commit into from
Mar 29, 2017
+94 −7
Merged

Use whitelist-source-range from configmap when no annotation on ingress. #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions core/pkg/ingress/annotations/ipwhitelist/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ func (a ipwhitelist) Parse(ing *extensions.Ingress) (interface{}, error) {
sort.Strings(defBackend.WhitelistSourceRange)

val, err := parser.GetStringAnnotation(whitelist, ing)
if err != nil {
return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, err
// A missing annotation is not a problem, just use the default
Copy link
Member

Choose a reason for hiding this comment

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

please change all this section to something like

	if err == ing_errors.ErrMissingAnnotations {
		return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, nil
	}

Copy link
Contributor Author

@ashb ashb Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean get rid of the else if err != nil block?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated (and force pushed since it's such a small change)

if err == ing_errors.ErrMissingAnnotations {
return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

this broke my annotation "ingress.kubernetes.io/whitelist-source-range", should this return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, err ?

#558

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&SourceRange{CIDR: defBackend.WhitelistSourceRange}, err is what it used to return, and the value was ignored because of the error, so I don't think so. But I'm not sure what's causing the issue.

Copy link
Member

Choose a reason for hiding this comment

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

@gianrubio this error is returned only when there is no annotation. Can you run the controller with --v=3?

Copy link
Contributor

@gianrubio gianrubio Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@aledbf @ashb This code fix my issue, sorry but I'm not sure if it's easy to read

if err != nil{
		if err == ing_errors.ErrMissingAnnotations &&
			len(defBackend.WhitelistSourceRange) > 0 {
			return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, nil
		}
		return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, err
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gianrubio That seems like an odd fix because the template already has that exact conditional https://github.com/kubernetes/ingress/blob/7ca7652ab26e1a5775f3066f53f28d5ea5eb3bb7/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl#L285:

            {{ if gt (len $location.Whitelist.CIDR) 0 }}
            {{ range $ip := $location.Whitelist.CIDR }}
            allow {{ $ip }};{{ end }}
            deny all;
            {{ end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

@ashb I was wrong, the issue is related to 8552351 #527

@jcmoraisjr could you help me?

Why are you doing this if we already mergeLocationAnnotations on https://github.com/kubernetes/ingress/blob/8552351af0d5b919fd581c978c1c211b02006443/core/pkg/ingress/controller/controller.go#L667:L679

// Add annotations to location of default backend (root context)
			if len(server.Locations) > 0 {
				loc := server.Locations[0]
				mergeLocationAnnotations(loc, anns)
			}

This is breaking #558

Copy link
Contributor

Choose a reason for hiding this comment

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

@gianrubio Those mergeLocationAnnotations doesn't configure the implicit default backend without an explicit path. This is used eg by app-root. I can fix this issue rewriting #527 in a proper way - merging only annotations that are relevant to the root context. I can create another PR in a couple of hours. And sorry about the mess.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries :) Let me know when you fix so I can test before merging

}

values := strings.Split(val, ",")
Expand Down
96 changes: 91 additions & 5 deletions core/pkg/ingress/annotations/ipwhitelist/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"k8s.io/kubernetes/pkg/util/intstr"

"k8s.io/ingress/core/pkg/ingress/defaults"
"k8s.io/ingress/core/pkg/ingress/errors"
)

func buildIngress() *extensions.Ingress {
Expand Down Expand Up @@ -63,10 +64,11 @@ func buildIngress() *extensions.Ingress {
}

type mockBackend struct {
defaults.Backend
}

func (m mockBackend) GetDefaultBackend() defaults.Backend {
return defaults.Backend{}
return m.Backend
}

func TestParseAnnotations(t *testing.T) {
Expand Down Expand Up @@ -105,16 +107,21 @@ func TestParseAnnotations(t *testing.T) {
t.Errorf("expected error parsing an invalid cidr")
}

if !errors.IsLocationDenied(err) {
t.Errorf("expected LocationDenied error: %+v", err)
}

delete(data, whitelist)
ing.SetAnnotations(data)
i, err = p.Parse(ing)

if err != nil {
t.Errorf("unexpected error when no annotation present: %v", err)
}

sr, ok = i.(*SourceRange)
if !ok {
t.Errorf("expected a SourceRange type")
}
if err == nil {
t.Errorf("expected error parsing an invalid cidr")
}
if !strsEquals(sr.CIDR, []string{}) {
t.Errorf("expected empty CIDR but %v returned", sr.CIDR)
}
Expand All @@ -140,6 +147,85 @@ func TestParseAnnotations(t *testing.T) {
}
}

// Test that when we have a whitelist set on the Backend that is used when we
// don't have the annotation
func TestParseAnnotationsWithDefaultConfig(t *testing.T) {
// TODO: convert test cases to tables
ing := buildIngress()

mockBackend := mockBackend{}
mockBackend.Backend.WhitelistSourceRange = []string{"4.4.4.0/24", "1.2.3.4/32"}
testNet := "10.0.0.0/24"
enet := []string{testNet}

data := map[string]string{}
data[whitelist] = testNet
ing.SetAnnotations(data)

expected := &SourceRange{
CIDR: enet,
}

p := NewParser(mockBackend)

i, err := p.Parse(ing)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
sr, ok := i.(*SourceRange)
if !ok {
t.Errorf("expected a SourceRange type")
}

if !reflect.DeepEqual(sr, expected) {
t.Errorf("expected %v but returned %s", sr, expected)
}

data[whitelist] = "www"
_, err = p.Parse(ing)
if err == nil {
t.Errorf("expected error parsing an invalid cidr")
}
if !errors.IsLocationDenied(err) {
t.Errorf("expected LocationDenied error: %+v", err)
}

delete(data, whitelist)
i, err = p.Parse(ing)

if err != nil {
t.Errorf("unexpected error when no annotation present: %v", err)
}

sr, ok = i.(*SourceRange)
if !ok {
t.Errorf("expected a SourceRange type")
}
if !strsEquals(sr.CIDR, mockBackend.WhitelistSourceRange) {
t.Errorf("expected fallback CIDR but %v returned", sr.CIDR)
}

i, _ = p.Parse(&extensions.Ingress{})
sr, ok = i.(*SourceRange)
if !ok {
t.Errorf("expected a SourceRange type")
}
if !strsEquals(sr.CIDR, mockBackend.WhitelistSourceRange) {
t.Errorf("expected fallback CIDR but %v returned", sr.CIDR)
}

data[whitelist] = "2.2.2.2/32,1.1.1.1/32,3.3.3.0/24"
i, _ = p.Parse(ing)
sr, ok = i.(*SourceRange)
if !ok {
t.Errorf("expected a SourceRange type")
}
ecidr := []string{"1.1.1.1/32", "2.2.2.2/32", "3.3.3.0/24"}
if !strsEquals(sr.CIDR, ecidr) {
t.Errorf("Expected %v CIDR but %v returned", ecidr, sr.CIDR)
}
}

func strsEquals(a, b []string) bool {
if len(a) != len(b) {
return false
Expand Down