Use whitelist-source-range from configmap when no annotation on ingress. #517
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
| @@ -56,8 +56,16 @@ 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 | |||
There was a problem hiding this comment.
please change all this section to something like
if err == ing_errors.ErrMissingAnnotations {
return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, nil
}
There was a problem hiding this comment.
Do you mean get rid of the else if err != nil block?
There was a problem hiding this comment.
Updated (and force pushed since it's such a small change)
Even though we were returning a SourceRange it was being ignored because we were also returning an error. Detect the case (and add tests) when the annotation is not present and use the BackendConfig in that case. Fixes #473.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@ashb thanks! |
| return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, err | ||
| // A missing annotation is not a problem, just use the default | ||
| if err == ing_errors.ErrMissingAnnotations { | ||
| return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, nil |
There was a problem hiding this comment.
this broke my annotation "ingress.kubernetes.io/whitelist-source-range", should this return &SourceRange{CIDR: defBackend.WhitelistSourceRange}, err ?
There was a problem hiding this comment.
&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.
There was a problem hiding this comment.
@gianrubio this error is returned only when there is no annotation. Can you run the controller with --v=3?
There was a problem hiding this comment.
@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
}
There was a problem hiding this comment.
@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 }}
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
No worries :) Let me know when you fix so I can test before merging
Even though we were returning a SourceRange it was being ignored because we were also returning an error. Detect the case (and add tests) when the annotation is not present and use the BackendConfig in that case.
fixes #473
fixes #312