[nginx-ingress-controller] Add support for default backend in Ingress rule #1830
Conversation
k8s-github-robot
added
the
cncf-cla: yes
aledbf
force-pushed
the
dbackend
branch
3 times, most recently
from
e621db8 to
6d1749b
Compare
|
[CLA-PING] @aledbf Thanks for your pull request. It looks like this may be your first contribution to a CNCF open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://identity.linuxfoundation.org/projects/cncf to sign. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
|
k8s-github-robot
added
cncf-cla: no
|
@bprashanth ready for a first review |
| @@ -739,19 +740,38 @@ func (lbc *loadBalancerController) getUpstreamServers(ngxCfg config.Configuratio | |||
| server = servers[defServerName] | |||
| } | |||
|
|
|||
| if rule.HTTP == nil && host != defServerName { | |||
| glog.Info("multiple root ISSUE") | |||
There was a problem hiding this comment.
maybe send an event, and turn the message into a WARNING or ERROR? Please either extend the message itself or add a comment.
There was a problem hiding this comment.
This is actually worth documenting. We just enforce that if there're multiple ingresses asking for a default backend, they must have the same backend? do we pick this backed on fcfs basis?
| nginxPath := path.Path | ||
| // if there's no path defined we assume / | ||
| if nginxPath == "" { | ||
| lbc.recorder.Eventf(ing, api.EventTypeWarning, "MAPPING", | ||
| "Ingress rule '%v/%v' contains no path definition. Assuming /", ing.GetNamespace(), ing.GetName()) | ||
| "Ingress rule '%v/%v' contains no path definition. Assuming /", |
There was a problem hiding this comment.
/ is the same as /* right? do you lose anything by just making it the latter (more explicit)
| nginxPath = rootLocation | ||
| } | ||
|
|
||
| // Validate that there is no another previuous | ||
| // Validate that there is no another previous |
| } | ||
|
|
||
| if !svcExists { | ||
| return upstreams, fmt.Errorf("service %v does not exists", svcKey) |
There was a problem hiding this comment.
do these translate into events? (the gce controller has the same problem of not sending an event for this case, pretty confusing to debug)
There was a problem hiding this comment.
no but this appears in the log of the pod
| @@ -128,7 +128,10 @@ func (ngx Manager) testTemplate(cfg []byte) error { | |||
| return err | |||
| } | |||
| defer tmpfile.Close() | |||
| defer os.Remove(tmpfile.Name()) | |||
| // do not remove temporal nginx.conf file if log level is >= 3 | |||
| if !glog.V(3) { | |||
There was a problem hiding this comment.
hmm, why? this looks like a weird lateral dependency. are we just leaking the file to debug later?
There was a problem hiding this comment.
change the behavior to remove the file only if there's no error running the check
aledbf
changed the title
|
LGTM sorry for the delay |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue [nginx-ingress-controller] Add support for default backend in Ingress rule replaces kubernetes-retired#1759
replaces #1759
This change is