DevFW-CICD/ingress-nginx-helm
16
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix handling of overlapping canary ingresses

Browse source 
The existingIngresses slice never contains any canary ingress, indeed
this kind of ingresses are always excluded from the server slice:
  func (n *NGINXController) createServers(data []*ingress.Ingress,
	upstreams map[string]*ingress.Backend,
	du *ingress.Backend) map[string]*ingress.Server {
    [...]
    if anns.Canary.Enabled {
      klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
      continue
    }
which means that the test aiming to compare a canary ingress with all
the other existing canary ingresses never succeeds.

This commit fixes the handling of conflicting canary ingresses.

Signed-off-by: Hervé Werner <dud225@hotmail.com>
This commit is contained in:
Hervé Werner 2023-01-27 12:09:47 +01:00
parent ea3fb5a156
commit fd5e14b096
2 changed files with 58 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

51
internal/ingress/controller/controller.go
View file

@ -339,7 +339,7 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
startTest := time.Now().UnixNano() / 1000000
_, servers, pcfg := n.getConfiguration(ings)
err := checkOverlap(ing, allIngresses, servers)
err := checkOverlap(ing, pcfg.Backends, servers)
if err != nil {
n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
return err
@ -550,7 +550,7 @@ func (n *NGINXController) getConfiguration(ingresses []*ingress.Ingress) (sets.S
for _, server := range servers {
// If a location is defined by a prefix string that ends with the slash character, and requests are processed by one of
// proxy_pass, fastcgi_pass, uwsgi_pass, scgi_pass, memcached_pass, or grpc_pass, then the special processing is performed.
// In response to a request with URI equal to // this string, but without the trailing slash, a permanent redirect with the
// In response to a request with URI equal to this string, but without the trailing slash, a permanent redirect with the
// code 301 will be returned to the requested URI with the slash appended. If this is not desired, an exact match of the
// URIand location could be defined like this:
//
@ -1482,7 +1482,7 @@ func locationApplyAnnotations(loc *ingress.Location, anns *annotations.Ingress)
loc.DefaultBackendUpstreamName = defUpstreamName
}
// OK to merge canary ingresses iff there exists one or more ingresses to potentially merge into
// OK to merge canary ingresses if there exists one or more ingresses to potentially merge into
func nonCanaryIngressExists(ingresses []*ingress.Ingress, canaryIngresses []*ingress.Ingress) bool {
return len(ingresses)-len(canaryIngresses) > 0
}
@ -1728,7 +1728,7 @@ func externalNamePorts(name string, svc *apiv1.Service) *apiv1.ServicePort {
}
}
func checkOverlap(ing *networking.Ingress, ingresses []*ingress.Ingress, servers []*ingress.Server) error {
func checkOverlap(ing *networking.Ingress, upstreams []*ingress.Backend, servers []*ingress.Server) error {
for _, rule := range ing.Spec.Rules {
if rule.HTTP == nil {
continue
@ -1738,7 +1738,6 @@ func checkOverlap(ing *networking.Ingress, ingresses []*ingress.Ingress, servers
rule.Host = defServerName
}
OUTER:
for _, path := range rule.HTTP.Paths {
if path.Backend.Service == nil {
// skip non-service backends
@ -1750,41 +1749,30 @@ func checkOverlap(ing *networking.Ingress, ingresses []*ingress.Ingress, servers
path.Path = rootLocation
}
existingIngresses := ingressForHostPath(rule.Host, path.Path, servers)
existingIngresses, numCanaries := ingressForHostPath(rule.Host, path.Path, upstreams, servers)
// no previous ingress
if len(existingIngresses) == 0 {
continue
}
// same ingress
// check for path overlap
for _, existing := range existingIngresses {
if existing.ObjectMeta.Namespace == ing.ObjectMeta.Namespace && existing.ObjectMeta.Name == ing.ObjectMeta.Name {
continue OUTER
if ing.ObjectMeta.UID != existing.ObjectMeta.UID {
if _, err := parser.GetBoolAnnotation("canary", ing); err == nil && numCanaries > 1 {
return fmt.Errorf(`host "%s" and path "%s" is already defined in a canary ingress`, rule.Host, path.Path)
} else if err == errors.ErrMissingAnnotations {
return fmt.Errorf(`host "%s" and path "%s" is already defined in ingress %s/%s`, rule.Host, path.Path, existing.Namespace, existing.Name)
} else if err != nil {
return fmt.Errorf(`Uanble to read annotation: %s`, err)
}
}
}
// path overlap. Check if one of the ingresses has a canary annotation
isCanaryEnabled, annotationErr := parser.GetBoolAnnotation("canary", ing)
for _, existing := range existingIngresses {
isExistingCanaryEnabled, existingAnnotationErr := parser.GetBoolAnnotation("canary", existing)
if isCanaryEnabled && isExistingCanaryEnabled {
return fmt.Errorf(`host "%s" and path "%s" is already defined in ingress %s/%s`, rule.Host, path.Path, existing.Namespace, existing.Name)
}
if annotationErr == errors.ErrMissingAnnotations && existingAnnotationErr == errors.ErrMissingAnnotations {
return fmt.Errorf(`host "%s" and path "%s" is already defined in ingress %s/%s`, rule.Host, path.Path, existing.Namespace, existing.Name)
}
}
}
}
return nil
}
func ingressForHostPath(hostname, path string, servers []*ingress.Server) []*networking.Ingress {
func ingressForHostPath(hostname, path string, upstreams []*ingress.Backend, servers []*ingress.Server) ([]*networking.Ingress, int) {
ingresses := make([]*networking.Ingress, 0)
var numCanaries int
for _, server := range servers {
if hostname != server.Hostname {
@ -1801,10 +1789,15 @@ func ingressForHostPath(hostname, path string, servers []*ingress.Server) []*net
}
ingresses = append(ingresses, &location.Ingress.Ingress)
for _, upstream := range upstreams {
if location.Backend == upstream.Name {
numCanaries += len(upstream.AlternativeBackends)
}
}
}
}
return ingresses
return ingresses, numCanaries
}
func (n *NGINXController) getStreamSnippets(ingresses []*ingress.Ingress) []string {

36
test/e2e/admission/admission.go
View file

@ -27,10 +27,12 @@ import (
"github.com/onsi/ginkgo/v2"
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
networking "k8s.io/api/networking/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/ingress-nginx/test/e2e/framework"
)
@ -99,6 +101,40 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller",
assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with the same host and path should not return an error using a canary annotation")
})
ginkgo.It("should not allow overlaps of host and paths from multiple ingresses with canary annotation", func() {
canaryAnnotations := map[string]string{
"nginx.ingress.kubernetes.io/canary": "true",
"nginx.ingress.kubernetes.io/canary-by-header": "CanaryByHeader",
}
secondIngress := framework.NewSingleIngress("second-ingress", "/", host, f.Namespace, framework.SlowEchoService, 80, canaryAnnotations)
_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), secondIngress, metav1.CreateOptions{})
assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with the same host and path should not return an error using a canary annotation")
svc := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "third-ingress-svc",
Namespace: f.Namespace,
},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
{
Name: framework.SlowEchoService,
Port: 80,
TargetPort: intstr.FromInt(80),
},
},
Selector: map[string]string{
"app": framework.SlowEchoService,
},
},
}
f.EnsureService(svc)
thirdIngress := framework.NewSingleIngress("third-ingress", "/", host, f.Namespace, "third-ingress-svc", 80, canaryAnnotations)
_, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), thirdIngress, metav1.CreateOptions{})
assert.True(ginkgo.GinkgoT(), k8sErrors.IsBadRequest(err), "creating multiple canary ingress with the same host and path should return an error")
})
ginkgo.It("should not allow overlaps of host and path in ingress comprised of multiple rules", func() {
secondIngress := framework.NewSingleIngress("second-ingress", "/", fmt.Sprintf("%s-non-overlapping", host), f.Namespace, framework.EchoService, 80, nil)
pathprefix := networking.PathTypePrefix