Skip to content

Commit

Permalink
Merge pull request #3586 from Shopify/disable-catch-all
Browse files Browse the repository at this point in the history
Add --disable-catch-all option to disable catch-all server
  • Loading branch information
k8s-ci-robot authored Jan 7, 2019
2 parents c326c55 + 1678d99 commit 8f57f95
Show file tree
Hide file tree
Showing 9 changed files with 172 additions and 10 deletions.
4 changes: 4 additions & 0 deletions cmd/nginx/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,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 @@ -257,6 +260,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 @@ -98,6 +98,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 @@ -128,7 +128,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 @@ -233,7 +233,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 @@ -311,6 +312,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 @@ -341,6 +346,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 @@ -359,13 +368,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))
} 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))
} 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 @@ -372,6 +372,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"`)
})
})

// 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))
// })
})

0 comments on commit 8f57f95

Please sign in to comment.