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.

Sign up for GitHub

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

Jump to bottom

Support configuring upstream zone sizes in VS/VSR #659

Merged

LorcanMcVeigh merged 10 commits into master from ext-upstreamzone-support

Aug 22, 2019

Contributor

LorcanMcVeigh commented Aug 14, 2019

Proposed changes

Extended upstream zone size support to nginx plus and vs/vsr

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto master
I will ensure my PR is targeting the master branch and pulling from my branch from my own fork


          Upstreamzone support for plus and vs

6fbcf1f

LorcanMcVeigh requested review from pleshakov, Rulox, Dean-Coakley, bvighnesha and ampant

August 14, 2019 13:38

LorcanMcVeigh self-assigned this

Dean-Coakley suggested changes

View reviewed changes

Contributor

Dean-Coakley left a comment

I think we need to do some validation in https://github.com/nginxinc/kubernetes-ingress/blob/master/pkg/apis/configuration/validation/validation.go#L282 ? I suggest writing a reuseable func validateSize to ensure a valid [size] was specified.
An example and docs should be included in: https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/virtualserver-and-virtualserverroute.md#upstream

pleshakov suggested changes

View reviewed changes

Contributor

pleshakov left a comment •

edited

Loading

@LorcanMcVeigh thanks for the PR. Please see my comments.

Thanks for the comments @Dean-Coakley However, they are not applicable because extending the VS/VSR resources upstream field is not part of this task. However, the configmap key upstream-zone-size must affect the generated config for VS/VSR. At the same time, please update the description of the upstream-zone-size entry in the configmap table for the case of NGINX Plus.

internal/configs/version1/nginx-plus.ingress.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx.virtualserver.tmpl Outdated Show resolved Hide resolved

pkg/apis/configuration/v1alpha1/types.go Outdated Show resolved Hide resolved

LorcanMcVeigh added 3 commits

August 15, 2019 10:25


          Address feedback

570feee


          Addressd feedback from pr

e810a36


          Address further feedback

ddcc41e

LorcanMcVeigh requested review from pleshakov and Dean-Coakley

August 15, 2019 13:34

pleshakov reviewed

View reviewed changes

Contributor

pleshakov left a comment

@LorcanMcVeigh thanks for the update. Please see an additional suggestion.
Also, the comment about the documentation wasn't addressed. We need to update the documentation to cover NGINX Plus.

internal/configs/version1/nginx-plus.ingress.tmpl Outdated Show resolved Hide resolved

LorcanMcVeigh added 3 commits

August 16, 2019 08:33


          Update docs for upstream-zone-size

11334f4

WIP

6b957af


          Address feedback from PR

8c75e16

LorcanMcVeigh requested a review from pleshakov

August 16, 2019 13:50

pleshakov suggested changes

View reviewed changes

Contributor

pleshakov left a comment

looks better! Please see the suggestions for the added parts.

internal/configs/version1/nginx-plus.ingress.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved


          Addressing feedback

80dc5d9

LorcanMcVeigh force-pushed the ext-upstreamzone-support branch from c3c9615 to 80dc5d9 Compare

August 19, 2019 07:58

Dean-Coakley approved these changes

View reviewed changes

Rulox suggested changes

View reviewed changes

Contributor

Rulox left a comment

lgtm, just a small comment, let me know if it makes sense.

internal/configs/version1/nginx-plus.ingress.tmpl Outdated Show resolved Hide resolved

pleshakov suggested changes

View reviewed changes

Contributor

pleshakov left a comment

Please see additional comments

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved


          Address feedback from reviews

5a1f177

LorcanMcVeigh requested review from Rulox and pleshakov

August 20, 2019 10:55

pleshakov suggested changes

View reviewed changes

Contributor

pleshakov left a comment

thanks @LorcanMcVeigh looks like we're almost there. Please see a few suggestions

internal/configs/version1/nginx-plus.ingress.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx.virtualserver.tmpl Outdated Show resolved Hide resolved


          Addressed feedback from pr

8ea2fea

LorcanMcVeigh requested a review from pleshakov

August 21, 2019 11:16

pleshakov approved these changes

View reviewed changes

Contributor

pleshakov left a comment

👍

LorcanMcVeigh merged commit f3e11f3 into master

LorcanMcVeigh deleted the ext-upstreamzone-support branch

August 22, 2019 08:57

pleshakov changed the title ~~Upstreamzone support for plus and vs~~ Support configuring upstream zone sizes in VS/VSR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pleshakov pleshakov approved these changes

Dean-Coakley Dean-Coakley approved these changes

bvighnesha Awaiting requested review from bvighnesha

ampant Awaiting requested review from ampant

Rulox Awaiting requested review from Rulox

Labels

None yet