Skip to content

Only support dynamic configuration #3198

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
Merged
merged 1 commit into from
Oct 10, 2018
Merged

Only support dynamic configuration #3198

merged 1 commit into from
Oct 10, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 7, 2018

No description provided.

@aledbf aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2018
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 7, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2018
@aledbf aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2018
diazjf
diazjf reviewed Oct 8, 2018
View reviewed changes
rootfs/etc/nginx/template/nginx.tmpl
@@ -407,35 +401,6 @@ http {
{{ $cfg.HTTPSnippet }}
{{ end }}

{{ if not $all.DynamicConfigurationEnabled }}
Copy link

Choose a reason for hiding this comment

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

Since DynamicConfiguration will be the only available option, does that mean that load-balancing algorithms other than round-robin and annotations such as:

  • nginx.ingress.kubernetes.io/upstream-max-fails
  • nginx.ingress.kubernetes.io/upstream-fail-timeout

will be deprecated as well. If so can they be removed as well in another commit?

Copy link

Choose a reason for hiding this comment

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

If so I can close #3193

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we can pass that info to the lua LB

Copy link
Member

Choose a reason for hiding this comment

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

I wonder how much value they have given we "outsource" healthchecking to k8s instead of doing it in ingress-nginx controller.

I'd rather not complicate Lua balancer logic further unless it's really necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not complicate Lua balancer logic further unless it's really necessary.

Makes sense. That said we already have https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/lua/balancer.lua#L158. Maybe upstream-max-fails makes sense in this case?

Edit: the docs https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.md#set_more_tries are not clear about the configuration

Copy link
Member

Choose a reason for hiding this comment

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

currently doing

ngx_balancer.set_more_tries(1)

retries the request (accring proxy_next_upstream) proxy_next_upstream_tries times. If proxy_next_upstream_tries is set to zero then it will try unlimited times until client timeouts or finally request succeeds.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I will remove this annotations in this PR to keep this simple

Copy link
Member

Choose a reason for hiding this comment

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

@aledbf I created #3207 to do so

ElvinEfendi
ElvinEfendi reviewed Oct 9, 2018
View reviewed changes
cmd/nginx/flags.go
return false, nil, fmt.Errorf(`SSL certificate chain completion cannot be enabled and dynamic configuration cannot be disabled when
dynamic certificates functionality is enabled. Please check the flags --enable-ssl-chain-completion and --enable-dynamic-configuration`)
}

// LuaJIT is not available on arch s390x and ppc64le
disableLua := false
if runtime.GOARCH == "s390x" || runtime.GOARCH == "ppc64le" {
Copy link
Member

@ElvinEfendi ElvinEfendi Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So this means we are also officially dropping support for s390x and ppc64le as well right? If so can you also cleanup make and build files?

Copy link
Member Author

@aledbf aledbf Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No, there is luajit support for ppc64le. I think I will disable s390x and ask for users with access to that hardware if they can provide some kind of access.

Edit: I will update this section after the PR is merged, generating a new nginx image (also removing the sticky module)

@aledbf aledbf force-pushed the only-dynamic branch 3 times, most recently from 6812fc8 to fe56e2d Compare October 10, 2018 00:22
ElvinEfendi
ElvinEfendi reviewed Oct 10, 2018
View reviewed changes
internal/ingress/controller/template/template.go Outdated
return ""
}

func buildUpstreamName(host string, b interface{}, loc interface{}) string {
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need host and b arguments anymore

ElvinEfendi
ElvinEfendi reviewed Oct 10, 2018
View reviewed changes
rootfs/etc/nginx/template/nginx.tmpl Outdated
@@ -802,7 +755,7 @@ stream {
ssl_stapling_verify on;
{{ end }}

{{ if and (not $all.DisableLua) $all.DynamicCertificatesEnabled}}
{{ if and $all.DynamicCertificatesEnabled}}
Copy link
Member

Choose a reason for hiding this comment

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

not need for and

ElvinEfendi
ElvinEfendi reviewed Oct 10, 2018
View reviewed changes
rootfs/etc/nginx/template/nginx.tmpl Outdated
@@ -1083,7 +1030,7 @@ stream {
{{ end }}

{{/* By default use vhost as Host to upstream, but allow overrides */}}
{{ if not (empty $location.UpstreamVhost) }}
{{ if not (empty $location.UpstreamVhost) }}s
Copy link
Member

Choose a reason for hiding this comment

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

redundant s in the end of line

@aledbf aledbf removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2018
@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 Oct 10, 2018
@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

@k8s-ci-robot k8s-ci-robot merged commit 3edf11b into kubernetes:master Oct 10, 2018
@aledbf aledbf deleted the only-dynamic branch October 10, 2018 13:03
@chen-anders chen-anders mentioned this pull request Oct 19, 2018
Closed
@abhaysingh1722 abhaysingh1722 mentioned this pull request Mar 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElvinEfendi ElvinEfendi ElvinEfendi left review comments

@diazjf diazjf diazjf left review comments

@antoineco antoineco Awaiting requested review from antoineco

Assignees

@ElvinEfendi ElvinEfendi

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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aledbf @ElvinEfendi @k8s-ci-robot @diazjf