Skip to content

Commit

Permalink
Add disable path overlap validation flag
Browse files Browse the repository at this point in the history
  ## What this PR does / why we need it:

In kubernetes#5651 there was a
request to throw an error when there are two ingresses defining the same
host and path, which was implemented as part of the validation webhook.

  Despite of this there are clear rules on the ingress controller that
describes what the controller would do in [such situation (the oldest
rule wins)](https://github.com/kubernetes/ingress-nginx/blob/main/docs/how-it-works.md?plain=1#L27)

  Some users are relying on this validation behaviour to prevent
misconfigurations, but there are use cases where allowing it, maybe
temporarily, is helpful.

Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9
  • Loading branch information
Fsero committed Feb 1, 2024
1 parent a6badc4 commit e8a10e1
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 39 deletions.
20 changes: 12 additions & 8 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,11 @@ type Configuration struct {

IngressClassConfiguration *ingressclass.IngressClassConfiguration

ValidationWebhook string
ValidationWebhookCertPath string
ValidationWebhookKeyPath string
DisableFullValidationTest bool
ValidationWebhook string
ValidationWebhookCertPath string
ValidationWebhookKeyPath string
DisableFullValidationTest bool
DisablePathOverlapValidation bool

GlobalExternalAuth *ngx_config.GlobalExternalAuth
MaxmindEditionFiles *[]string
Expand Down Expand Up @@ -319,11 +320,14 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
startTest := time.Now().UnixNano() / 1000000
_, servers, pcfg := n.getConfiguration(ings)

err := checkOverlap(ing, allIngresses, servers)
if err != nil {
n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
return err
if !n.cfg.DisablePathOverlapValidation {
err := checkOverlap(ing, allIngresses, servers)
if err != nil {
n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
return err
}
}

testedSize := len(ings)
if n.cfg.DisableFullValidationTest {
_, _, pcfg = n.getConfiguration(ings[len(ings)-1:])
Expand Down
77 changes: 77 additions & 0 deletions internal/ingress/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,83 @@ func TestCheckIngress(t *testing.T) {
t.Errorf("with a new ingress without error, no error should be returned")
}
})

t.Run("When there is a duplicated ingress with same host and path it should error", func(t *testing.T) {
ing := &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "test-namespace",
Annotations: map[string]string{
"kubernetes.io/ingress.class": "nginx",
},
},
Spec: networking.IngressSpec{
Rules: []networking.IngressRule{
{
Host: "example.com",
IngressRuleValue: networking.IngressRuleValue{
HTTP: &networking.HTTPIngressRuleValue{
Paths: []networking.HTTPIngressPath{
{
Path: "/",
PathType: &pathTypePrefix,
Backend: networking.IngressBackend{
Service: &networking.IngressServiceBackend{
Name: "http-svc",
Port: networking.ServiceBackendPort{
Number: 80,
Name: "http",
},
},
},
},
},
},
},
},
},
},
}

nginx.store = &fakeIngressStore{
ingresses: []*ingress.Ingress{
{
Ingress: *ing,
ParsedAnnotations: &annotations.Ingress{},
},
},
}
duplicatedIngress := ing.DeepCopy()
duplicatedIngress.ObjectMeta.Name = "duplicated-ingress"

nginx.cfg.DisablePathOverlapValidation = false
nginx.command = testNginxTestCommand{
t: t,
err: nil,
expected: "_,example.com",
}

err = nginx.CheckIngress(duplicatedIngress)
if err == nil {
t.Errorf("expected errors but none occurred")
}
t.Run("when disablePathOverlap is enabled should not throw any error", func(t *testing.T) {
duplicatedIngress := ing.DeepCopy()
duplicatedIngress.ObjectMeta.Name = "duplicated-ingress"

nginx.cfg.DisablePathOverlapValidation = true
nginx.command = testNginxTestCommand{
t: t,
err: nil,
expected: "_,example.com",
}

err = nginx.CheckIngress(duplicatedIngress)
if err != nil {
t.Errorf("expected no errors but one %+v occurred", err)
}
})
})
})

t.Run("When the ingress is marked as deleted", func(t *testing.T) {
Expand Down
68 changes: 37 additions & 31 deletions pkg/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ Takes the form "<host>:port". If not provided, no admission controller is starte
`The path of the validating webhook certificate PEM.`)
validationWebhookKey = flags.String("validating-webhook-key", "",
`The path of the validating webhook key PEM.`)
disablePathOverlapValidation = flags.Bool("disable-path-overlap-validation", false,
`Disable path overlap validation. Validation webhook blocks creating ingresses with the same hostname and path in the same ingressClass.
These flags turn off this validation as it helps split ingresses or move them between namespaces as it relies on the existing model rules:
'If the same path for the same host is defined in more than one Ingress, the oldest rule wins'`)

disableFullValidationTest = flags.Bool("disable-full-test", false,
`Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default).`)

Expand Down Expand Up @@ -315,37 +320,38 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
ngx_config.EnableSSLChainCompletion = *enableSSLChainCompletion

config := &controller.Configuration{
APIServerHost: *apiserverHost,
KubeConfigFile: *kubeConfigFile,
UpdateStatus: *updateStatus,
ElectionID: *electionID,
EnableProfiling: *profiling,
EnableMetrics: *enableMetrics,
MetricsPerHost: *metricsPerHost,
MetricsBuckets: histogramBuckets,
ReportStatusClasses: *reportStatusClasses,
MonitorMaxBatchSize: *monitorMaxBatchSize,
DisableServiceExternalName: *disableServiceExternalName,
EnableSSLPassthrough: *enableSSLPassthrough,
ResyncPeriod: *resyncPeriod,
DefaultService: *defaultSvc,
Namespace: *watchNamespace,
WatchNamespaceSelector: namespaceSelector,
ConfigMapName: *configMap,
TCPConfigMapName: *tcpConfigMapName,
UDPConfigMapName: *udpConfigMapName,
DisableFullValidationTest: *disableFullValidationTest,
DefaultSSLCertificate: *defSSLCertificate,
DeepInspector: *deepInspector,
PublishService: *publishSvc,
PublishStatusAddress: *publishStatusAddress,
UpdateStatusOnShutdown: *updateStatusOnShutdown,
ShutdownGracePeriod: *shutdownGracePeriod,
PostShutdownGracePeriod: *postShutdownGracePeriod,
UseNodeInternalIP: *useNodeInternalIP,
SyncRateLimit: *syncRateLimit,
HealthCheckHost: *healthzHost,
DynamicConfigurationRetries: *dynamicConfigurationRetries,
APIServerHost: *apiserverHost,
KubeConfigFile: *kubeConfigFile,
UpdateStatus: *updateStatus,
ElectionID: *electionID,
EnableProfiling: *profiling,
EnableMetrics: *enableMetrics,
MetricsPerHost: *metricsPerHost,
MetricsBuckets: histogramBuckets,
ReportStatusClasses: *reportStatusClasses,
MonitorMaxBatchSize: *monitorMaxBatchSize,
DisableServiceExternalName: *disableServiceExternalName,
EnableSSLPassthrough: *enableSSLPassthrough,
ResyncPeriod: *resyncPeriod,
DefaultService: *defaultSvc,
Namespace: *watchNamespace,
WatchNamespaceSelector: namespaceSelector,
ConfigMapName: *configMap,
TCPConfigMapName: *tcpConfigMapName,
UDPConfigMapName: *udpConfigMapName,
DisableFullValidationTest: *disableFullValidationTest,
DisablePathOverlapValidation: *disablePathOverlapValidation,
DefaultSSLCertificate: *defSSLCertificate,
DeepInspector: *deepInspector,
PublishService: *publishSvc,
PublishStatusAddress: *publishStatusAddress,
UpdateStatusOnShutdown: *updateStatusOnShutdown,
ShutdownGracePeriod: *shutdownGracePeriod,
PostShutdownGracePeriod: *postShutdownGracePeriod,
UseNodeInternalIP: *useNodeInternalIP,
SyncRateLimit: *syncRateLimit,
HealthCheckHost: *healthzHost,
DynamicConfigurationRetries: *dynamicConfigurationRetries,
ListenPorts: &ngx_config.ListenPorts{
Default: *defServerPort,
Health: *healthzPort,
Expand Down

0 comments on commit e8a10e1

Please sign in to comment.