-
Notifications
You must be signed in to change notification settings - Fork 672
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
Inspect loadbalancer address from Envoy ingress objects #5987
Conversation
Hi @hligit! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace |
293a3da
to
98835b0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5987 +/- ##
==========================================
- Coverage 78.62% 78.28% -0.34%
==========================================
Files 138 138
Lines 19631 19809 +178
==========================================
+ Hits 15434 15508 +74
- Misses 3894 3998 +104
Partials 303 303
|
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.
Hi @hligit, thanks for contributing, and continuing @kahirokunn's work!
I had small initial finding while doing a quick manual testing. Please check comments inline!
Since the new flag covers functionality of some existing flags, if both --load-balancer-status and --ingress-status-address or envoy-service-name/namespace are specified, only value of --load-balancer-status is used and a warning message is logged.
I think that for backwards compatibility, even in case of configuration options like these, it is "safer" to give priority for the old method and not the other way around. Often it may not make big difference, but in this case as the new option got assigned with default values, old option just stops working. It may be simplest just to swap the precedence and prioritise old options first.
cmd/contour/serve.go
Outdated
} else { | ||
if lbAddress := contourConfiguration.Ingress.StatusAddress; len(lbAddress) > 0 { | ||
s.log.WithField("loadbalancer-address", lbAddress).Info("Using supplied information for Ingress status") | ||
lbsw.lbStatus <- parseStatusFlag(lbAddress) | ||
} else { | ||
// Register an informer to watch envoy's service if we haven't been given static details. | ||
serviceHandler := &k8s.ServiceStatusLoadBalancerWatcher{ | ||
ServiceName: contourConfiguration.Envoy.Service.Name, | ||
LBStatus: lbsw.lbStatus, | ||
Log: s.log.WithField("context", "serviceStatusLoadBalancerWatcher"), | ||
} | ||
|
||
var handler cache.ResourceEventHandler = serviceHandler | ||
if contourConfiguration.Envoy.Service.Namespace != "" { | ||
handler = k8s.NewNamespaceFilter([]string{contourConfiguration.Envoy.Service.Namespace}, handler) | ||
} | ||
|
||
if err := s.informOnResource(&corev1.Service{}, handler); err != nil { | ||
s.log.WithError(err).WithField("resource", "services").Fatal("failed to create services informer") | ||
} | ||
|
||
s.log.WithField("envoy-service-name", contourConfiguration.Envoy.Service.Name). | ||
WithField("envoy-service-namespace", contourConfiguration.Envoy.Service.Namespace). | ||
Info("Watching Service for Ingress status") | ||
} | ||
} | ||
|
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.
If we parse the old options also into envoyLoadBalancerStatus
struct, we could handle them in the same code as the new options are handled (above this block) and this code could be deleted.
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.
Good idea. I've changed as suggested. Also extracted the block into its own function.
cmd/contour/serve.go
Outdated
ServiceName: contourConfiguration.Envoy.Service.Name, | ||
LBStatus: lbsw.lbStatus, | ||
Log: s.log.WithField("context", "serviceStatusLoadBalancerWatcher"), | ||
if contourConfiguration.Envoy.LoadBalancer != "" { |
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.
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.
Good catch! Thanks! I have removed setting value in Defaults()
.
Also changed the order of flags by inspecting if ingress-status-address is empty, than the new flag and lastly the envoy-service-name
and envoy-service-namespace
, because they always have default values set.
Do you think this is good enough or we should check if the envoy service flags are explicitly set and should take precedence of the new flags?
…rojectcontour#4952) Signed-off-by: kahirokunn <okinakahiro@gmail.com>
98835b0
to
c3ed0b8
Compare
Signed-off-by: Haitao Li <hli@atlassian.com>
c3ed0b8
to
9640ce1
Compare
@@ -253,6 +253,16 @@ type EnvoyConfig struct { | |||
// +optional | |||
Service *NamespacedName `json:"service,omitempty"` | |||
|
|||
// LoadBalancer specifies how Contour should set the ingress status address. | |||
// If provided, the value can be in one of the formats: | |||
// - hostname:<address,...>: Contour will use the provided comma separated list of addresses directly. |
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.
currently we support setting ingress status address via config flag/parameter here and it can be an IP or hostname
would be good to keep that functionality (and not just support hostnames) if we intend to deprecate the old method of configuring this
looks like that might just mean a naming change here and elsewhere since existing helpers are being used
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
keep |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
keep |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
keep |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
keep |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
Contour sets load balancer address in Ingress/HTTPProxy object's status. The load balancer address is extracted from Envoy
Service
object's status. This doesn't work in configuration that EnvoyService
is of typeClusterIP
and an EnvoyIngress
object is used for inbound. This patch adds support for inspecting the EnvoyIngress
object for setting load balancer address.A new flag
--load-balancer-status
is introduced as suggested by @tsaarni in #5083 (comment)Flag
--load-balancer-status
takes value in one of below formats:hostname:fqdn1[,fqdn2]
: This provides the same functionality as--ingress-status-address
.sevice:namespace/name
: same as flagsenvoy-service-name
andenvoy-service-namespace
combined.ingress:namespace/name
: new feature for getting address from Ingress objects.Since the new flag covers functionality of some existing flags, if both
--load-balancer-status
and--ingress-status-address
orenvoy-service-name/namespace
are specified, only value of--load-balancer-status
is used and a warning message is logged.This is based on kahirokunn's PR #5083 .
Fixes #4952