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 support for IngressClass and ingress.class annotation #5410

Merged
merged 1 commit into from Apr 22, 2020

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Apr 20, 2020

What this PR does / why we need it:

Add support for new IngressClass resource, keeping support for ingress.class annotation.

Ingress controller deployment flag: --ingress-class=internal
Existing ingresses with annotation: kubernetes.io/ingress.class: "internal"
New IngressClass:

apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: "internal"
spec:
  controller: "k8s.io/ingress-nginx"

The flag will be reused for the annotation and the new IngressClass resource.
The ingress controller will check the IngressClass on startup in the same namespace where is running

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

replaces #5354

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2020
@aledbf aledbf added this to In Progress in 0.31.0 Apr 20, 2020
@codecov-io
Copy link

codecov-io commented Apr 20, 2020

Codecov Report

Merging #5410 into master will decrease coverage by 0.15%.
The diff coverage is 23.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5410      +/-   ##
==========================================
- Coverage   58.64%   58.48%   -0.16%     
==========================================
  Files          88       88              
  Lines        6913     6916       +3     
==========================================
- Hits         4054     4045       -9     
- Misses       2414     2426      +12     
  Partials      445      445              
Impacted Files Coverage Δ
cmd/nginx/main.go 6.06% <0.00%> (-0.35%) ⬇️
internal/ingress/controller/store/store.go 59.78% <ø> (-0.30%) ⬇️
internal/ingress/controller/template/template.go 80.10% <ø> (ø)
internal/k8s/main.go 69.23% <ø> (ø)
internal/ingress/annotations/class/main.go 54.54% <50.00%> (-34.35%) ⬇️
internal/ingress/controller/controller.go 49.28% <0.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 837d370...cfc90c9. Read the comment docs.

@aledbf aledbf force-pushed the ingressclass branch 3 times, most recently from f938fdc to 28ed7cf Compare April 21, 2020 14:13
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2020
@aledbf
Copy link
Member Author

aledbf commented Apr 21, 2020

/retest

@aledbf aledbf force-pushed the ingressclass branch 2 times, most recently from d714e83 to 84d5df4 Compare April 21, 2020 23:22
@aledbf aledbf changed the title WIP: Add support for IngressClass and ingress.class annotation Add support for IngressClass and ingress.class annotation Apr 21, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2020
@aledbf
Copy link
Member Author

aledbf commented Apr 22, 2020

/retest

@aledbf
Copy link
Member Author

aledbf commented Apr 22, 2020

/assign @ElvinEfendi

@aledbf
Copy link
Member Author

aledbf commented Apr 22, 2020

/retest


klog.Warningf("No IngressClass resource with name %v found. Only annotation will be used.", class.IngressClass)

// TODO: remove once this is fixed in client-go
Copy link
Member

Choose a reason for hiding this comment

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

What is "this" here? The value client-go returns is non nil in the presence of error? It'd be great if you can link a bug report in the TODO.

ingress, ok := ing.GetAnnotations()[IngressKey]
if ok {
// empty annotation and same annotation on ingress
if ingress == "" && IngressClass == DefaultClass {
Copy link
Member

Choose a reason for hiding this comment

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

My interpretation of https://github.com/kubernetes/kubernetes/blob/d911254debefb5b630498aa3552899ee2f99f22e/pkg/apis/networking/types.go#L276 is that the controller should accept ingresses without set annotation only if configured IngressClass for the controller has ingressclass.kubernetes.io/is-default-class set to true.

I also assume the API won't allow more than one IngressClass resource to have that annotation set to true in a given cluster, which together with validation of unique IngressClass name will ensure there's no two controller in the cluster fighting over the same Ingress resource.

Given above, are you deliberately ignoring ingressclass.kubernetes.io/is-default-class for now or I'm missing something?

Copy link
Member Author

@aledbf aledbf Apr 22, 2020

Choose a reason for hiding this comment

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

We don't need to test that. The annotation is handled by an admission controller in k8s itself.

echo "
apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: nginx
  annotations:
    ingressclass.kubernetes.io/is-default-class: 'true'
spec:
  controller: k8s.io/ingress-nginx
" | kubectl apply -f

echo "
apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  name: ingress
spec:
  rules:
  - host: foo.bar.com
    http:
      paths:
      - path: /
        backend:
          serviceName: http-svc
          servicePort: 80
" | kubectl apply -f -

IngressClass with the annotation adds the ingressClassName in the ingress with value nginx.

k get ing -o yaml ingress
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"networking.k8s.io/v1beta1","kind":"Ingress","metadata":{"annotations":{},"name":"ingress","namespace":"default"},"spec":{"rules":[{"host":"foo.bar.com","http":{"paths":[{"backend":{"serviceName":"http-svc","servicePort":80},"path":"/"}]}}]}}
  creationTimestamp: "2020-04-22T13:29:35Z"
  generation: 1
  managedFields:
  - apiVersion: networking.k8s.io/v1beta1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .: {}
          f:kubectl.kubernetes.io/last-applied-configuration: {}
      f:spec:
        f:rules: {}
    manager: kubectl
    operation: Update
    time: "2020-04-22T13:29:35Z"
  - apiVersion: networking.k8s.io/v1beta1
    fieldsType: FieldsV1
    fieldsV1:
      f:status:
        f:loadBalancer:
          f:ingress: {}
    manager: nginx-ingress-controller
    operation: Update
    time: "2020-04-22T13:30:01Z"
  name: ingress
  namespace: default
  resourceVersion: "2826"
  selfLink: /apis/extensions/v1beta1/namespaces/default/ingresses/ingress
  uid: 790496fb-7e11-4b81-a964-b70d45d30a57
spec:
  ingressClassName: nginx
  rules:
  - host: foo.bar.com
    http:
      paths:
      - backend:
          serviceName: http-svc
          servicePort: 80
        path: /
        pathType: ImplementationSpecific
status:
  loadBalancer:
    ingress:
    - ip: 10.110.197.76

Copy link
Member Author

Choose a reason for hiding this comment

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

When a
// single IngressClass resource has this annotation set to true, new Ingress
// resources without a class specified will be assigned this default class.

The behavior is to assign the IngressClass to new ingress resources if the class contains the annotation.

@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aledbf
Copy link
Member Author

aledbf commented Apr 22, 2020

/retest

1 similar comment
@aledbf
Copy link
Member Author

aledbf commented Apr 22, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit afa91cc into kubernetes:master Apr 22, 2020
@aledbf aledbf moved this from In Progress to done in 0.31.0 Apr 22, 2020
@aledbf aledbf deleted the ingressclass branch April 22, 2020 16:14
@khs1994
Copy link

khs1994 commented Apr 27, 2020

@aledbf

apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: "internal"
- namespace: "ingress-nginx"
spec:
  controller: "k8s.io/ingress-nginx"

IngressClass is NOT NAMESPACED

@khs1994
Copy link

khs1994 commented Apr 27, 2020

how to create IngressClass?

apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: "nginx"
spec:
  controller: "k8s.io/ingress-nginx"

is right?

@alexellis
Copy link

+1 to @khs1994 - I'd also like to see a worked example if possible - how to install the chart with this enabled (if any changes are needed)

Do I assume correctly that you must first create the IngressClass before installing the ingress-controller?

@matoszz
Copy link

matoszz commented Jun 15, 2020

+1 on functional examples of using IngressClass - the resource is not included in any installation setups / docs

@realdream
Copy link

sorrt to disturb, very confuse how to setup the new api in self-host-clusters
k8s: v1.18.4
ingress-nginx: v0.34.1

apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: external-lb
spec:
  controller: example.com/ingress-controller #what's controller mean? point to the ingress-nginx's service? deployment?
  parameters:
    apiGroup: k8s.example.com/v1alpha
    kind: IngressParameters
    name: external-lb

@aledbf
Copy link
Member Author

aledbf commented Jul 29, 2020

@realdream please check https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#ingressclassspec-v1beta1-networking-k8s-io

Basically, this field defines which ingress controller implementation is going to be used controller: "k8s.io/ingress-nginx"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
0.31.0
  
done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants