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

Support configuring upstream zone sizes in VS/VSR #659

Merged
merged 10 commits into from
Aug 22, 2019

Conversation

LorcanMcVeigh
Copy link
Contributor

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

Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

  1. 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.

  2. An example and docs should be included in: https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/virtualserver-and-virtualserverroute.md#upstream

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@Rulox Rulox left a comment

Choose a reason for hiding this comment

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

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

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

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Please see additional comments

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

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

👍

@LorcanMcVeigh LorcanMcVeigh merged commit f3e11f3 into master Aug 22, 2019
@LorcanMcVeigh LorcanMcVeigh deleted the ext-upstreamzone-support branch August 22, 2019 08:57
@pleshakov pleshakov changed the title Upstreamzone support for plus and vs Support configuring upstream zone sizes in VS/VSR Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants