> Don't reload nginx when L4 endpoints changed

[yowenter]

What this PR does / why we need it:

Since we use lua upstream for L4 service balancer. We don't need reload nginx when L4 service endpoints changed.

[yowenter]

#3374

@aledbf

[yowenter]

/assign @ElvinEfendi

[yowenter]

Hi, @ElvinEfendi ,
As we can see the code below, we dynamic reload tcp endpoints and udp endpoints.
So, I think we don't need reload nginx.conf by set the pcfg.endpoints.
But, I can't figure out why the e2e test failed.

https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/nginx.go#L780

	for _, ep := range pcfg.TCPEndpoints {
		var service *apiv1.Service
		if ep.Service != nil {
			service = &apiv1.Service{Spec: ep.Service.Spec}
		}

		key := fmt.Sprintf("tcp-%v-%v-%v", ep.Backend.Namespace, ep.Backend.Name, ep.Backend.Port.String())
		streams = append(streams, ingress.Backend{
			Name:      key,
			Endpoints: ep.Endpoints,
			Port:      intstr.FromInt(ep.Port),
			Service:   service,
		})
	}
	for _, ep := range pcfg.UDPEndpoints {
		var service *apiv1.Service
		if ep.Service != nil {
			service = &apiv1.Service{Spec: ep.Service.Spec}
		}

		key := fmt.Sprintf("udp-%v-%v-%v", ep.Backend.Namespace, ep.Backend.Name, ep.Backend.Port.String())
		streams = append(streams, ingress.Backend{
			Name:      key,
			Endpoints: ep.Endpoints,
			Port:      intstr.FromInt(ep.Port),
			Service:   service,
		})
	}

	err = updateStreamConfiguration(streams)

[ElvinEfendi]

Hi @yowenter, the test fails at

ingress-nginx/test/e2e/tcpudp/tcp.go

Line 84 in d74dea7

return strings.Contains(cfg, fmt.Sprintf(`ngx.var.proxy_upstream_name="tcp-%v-http-svc-80"`, f.IngressController.Namespace))

.

I don't know offhand why it's failing but my guess is because of your changes in this PR, controller does not regenerate a new Nginx configuration.

I hope that gives you some starting point to debug - sorry I don't have much time now to dig into this deeper. But your PR makes sense in general!

[yowenter]

Thanks for your information, I'll fix it soon. Sent by my pixel 2

…

On Tue, Feb 19, 2019, 12:01 Elvin Efendi ***@***.*** wrote: Hi @yowenter <https://github.com/yowenter>, the test fails at https://github.com/kubernetes/ingress-nginx/blob/d74dea7585b7b26cf5a16ca9d7ac402b1e0cf8df/test/e2e/tcpudp/tcp.go#L84 . I don't know offhand why it's failing but my guess is because of your changes in this PR, controller does not regenerate a new Nginx configuration. I hope that gives you some starting point to debug - sorry I don't have much time now to dig into this deeper. But your PR makes sense in general! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3695 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGWWtgPmwhU3eQ0Xf8ymTxtvqRqfuD4zks5vO3c1gaJpZM4aQ73E> .

[yowenter]

@ElvinEfendi I've fixed the test.

    # TCP services
    {{ range $tcpServer := .TCPBackends }}
    server {
        preread_by_lua_block {
            ngx.var.proxy_upstream_name="tcp-{{ $tcpServer.Backend.Namespace }}-{{ $tcpServer.Backend.Name }}-{{ $tcpServer.Backend.Port }}";
        }

        {{ range $address := $all.Cfg.BindAddressIpv4 }}
        listen                  {{ $address }}:{{ $tcpServer.Port }}{{ if $tcpServer.Backend.ProxyProtocol.Decode }} proxy_protocol{{ end }};
        {{ else }}
        listen                  {{ $tcpServer.Port }}{{ if $tcpServer.Backend.ProxyProtocol.Decode }} proxy_protocol{{ end }};
        {{ end }}
        {{ if $IsIPV6Enabled }}
        {{ range $address := $all.Cfg.BindAddressIpv6 }}
        listen                  {{ $address }}:{{ $tcpServer.Port }}{{ if $tcpServer.Backend.ProxyProtocol.Decode }} proxy_protocol{{ end }};
        {{ else }}
        listen                  [::]:{{ $tcpServer.Port }}{{ if $tcpServer.Backend.ProxyProtocol.Decode }} proxy_protocol{{ end }};
        {{ end }}
        {{ end }}
        proxy_timeout           {{ $cfg.ProxyStreamTimeout }};
        proxy_pass              upstream_balancer;
        {{ if $tcpServer.Backend.ProxyProtocol.Encode }}
        proxy_protocol          on;
        {{ end }}
    }
    {{ end }}

The nginx use TCPBackends for L4 services.
Since we use lua tcp_udp_balancer.lua, we don't need reload nginx.conf when service tcp endpoints change.
We just need reload nginx.conf only when the tcpServer.Backend change.

[ElvinEfendi]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, yowenter

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment