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.

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

Adds new flag --metrics-host #811

Merged
merged 4 commits into from
Aug 8, 2023
Merged

Adds new flag --metrics-host #811

merged 4 commits into from
Aug 8, 2023

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Aug 3, 2023

Kured runs with hostNetwork=true, which means that, today, Kured will expose the metrics endpoint on what is probably a public interface for many deployments. The metrics are generally not sensitive data, but as a policy it seems wrong to do this. Allowing the user to configure which port Kured exposes metrics on is a step in the right direction, but the current implementation does not allow the user to specify the address to listen on. For example, in our use case, we would like for the metrics server to only listen on a loopback address. This would allow us to put kube-rbac-proxy in front of the metrics endpoint.

This change could slightly complicate the Helm chart because it not only affects the metrics endpoint, but also the readinessProbe configuration.

@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 3, 2023 •
edited
Loading

I just noticed that a PR was merged yesterday which set hostNetwork=false by default in the chart. That change may obviate the need for this PR, at least for our use case. However, in cases where someone does for some reason want to run with hostNetwork=true it may still be useful for them to be able to specify the full listen address. There may be other reasons I'm not thinking of.

ckotzbauer
ckotzbauer requested changes Aug 4, 2023
View reviewed changes
Copy link
Member

@ckotzbauer ckotzbauer left a comment

Choose a reason for hiding this comment

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

Hi @nkinkade,
thanks for your PR. Yes, since a few days, the hostNetwork=true setting is not the default anymore, so this problem should not occur too often.

But, you are right, it may be good in some cases to also configure the host of the interface to listen on. However, the --metrics-port flag was released with 1.13.2, so this PR would be a breaking change, which we want to avoid. Instead of replacing the flag it would be good to add an additional --metrics-host flag to kured (and also to the chart).

@nkinkade nkinkade changed the title Replaces flag --metrics-port with --metrics-addresss Adds new flag --metrics-host Aug 4, 2023
@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 4, 2023

@ckotzbauer: I have modified the PR and its title. The changes now only add a new flag --metrics-host.

I am happy to contribute a corresponding PR to the charts repo, once I have confirmation from you that this change looks okay.

Thank you.

@ckotzbauer
Copy link
Member

The code looks good, thanks!
Can you please

  • signoff all commits to our repos?
  • open a PR against the chart-repository
  • open a PR against the website-repository and document the flag

Thanks!!

nkinkade added a commit to nkinkade/kured-charts that referenced this pull request Aug 7, 2023
@nkinkade
Adds new flag --metrics-host to chart
e250d47 
kubereboot/kured#811
nkinkade added a commit to nkinkade/kured-charts that referenced this pull request Aug 7, 2023
@nkinkade
Adds new flag --metrics-host to chart
57edac7 
kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade nkinkade mentioned this pull request Aug 7, 2023
Merged
nkinkade added a commit to nkinkade/kured-website that referenced this pull request Aug 7, 2023
@nkinkade
Documents new --metrics-host flag
3b1ff46 
kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade nkinkade mentioned this pull request Aug 7, 2023
Merged
@nkinkade
Revert "Replaces flag --metrics-port with --metrics-addresss"
955b79d 
This reverts commit 528c7bb.

Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade
Adds new --metrics-host flag
f04db81 
The flag --metrics-port already exists. While not as clean, to avoid
introducing a backward incompatible change to flags, this commit adds a
new --metrics-host flag, which in combination with the existing
--metrics-port flag can define a complete listen address for the metrics
server as "<metrics-host>:<metrics-port>"

Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade
Adds new, commented flags --metrics-{port,host}
e5d94c4 
Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 7, 2023

@ckotzbauer:

Let me know if I missed anything.

Thank you!

@ckotzbauer ckotzbauer added this to the 1.14.0 milestone Aug 8, 2023
@ckotzbauer
Copy link
Member

Thanks for the changes!!

@ckotzbauer ckotzbauer merged commit 351ca71 into kubereboot:main Aug 8, 2023
11 checks passed
ckotzbauer pushed a commit to kubereboot/website that referenced this pull request Aug 19, 2023
@nkinkade
doc: Documents new --metrics-host flag (#69)
9b0429a 
kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
ckotzbauer added a commit to kubereboot/charts that referenced this pull request Aug 19, 2023
@nkinkade @ckotzbauer
Adds new flag --metrics-host to chart (#50)
3763d87 
kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>
@ckotzbauer ckotzbauer mentioned this pull request Aug 19, 2023
Merged
ckotzbauer added a commit to kubereboot/website that referenced this pull request Aug 21, 2023
@ckotzbauer @nkinkade
Release 1.14.0 (#76)
646869b 
* doc: add drain-pod-selector (#71)

Signed-off-by: Christian Kotzbauer <[email protected]>

* doc: Documents new --metrics-host flag (#69)

kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>

* doc: add version range

Signed-off-by: Christian Kotzbauer <[email protected]>

* doc: add new arguments

Signed-off-by: Christian Kotzbauer <[email protected]>

---------

Signed-off-by: Christian Kotzbauer <[email protected]>
Signed-off-by: Nathan Kinkade <[email protected]>
Co-authored-by: nkinkade <[email protected]>
ckotzbauer added a commit to kubereboot/charts that referenced this pull request Aug 21, 2023
@ckotzbauer @docbobo @nkinkade
Release 1.14.0 (#53)
846e8ed 
* expose --drain-pod-selector (#43)

Signed-off-by: Boris Pruessmann <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>

* Adds new flag --metrics-host to chart (#50)

kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>

* feat: changes for 1.14.0

Signed-off-by: Christian Kotzbauer <[email protected]>

---------

Signed-off-by: Boris Pruessmann <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Signed-off-by: Nathan Kinkade <[email protected]>
Co-authored-by: Boris Prüßmann <[email protected]>
Co-authored-by: nkinkade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckotzbauer ckotzbauer ckotzbauer approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nkinkade @ckotzbauer