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

Limit serving of insecure metrics by allowing configurable IP #275

Conversation

swatisehgal
Copy link
Collaborator

@swatisehgal swatisehgal commented Apr 4, 2024

Currently we are serving insecure metrics on all IPv4 routable addresses on the local machine (0.0.0.0).

In this PR, we make the metric IP configurable so that in order to ensure that we listen for insecure metrics port only ona configurable IP.

@swatisehgal swatisehgal requested a review from ffromani April 4, 2024 12:16
@swatisehgal swatisehgal changed the title Limit serving of insecure metrics to one IP (localhost by default) WIP: Limit serving of insecure metrics to one IP (localhost by default) Apr 4, 2024
@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch 3 times, most recently from ecbe724 to 69b7776 Compare April 4, 2024 12:46
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

LGTM

minor comments

cmd/resource-topology-exporter/main.go Outdated Show resolved Hide resolved
pkg/config/defaults.go Outdated Show resolved Hide resolved
pkg/config/environ.go Outdated Show resolved Hide resolved
pkg/metrics/server/setup.go Outdated Show resolved Hide resolved
pkg/metrics/server/setup.go Show resolved Hide resolved
@ffromani
Copy link
Contributor

ffromani commented Apr 4, 2024

the e2e failure is curl failing with error 35 which is SSL connection error:
https://serverfault.com/questions/606135/curl-35-ssl-connect-error

@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch 2 times, most recently from ad3d191 to 3370707 Compare April 4, 2024 15:26
@swatisehgal swatisehgal changed the title WIP: Limit serving of insecure metrics to one IP (localhost by default) WIP: Limit serving of insecure metrics by allowing configurable IP Apr 4, 2024
@ffromani
Copy link
Contributor

ffromani commented Apr 4, 2024

curl being obsolete is unlikely the culprit. Let's add -v to all the curl commandlines (curl -v -L ...) to have more debugging infos

@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch from 3370707 to e7f52fa Compare April 4, 2024 15:37
@swatisehgal
Copy link
Collaborator Author

curl being obsolete is unlikely the culprit. Let's add -v to all the curl commandlines (curl -v -L ...) to have more debugging infos

Ack, will do now.

@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch 7 times, most recently from cafb1cf to 2fb3202 Compare April 4, 2024 17:10
@swatisehgal
Copy link
Collaborator Author

the e2e failure is curl failing with error 35 which is SSL connection error: https://serverfault.com/questions/606135/curl-35-ssl-connect-error

  [FAILED] failed exec command on pod. pod="default/resource-topology-exporter-ds-cnkcw"; cmd=["curl" "-v" "-L" "https://127.0.0.1:2112/metrics"]; err=failed to run command [curl -v -L https://127.0.0.1:2112/metrics]: command terminated with exit code 35; stderr=""
  Unexpected error:
      <*fmt.wrapError | 0xc0002882c0>: 
      failed to run command [curl -v -L https://127.0.0.1:2112/metrics]: command terminated with exit code 35
      {
          msg: "failed to run command [curl -v -L https://127.0.0.1:2112/metrics]: command terminated with exit code 35",
          err: <exec.CodeExitError>{
              Err: <*errors.errorString | 0xc0000694c0>{
                  s: "command terminated with exit code 35",
              },
              Code: 35,
          },
      }
  occurred

I tried a few things suggested in the link you recommended as well as a few others but didn't have much luck.
Will investigate this tomorrow with a fresh mind.

Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

weird that -v doesn't give us more output, but I think I found the culprit anyway

test/e2e/rte/metrics.go Outdated Show resolved Hide resolved
test/e2e/rte/metrics.go Outdated Show resolved Hide resolved
test/e2e/rte/metrics.go Outdated Show resolved Hide resolved
@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch 2 times, most recently from ba4a620 to 1db2cad Compare April 5, 2024 09:46
@swatisehgal swatisehgal changed the title WIP: Limit serving of insecure metrics by allowing configurable IP Limit serving of insecure metrics by allowing configurable IP Apr 5, 2024
@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch from 1db2cad to a28c78f Compare April 5, 2024 09:50
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

almost there, let's fix a minor error issue which survived the last round of updates and we can merge (and begin the backport dance)

pkg/metrics/server/setup.go Outdated Show resolved Hide resolved
@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch from a28c78f to d6d3428 Compare April 5, 2024 10:26
Currently we are serving insecure metrics on all IPv4
routable addresses on the local machine (0.0.0.0).

In this PR, we make the metric IP configurable in order to ensure
that we listen for insecure metrics port only on one IP to reduce
security vulnerability.

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal swatisehgal force-pushed the serve-insecure-metrics-on-localhost branch from d6d3428 to 1042edc Compare April 5, 2024 10:46
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

thanks, LGTM!

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.

2 participants