-
Notifications
You must be signed in to change notification settings - Fork 476
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
[kong] add controller to service monitor #430
Conversation
Add the static controller metrics port (10255) to the controller container and service monitor. Only include this port if the controller image is a supported version (2.0.0 or higher). Add a "kong.controller2xplus" helper which returns (string) "true" if the controller major version is 2 or higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change imposes new requirements on values. The controller image tag must be semver-compliant. The template will completely fail to render if it is not.
Hm, this will not work nicely with custom builds and with our tags such as next
.
What do you think about something like this:
- add a toggle to values.yaml that will control whether the controller is included in the service monitor, with the following semantics:
- if unset, defaults to checking the semver version from the tag - if inconclusive, throwing a human-readable error suggesting that the user set the setting
- if enabled/disabled: enabling/disabling the controller port in the service monitor
- control rendering of the controller endpoint in the service monitor based solely on the value of that toggle
Bah, yeah, For custom builds, the ask is that you use semver metadata. If you want to distinguish your custom build in the tag, and not just by using your registry, you'd have something like We could use something compatible for our bleeding edge tags also ( I don't want to add toggles for individual version-dependent features that we want to have on by default on compatible versions. You shouldn't have to cross-reference the chart feature set against controller release notes, and shouldn't have to walk through all the values.yaml settings to toggle on things that you'd expect to be on by default. I can add an optional user-set value for the controller semver version. If you provide it, we use that for the semver input instead of the tag. You'd only set it if you are using something like |
After 98e50e6:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of the attached comments may show my helm templating language ignorance 🤷♂️
{{- else -}} | ||
{{- $version = semver .Values.ingressController.image.tag -}} | ||
{{- end -}} | ||
{{- ge $version.Major 2 -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this template parametric in the following way, for reusability:
- accept the major version as parameter
- accept the
image
object as a parameter
so that it's reusable if we, say, need to check elsewhere if the gateway has major version >= 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at this briefly and I think I'd like to defer it for now. This function is intentionally a bit weird because we want this to work for the beta, and for that the major version comparison is necessary over a more conventional semver.Compare
: 2.0.0-beta.1
is not greater than 2.0.0
(and IIRC it's not even greater than 1.3
, ignoring that using that would cause issues if we eventually released 1.4). Realistically our more immediate need, if any, will be to check if the controller version is >= 2.1, not >= 3.
We could work around that by converting to semver first, extracting the major, and passing that in, but IMO more work than its worth at this point--makes more sense to release a simpler version for now and then refactor into something more reusable after 2.0 GA.
Making a "function" in template land is a bit cumbersome, since parameters aren't really a thing--you have only a single parameter, so you have to construct a dict on the fly and don't have the benefit of signature checking. It's clunky but doable.
Whether we truly want to do that is an open question--my general experience with developing entirely in templates is that it's an exercise in masochism. This is a good example, where we have odd workarounds for not having proper functions and types and lack access to the full feature set of the underlying semver library. I think a reasonable next major project is to begin porting chart functionality into a full Go operator that handles complex decisions in Go and only uses templates for a final "fill in the exact given values" step to create a manifest.
What this PR does / why we need it:
Adds the controller metrics port to the controller container template and Prometheus ServiceMonitor template. The port only appears if the controller's major version is >= 2.
Which issue this PR fixes
Part of Kong/kubernetes-ingress-controller#1497
Special notes for your reviewer:
This change imposes new requirements on values. The controller image tag must be semver-compliant. The template will completely fail to render if it is not.
This does not pose issues for our stock image tags, but may affect users who use custom controller images. If they do not currently use semver tags, they must make them semver compliant. IMO this is a reasonable ask: they can use the base image version and include any additional info relevant to their build using semver metadata.
The helper output is annoyingly considered a string, with no available function to convert it to a boolean. Go templates are cumbersome.
Checklist
next
branch and targetsnext
, notmain
[kong]
)