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 annotation for client-body-buffer-size per location #1186

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

diazjf
Copy link

@diazjf diazjf commented Aug 20, 2017

Adds an annotation which allows for client-body-buffer-size to
be configured per specific locations specified in the ingress
resource yaml. Fixes #826

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 20, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 44.509% when pulling 8caeb5b on diazjf:client-body-buffer-size into 7010627 on kubernetes:master.

@diazjf
Copy link
Author

diazjf commented Aug 21, 2017

Currently only working on creation, but update is not configuring the client_body_buffer_size.

Annotation name was changed to ingress.kubernetes.io/client-body-buffer-size to avoid conflicts with the already existing ingress.kubernetes.io/proxy-body-size which configures client_max_body_size.

@diazjf
Copy link
Author

diazjf commented Aug 21, 2017

If invalid value is provided ingress-controller will fail as follows:

Error: exit status 1
2017/08/21 01:52:57 [emerg] 596#596: "client_body_buffer_size" directive invalid value in /tmp/nginx-cfg431539895:277
nginx: [emerg] "client_body_buffer_size" directive invalid value in /tmp/nginx-cfg431539895:277
nginx: configuration file /tmp/nginx-cfg431539895 test failed

Would this be expected behavior?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 44.398% when pulling ff83a35 on diazjf:client-body-buffer-size into a0e4661 on kubernetes:master.

@diazjf
Copy link
Author

diazjf commented Aug 21, 2017

@aledbf two questions:

1.) Do annotation values regularly need validation, for example in client_body_buffer_size it must be in the format of an int which can be followed by a k or m. Should we check this before generating a template and if it is incorrect set a default of "" so that nothing is generated in the config or let it of to the error:

Error: exit status 1
2017/08/21 01:52:57 [emerg] 596#596: "client_body_buffer_size" directive invalid value in /tmp/nginx-cfg431539895:277
nginx: [emerg] "client_body_buffer_size" directive invalid value in /tmp/nginx-cfg431539895:277
nginx: configuration file /tmp/nginx-cfg431539895 test failed

2.) Should the location parameter for client_body_buffer_size be updated each time the annotation is updated? I'm thinking this would require us to reload the ingress backend anytime there is a change detected from the current value in the location parameter and whats in the annotation for a particular location.

Thanks again and have a good night!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 44.363% when pulling 30d4cca on diazjf:client-body-buffer-size into 6ef6343 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Aug 21, 2017

1.) yes, we need to add that feature but not in this PR :)

2.) this is done each time the sync loop is executed
https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L373

Adds an annotation which allows for client-body-buffer-size to
be configured per specific locations specified in the ingress
resource yaml.
@diazjf
Copy link
Author

diazjf commented Aug 22, 2017

Ingress Resource YAML:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: ok-ingress
  annotations:
    ingress.kubernetes.io/client-body-buffer-size: 4k
spec:
  tls:
  - hosts:
    - ok.com
  rules:
  - host: ok.com
    http:
      paths:
      - path: /tea
        backend:
          serviceName: tea-svc
          servicePort: 80

produces https://pastebin.com/0eSbjHfx in which you can see that client_body_buffer_size is 4k for location/ and location/tea.

It also updates each time a change is made.

@diazjf
Copy link
Author

diazjf commented Aug 22, 2017

@aledbf ready for review. My only question is if client_body_buffer_size shouldn't be applied to location / as well.

@diazjf diazjf changed the title WIP: Add annotation for client-body-buffer-size per location Add annotation for client-body-buffer-size per location Aug 22, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 44.463% when pulling e9ffbf0 on diazjf:client-body-buffer-size into 1da974f on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Aug 22, 2017

My only question is if client_body_buffer_size shouldn't be applied to location / as well.

No, only the the paths in the ingress.

@aledbf
Copy link
Member

aledbf commented Aug 22, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2017
@aledbf
Copy link
Member

aledbf commented Aug 22, 2017

@diazjf thanks!

@aledbf aledbf merged commit 9863140 into kubernetes:master Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants