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

Ignore ingress definitions for other ingressclasses #7277

Closed
liggitt opened this issue Jun 24, 2021 · 5 comments · Fixed by #7341
Closed

Ignore ingress definitions for other ingressclasses #7277

liggitt opened this issue Jun 24, 2021 · 5 comments · Fixed by #7341
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@liggitt
Copy link
Member

liggitt commented Jun 24, 2021

NGINX Ingress controller version: v1.0.0-alpha.1

Kubernetes version (use kubectl version): v1.19+

Environment: all

  • Cloud provider or hardware configuration: all
  • OS (e.g. from /etc/os-release): all
  • Kernel (e.g. uname -a): all
  • Install tools: all
  • Others: all

What happened: ingress definitions created intended to be serviced by other ingress controllers are handled by ingress-nginx

Reviewing the code path ingress-nginx uses to fetch ingress objects, I don't see filtering of ingress objects targeting other ingress classes:

  • pods, err := api.Ingresses(namespace).List(context.TODO(), metav1.ListOptions{})
    if err != nil {
    return make([]networking.Ingress, 0), err
    }
    return pods.Items, nil
  • func ingresses(flags *genericclioptions.ConfigFlags, host string, allNamespaces bool) error {
    var namespace string
    if allNamespaces {
    namespace = ""
    } else {
    namespace = util.GetNamespace(flags)
    }
    ingresses, err := request.GetIngressDefinitions(flags, namespace)
    if err != nil {
    return err
    }
    rows := getIngressRows(&ingresses)
  • func getIngressRows(ingresses *[]networking.Ingress) []ingressRow {
    rows := make([]ingressRow, 0)
    for _, ing := range *ingresses {
    address := ""
    for _, lbIng := range ing.Status.LoadBalancer.Ingress {
    if len(lbIng.IP) > 0 {
    address = address + lbIng.IP + ","
    }
    if len(lbIng.Hostname) > 0 {
    address = address + lbIng.Hostname + ","
    }
    }
    if len(address) > 0 {
    address = address[:len(address)-1]
    }
    tlsHosts := make(map[string]struct{})
    for _, tls := range ing.Spec.TLS {
    for _, host := range tls.Hosts {
    tlsHosts[host] = struct{}{}
    }
    }
    defaultBackendService := ""
    defaultBackendPort := ""
    if ing.Spec.DefaultBackend != nil {
    name, port := serviceToNameAndPort(ing.Spec.DefaultBackend.Service)
    defaultBackendService = name
    defaultBackendPort = port.String()
    }
    // Handle catch-all ingress
    if len(ing.Spec.Rules) == 0 && len(defaultBackendService) > 0 {
    row := ingressRow{
    Namespace: ing.Namespace,
    IngressName: ing.Name,
    Host: "*",
    ServiceName: defaultBackendService,
    ServicePort: defaultBackendPort,
    Address: address,
    }
    rows = append(rows, row)
    continue
    }

That means that ingress-nginx can't coexist with another ingress controller partitioned by ingress class, which is part of Ingress v1.

/kind bug

@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Jun 24, 2021
@liggitt liggitt changed the title Ignore ingress definitions for other ingressclasses cmd/ingresses: Ignore ingress definitions for other ingressclasses Jun 24, 2021
@liggitt liggitt changed the title cmd/ingresses: Ignore ingress definitions for other ingressclasses Ignore ingress definitions for other ingressclasses Jun 24, 2021
@rikatz
Copy link
Contributor

rikatz commented Jun 24, 2021

/triage accepted
/priority critical-urgent
/assgn @rikatz

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jun 24, 2021
@rikatz rikatz self-assigned this Jun 24, 2021
@rikatz
Copy link
Contributor

rikatz commented Jun 27, 2021

Thanks @liggitt

I'll take a look into this.

For my future me (or people helping) we need to:

  • Implement the filtering
  • Implement e2e tests and check if this filtering is working
    • Objects with IngressClass = nginx class should be parsed and served
    • Objects with IngressClass != controller ingress class should be filtered and ignored
    • Check this case with a bunch of ingress objects and the same ingress controller

@rikatz
Copy link
Contributor

rikatz commented Jul 29, 2021

/close

Closed in #7341

@k8s-ci-robot
Copy link
Contributor

@rikatz: Closing this issue.

In response to this:

/close

Closed in #7341

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@clarax
Copy link

clarax commented Jul 20, 2022

@rikatz Hello, I have a question on a potential bug of Ingress-Nginx. We have two ingress controllers, controller A (v1.2.0) and controller B (v 1.2.1), installed on a Kubernetes cluster(version: v1.22.9) Each controller has a different IngressClass: A and B. When I try to create an ingress of ingress class A, it failed with error "Error from server (InternalError): error when creating "ingress-test.yaml": Internal error occurred: failed calling webhook "validate.nginx.ingress.kubernetes.io": Post https://ingress-nginx-controller-admission.ingress-nginx.svc:443/extensions/v1/ingresses?timeout=30s: context deadline exceeded. The controller B complains "controller I0720 18:12:05.800513 7 store.go:425] "Ignoring ingress because of error while validating ingress class" ingress="ingress-test" error="no object matching key "A" in local store"". Ingress of IngressClass A is ignored because of validating error. If we have the fix, we shouldn't need to validate at all because of the IngressClass. Is this caused by an incomplete fix? Is this expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants