-
Notifications
You must be signed in to change notification settings - Fork 131
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
inherit nodeReporterPort from felixconfig #1227
inherit nodeReporterPort from felixconfig #1227
Conversation
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.
Looking good, maybe one validation to add to the controller?
53ba19c
to
e492dff
Compare
|
||
if nodeReporterMetricsPort == 0 { | ||
err := errors.New("felixConfiguration prometheusReporterPort=0 not supported") | ||
r.SetDegraded("", err, reqLogger) |
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.
I think we need to have something in the 1st field because that is the Degraded "reason", I think saying "Invalid metrics port" would work and you can keep the error message as you have it.
66738d5
to
babde82
Compare
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.
LGTM
Description
Some of our users are setting the PrometheusReporterPort in FelixConfiguration (Enterprise) to prevent port conflicts with other services on their hosts. That felixconfig option gets overwritten by the env var Operator sets.
This PR enables Operator to check if the value is set in the FelixConfiguration and use that value instead of its hardcoded default.
For PR author
make gen-files
make gen-versions
For PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bug
if this is a bugfix.kind/enhancement
if this is a a new feature.enterprise
if this PR applies to Calico Enterprise only.