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

Fix envoy bootstrap logic to not append multiple self_admin clusters #8371

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

crhino
Copy link
Contributor

@crhino crhino commented Jul 23, 2020

Previously, the envoy bootstrap config would blindly copy the self_admin
cluster into the list of static clusters when configuring either
ReadyBindAddr, PrometheusBindAddr, or StatsBindAddr.

Since ingress gateways always configure the ReadyBindAddr property,
users run into this case much more often than previously.

Previously, the envoy bootstrap config would blindly copy the self_admin
cluster into the list of static clusters when configuring either
ReadyBindAddr, PrometheusBindAddr, or StatsBindAddr.

Since ingress gateways always configure the ReadyBindAddr property,
users ran into this case much more often than previously.
@crhino crhino added type/bug Feature does not function as expected backport/1.8 theme/ingress-gw Track ingress work labels Jul 23, 2020
@crhino crhino requested a review from a team July 23, 2020 17:13
clusterJSON = ",\n" + clusterJSON
// Make sure we do not append the same cluster multiple times, as that will
// cause envoy startup to fail.
selfAdminClusterExists, err := containsSelfAdminCluster(args.StaticClustersJSON)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started off by just doing a strings.Contains(args.StaticClustersJSON, clusterJSON), but decided to actually parse out the cluster names to validate it. I think the string comparison would've worked just fine, but parsing out the JSON and checking cluster names felt more semantically correct.

Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just tried it out locally.

Thanks for jumping on this!

@crhino crhino merged commit 7c4cc71 into master Jul 23, 2020
@crhino crhino deleted the bug/deduplicate-envoy-bootstrap-cluster branch July 23, 2020 18:12
@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 7c4cc71 onto release/1.8.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Jul 23, 2020
…8371)

Previously, the envoy bootstrap config would blindly copy the self_admin
cluster into the list of static clusters when configuring either
ReadyBindAddr, PrometheusBindAddr, or StatsBindAddr.

Since ingress gateways always configure the ReadyBindAddr property,
users ran into this case much more often than previously.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ingress-gw Track ingress work type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants