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

[404-server] Conceal /metrics and /healthz behind port 10254 #3125

Merged
merged 1 commit into from
Sep 26, 2018
Merged

[404-server] Conceal /metrics and /healthz behind port 10254 #3125

merged 1 commit into from
Sep 26, 2018

Conversation

jonpulsifer
Copy link
Contributor

@jonpulsifer jonpulsifer commented Sep 25, 2018

What this PR does / why we need it:

  • Implements an HTTP multiplexer for the /metrics and /healthz endpoints and exposes a command line flag to change the port (default 10254)
  • Implements a timeout (default 5s) for the graceful shutdown

Which issue this PR fixes:

Fixes #1733

Special notes for your reviewer:

This PR is getting pretty big.. I don't know if we should cut a release yet, or wait until we bump the manifests? code has been updated to maintain existing functionality, so i don't think i'm that worried anymore

The 404-server is supposed to be small, but it's getting bigger, I don't know what we want to do here

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 25, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 25, 2018
@jonpulsifer
Copy link
Contributor Author

/assign @aledbf

@jonpulsifer
Copy link
Contributor Author

jonpulsifer commented Sep 25, 2018

I guess the real question is, does anyone use these metrics? This is supposed to just return http.StatusNotFound and the default backend - 404 string.

I don't know why but I just feel wrong feature creeping this thing

@aledbf
Copy link
Member

aledbf commented Sep 25, 2018

I don't know why but I just feel wrong feature creeping this thing

I am not sure this adds real value for two reasons, just a counter for 404 is not very helpful and if you add the host being requested could produce an explosion of metrics.
Maybe this PR should be referenced as an example for those users that want this feature? (building a custom 404 image is easy). WDYT?

@ElvinEfendi
Copy link
Member

IMO ingress-nginx should not be concerned about default backend service at all. By default we should not require --default-backend-service to be set and just return 404 from Nginx directly. And if a user wants to customize then they can deploy their custom default backend service and use --default-backend-service to configure it.

@aledbf
Copy link
Member

aledbf commented Sep 25, 2018

By default we should not require --default-backend-service to be set and just return 404 from Nginx directly.

We cannot break this rule.

@ElvinEfendi
Copy link
Member

We cannot break this rule.

@aledbf can you elaborate? Are you worried that it will introduce snowflake in controller.go logic?

@jonpulsifer
Copy link
Contributor Author

Maybe this PR should be referenced as an example for those users that want this feature? (building a custom 404 image is easy). WDYT?

I'm totally comfortable with that, I kinda just want to 🔥 the metrics endpoint altogether? I'm okay with /healthz being public from a security perspective

@ElvinEfendi
Copy link
Member

I sketched something quickly for the idea I suggest at #3125 (comment), it is simpler than I guessed: https://github.com/kubernetes/ingress-nginx/pull/3126/files

@aledbf
Copy link
Member

aledbf commented Sep 25, 2018

I'm totally comfortable with that, I kinda just want to fire the metrics endpoint altogether?

Not sure about that because someone could be using what's being exposed right now.

@aledbf
Copy link
Member

aledbf commented Sep 25, 2018

Closing

@aledbf aledbf closed this Sep 25, 2018
@jonpulsifer jonpulsifer deleted the change-default-backend-metrics-port branch September 25, 2018 16:16
@ElvinEfendi
Copy link
Member

ElvinEfendi commented Sep 26, 2018

@aledbf what's the problem with this PR? It is making things better without breaking anything, no? After #3126 gets merged https://github.com/kubernetes/ingress-nginx/tree/master/images/404-server will serve just as an example, and I don't see any issue with making that example custom 404 page more secure and configurable.

@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

@ElvinEfendi this introduce the same issue than here #3116 (comment) but for all 404 handled by the default backend

@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

Also, the 404-server image is not used only by ingress-nginx

@jonpulsifer
Copy link
Contributor Author

the 404-server aka k8s.gcr.io/defaultbackend is also the backend used by the https://github.com/kubernetes/ingress-gce on GKE, you can find it in the kube-system namespace as deploy/l7-default-backend

@jonpulsifer
Copy link
Contributor Author

/reopen

@k8s-ci-robot
Copy link
Contributor

@jonpulsifer: Reopening this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

@jonpulsifer: failed to re-open PR: state cannot be changed. The change-default-backend-metrics-port branch has been deleted.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jonpulsifer jonpulsifer restored the change-default-backend-metrics-port branch September 26, 2018 18:53
@jonpulsifer
Copy link
Contributor Author

/reopen

@k8s-ci-robot
Copy link
Contributor

@jonpulsifer: Reopening this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Sep 26, 2018
@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 26, 2018
@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2018
@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

@bowei @nicksardo ping

@nicksardo nicksardo requested review from bowei and removed request for nicksardo September 26, 2018 19:16
This commit conceals the /metrics endpoint behind port 10254.

Thanks to @aledbf for all the help getting this through!

Signed-off-by: Jonathan Pulsifer <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2018
@jonpulsifer
Copy link
Contributor Author

jonpulsifer commented Sep 26, 2018

commits have been squashed, ready to go I think 😄

also super thanks for the incorporated adjustments to the code (retaining functionality on :8080/healthz and the rest of the golang stuff)

@bowei
Copy link
Member

bowei commented Sep 26, 2018

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, bowei, jonpulsifer

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 aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2018
@k8s-ci-robot k8s-ci-robot merged commit 50bb789 into kubernetes:master Sep 26, 2018
@aledbf
Copy link
Member

aledbf commented Sep 26, 2018

@bowei can you help with the publication of the image to gcr?

@bowei
Copy link
Member

bowei commented Sep 27, 2018

ahhhh ok, doing it

@NicoleG25
Copy link

It appears that CVE-2018-1002104 was assigned to this issue.

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[nginx] Metrics of the defaultbackend
6 participants