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.

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

Add --disable-catch-all option to disable catch-all server #3586

Merged
merged 1 commit into from
Jan 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions cmd/nginx/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ Feature backed by OpenResty Lua libraries. Requires that OCSP stapling is not en
sslProxyPort = flags.Int("ssl-passthrough-proxy-port", 442, `Port to use internally for SSL Passthrough.`)
defServerPort = flags.Int("default-server-port", 8181, `Port to use for exposing the default server (catch-all).`)
healthzPort = flags.Int("healthz-port", 10254, "Port to use for the healthz endpoint.")

disableCatchAll = flags.Bool("disable-catch-all", false,
`Disable support for catch-all Ingresses`)
)

flag.Set("logtostderr", "true")
Expand Down Expand Up @@ -254,6 +257,7 @@ Feature backed by OpenResty Lua libraries. Requires that OCSP stapling is not en
SSLProxy: *sslProxyPort,
Status: *statusPort,
},
DisableCatchAll: *disableCatchAll,
}

return false, config, nil
Expand Down
1 change: 1 addition & 0 deletions docs/user-guide/cli-arguments.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,4 @@ They are set in the container spec of the `nginx-ingress-controller` Deployment
| `--version` | Show release information about the NGINX Ingress controller and exit. |
| `--vmodule moduleSpec` | comma-separated list of pattern=N settings for file-filtered logging |
| `--watch-namespace string` | Namespace the controller watches for updates to Kubernetes objects. This includes Ingresses, Services and all configuration resources. All namespaces are watched if this parameter is left empty. |
| `--disable-catch-all` | Disable support for catch-all Ingresses. |
2 changes: 2 additions & 0 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ type Configuration struct {
SyncRateLimit float32

DynamicCertificatesEnabled bool

DisableCatchAll bool
}

// GetPublishService returns the Service used to set the load-balancer status of Ingresses.
Expand Down
3 changes: 2 additions & 1 deletion internal/ingress/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,8 @@ func newNGINXController(t *testing.T) *NGINXController {
fs,
channels.NewRingChannel(10),
false,
pod)
pod,
false)

config := &Configuration{
ListenPorts: &ngx_config.ListenPorts{
Expand Down
3 changes: 2 additions & 1 deletion internal/ingress/controller/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ func NewNGINXController(config *Configuration, mc metric.Collector, fs file.File
fs,
n.updateCh,
config.DynamicCertificatesEnabled,
pod)
pod,
config.DisableCatchAll)

n.syncQueue = task.NewTaskQueue(n.syncIngress)

Expand Down
27 changes: 25 additions & 2 deletions internal/ingress/controller/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ func New(checkOCSP bool,
fs file.Filesystem,
updateCh *channels.RingChannel,
isDynamicCertificatesEnabled bool,
pod *k8s.PodInfo) Storer {
pod *k8s.PodInfo,
disableCatchAll bool) Storer {

store := &k8sStore{
isOCSPCheckEnabled: checkOCSP,
Expand Down Expand Up @@ -310,6 +311,10 @@ func New(checkOCSP bool,
klog.Infof("ignoring add for ingress %v based on annotation %v with value %v", ing.Name, class.IngressKey, a)
return
}
if ing.Spec.Backend != nil && disableCatchAll {
klog.Infof("ignoring add for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
return
}
recorder.Eventf(ing, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name))

store.syncIngress(ing)
Expand Down Expand Up @@ -340,6 +345,10 @@ func New(checkOCSP bool,
klog.Infof("ignoring delete for ingress %v based on annotation %v", ing.Name, class.IngressKey)
return
}
if ing.Spec.Backend != nil && disableCatchAll {
klog.Infof("ignoring delete for catch-all ingress %v/%v because of --disable-catch-all", ing.Namespace, ing.Name)
return
}
recorder.Eventf(ing, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", ing.Namespace, ing.Name))

store.listers.IngressWithAnnotation.Delete(ing)
Expand All @@ -358,13 +367,27 @@ func New(checkOCSP bool,
validOld := class.IsValid(oldIng)
validCur := class.IsValid(curIng)
if !validOld && validCur {
if curIng.Spec.Backend != nil && disableCatchAll {
klog.Infof("ignoring update for catch-all ingress %v/%v because of --disable-catch-all", curIng.Namespace, curIng.Name)
return
}

klog.Infof("creating ingress %v based on annotation %v", curIng.Name, class.IngressKey)
recorder.Eventf(curIng, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
} else if validOld && !validCur {
klog.Infof("removing ingress %v based on annotation %v", curIng.Name, class.IngressKey)
// FIXME: this does not actually delete the Ingress resource.
// The existing one will be updated with latest changes and invalid ingress.class will be missed.
recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
Copy link
Member

Choose a reason for hiding this comment

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

@aledbf is this an intended behaviour of the controller to not ignore an ingress after it changes its class to something that's not handled by ingress-nginx?

Copy link
Member

Choose a reason for hiding this comment

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

No, this is a bug introduced when I added a copy of the Ingress with annotations. We should remove the ingress from the local store (store.listers.IngressWithAnnotation.Delete(oldIng))

Copy link
Member

Choose a reason for hiding this comment

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

Alright, we will fix it in a subsequent PR then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll have a look

} else if validCur && !reflect.DeepEqual(old, cur) {
recorder.Eventf(curIng, corev1.EventTypeNormal, "UPDATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
if curIng.Spec.Backend != nil && disableCatchAll {
klog.Infof("ignoring update for catch-all ingress %v/%v because of --disable-catch-all", curIng.Namespace, curIng.Name)
// FIXME: this does not actually delete the Ingress resource.
// The existing one will be updated with latest changes.
recorder.Eventf(curIng, corev1.EventTypeNormal, "DELETE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
Copy link
Member

Choose a reason for hiding this comment

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

Why are you not deleting ingress here? To keep it consistent with the valid class behaviour? If so then why are you recording DELETE event rather than UPDATE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has the same behavior has the valid class check. It records a DELETE event, but don't actually delete it.

I wanted more feedback on this issue before trying any fix here.
I've quickly tested to call the DeleteFunc handler when needed here and it works as expected. But I'm not 100% sure this is the solution.

Also, I think this should be fixed in a 2nd PR (that can be merge before this one).

Copy link
Member

Choose a reason for hiding this comment

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

👍

} else {
recorder.Eventf(curIng, corev1.EventTypeNormal, "UPDATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name))
}
} else {
klog.Infof("ignoring ingress %v based on annotation %v", curIng.Name, class.IngressKey)
return
Expand Down
18 changes: 12 additions & 6 deletions internal/ingress/controller/store/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down Expand Up @@ -163,7 +164,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down Expand Up @@ -316,7 +318,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down Expand Up @@ -424,7 +427,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down Expand Up @@ -514,7 +518,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down Expand Up @@ -627,7 +632,8 @@ func TestStore(t *testing.T) {
fs,
updateCh,
false,
pod)
pod,
false)

storer.Run(stopCh)

Expand Down
15 changes: 15 additions & 0 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,21 @@ func UpdateDeployment(kubeClientSet kubernetes.Interface, namespace string, name
return nil
}

// UpdateIngress runs the given updateFunc on the ingress
func UpdateIngress(kubeClientSet kubernetes.Interface, namespace string, name string, updateFunc func(d *extensions.Ingress) error) error {
ingress, err := kubeClientSet.ExtensionsV1beta1().Ingresses(namespace).Get(name, metav1.GetOptions{})
if err != nil {
return err
}

if err := updateFunc(ingress); err != nil {
return err
}

_, err = kubeClientSet.ExtensionsV1beta1().Ingresses(namespace).Update(ingress)
return err
}

// NewSingleIngressWithTLS creates a simple ingress rule with TLS spec included
func NewSingleIngressWithTLS(name, path, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, true)
Expand Down
109 changes: 109 additions & 0 deletions test/e2e/settings/disable_catch_all.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package settings

import (
"strings"

. "github.com/onsi/ginkgo"
appsv1beta1 "k8s.io/api/apps/v1beta1"

"k8s.io/ingress-nginx/test/e2e/framework"
)

var _ = framework.IngressNginxDescribe("Disabled catch-all", func() {
f := framework.NewDefaultFramework("disabled-catch-all")

BeforeEach(func() {
f.NewEchoDeploymentWithReplicas(1)

framework.UpdateDeployment(f.KubeClientSet, f.IngressController.Namespace, "nginx-ingress-controller", 1,
func(deployment *appsv1beta1.Deployment) error {
args := deployment.Spec.Template.Spec.Containers[0].Args
args = append(args, "--disable-catch-all=true")
deployment.Spec.Template.Spec.Containers[0].Args = args
_, err := f.KubeClientSet.AppsV1beta1().Deployments(f.IngressController.Namespace).Update(deployment)

return err
})
})

AfterEach(func() {
})

It("should ignore catch all Ingress", func() {
host := "foo"

ing := framework.NewSingleCatchAllIngress("catch-all", f.IngressController.Namespace, "http-svc", 80, nil)
f.EnsureIngress(ing)

ing = framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)
f.EnsureIngress(ing)

f.WaitForNginxServer(host, func(cfg string) bool {
return strings.Contains(cfg, "server_name foo")
})

f.WaitForNginxServer("_", func(cfg string) bool {
return strings.Contains(cfg, `set $ingress_name ""`) &&
strings.Contains(cfg, `set $proxy_upstream_name "upstream-default-backend"`)
})
Copy link
Member

@ElvinEfendi ElvinEfendi Jan 7, 2019

Choose a reason for hiding this comment

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

While this works, it leaks internals of Nginx configuration to the test. I know we do it in another e2e tests, but here we can avoid this and test the functionality by sending a request and asserting that the response is 404.

Similarly instead of https://github.com/kubernetes/ingress-nginx/pull/3586/files#diff-cb7b9b28ba2a89db95d178caee979d06R57 you can send a request and assert 200 response code.

})

// FIXME: This test doesn't work because of a bug in Ingress update handle in store package.
// It("should delete Ingress updated to catch-all", func() {
// host := "foo"

// ing := framework.NewSingleIngress(host, "/", host, f.IngressController.Namespace, "http-svc", 80, nil)
// f.EnsureIngress(ing)

// f.WaitForNginxServer(host,
// func(server string) bool {
// return strings.Contains(server, "server_name foo")
// })

// resp, _, errs := gorequest.New().
// Get(f.IngressController.HTTPURL).
// Set("Host", host).
// End()
// Expect(errs).To(BeNil())
// Expect(resp.StatusCode).Should(Equal(http.StatusOK))

// err := framework.UpdateIngress(f.KubeClientSet, f.IngressController.Namespace, host, func(ingress *extensions.Ingress) error {
// ingress.Spec.Rules = nil
// ingress.Spec.Backend = &extensions.IngressBackend{
// ServiceName: "http-svc",
// ServicePort: intstr.FromInt(80),
// }
// return nil
// })
// Expect(err).ToNot(HaveOccurred())

// f.WaitForNginxConfiguration(func(cfg string) bool {
// return !strings.Contains(cfg, "server_name foo") &&
// !strings.Contains(cfg, `set $ingress_name "foo"`) &&
// !strings.Contains(cfg, `set $service_name "http-svc"`)
// })

// resp, _, errs = gorequest.New().
// Get(f.IngressController.HTTPURL).
// Set("Host", host).
// End()
// Expect(errs).To(BeNil())
// Expect(resp.StatusCode).Should(Equal(http.StatusNotFound))
// })
})