added rest xds server for endpoints #3799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
keep pr updated
|
Issues linked to changelog: |
kdorosh
suggested changes
Nov 2, 2020
kdorosh
reviewed
Nov 2, 2020
changelog/v1.6.0-beta6/rest-eds.yaml
Outdated
Comment on lines
10
to
13
| - type: FIX | ||
| issueLink: https://github.com/solo-io/gloo/issues/3673 | ||
| description: Fix EDS so modifying health checks on Upstream no longer results in 503s | ||
| resolvesIssue: true |
kdorosh
reviewed
Nov 2, 2020
install/test/helm_suite_test.go
Outdated
Comment on lines
312
to
313
| // file, err := ioutil.ReadFile(fixtureName) | ||
| // ExpectWithOffset(1, err).NotTo(HaveOccurred()) |
kdorosh
reviewed
Nov 2, 2020
projects/gloo/api/v1/settings.proto
Outdated
| // Some examples are: | ||
| // 1. https://github.com/solo-io/gloo/issues/3673 | ||
| // 2. https://github.com/solo-io/gloo/issues/3710 | ||
| // 3. https://github.com/solo-io/gloo/issues/3673 |
There was a problem hiding this comment.
this is duplicated. perhaps link to the envoy issue instead?
kdorosh
reviewed
Nov 2, 2020
kdorosh
reviewed
Nov 2, 2020
changelog/v1.6.0-beta6/rest-eds.yaml
Outdated
| - type: NEW_FEATURE | ||
| issueLink: https://github.com/solo-io/gloo/issues/3805 | ||
| description: >- | ||
| Allow toggling of EDS to rest XDS to avoid the envoy issue described in the following issue: https://github.com/envoyproxy/envoy/issues/13070. |
There was a problem hiding this comment.
what is the default? this was a change from the original behavior before, correct?
kdorosh
reviewed
Nov 2, 2020
|
Issues linked to changelog: |
kdorosh
reviewed
Nov 2, 2020
| @@ -15,6 +15,7 @@ spec: | |||
| {{- else }} | |||
| xdsBindAddr: "0.0.0.0:{{ .Values.gloo.deployment.xdsPort }}" | |||
| restXdsBindAddr: "0.0.0.0:{{ .Values.gloo.deployment.restXdsPort }}" | |||
| enableRestEds: true | |||
There was a problem hiding this comment.
we should have a helm value for this
kdorosh
reviewed
Nov 2, 2020
| // Defaults to `0.0.0.0:9976` | ||
| string rest_xds_bind_addr = 11; | ||
|
|
||
| // Whether or not to use rest xds for all EDS by default. | ||
| // Set to true by default in versions > `v1.6.0`. |
There was a problem hiding this comment.
only set to true in helm installations, we should fix the implementation to handle nil as true
kdorosh
approved these changes
Nov 2, 2020
kdorosh
previously approved these changes
Nov 2, 2020
kdorosh
added
the
work in progress
EItanya
removed
the
work in progress
kdorosh
reviewed
Nov 2, 2020
| @@ -66,7 +66,7 @@ require ( | |||
| github.com/solo-io/reporting-client v0.1.2 | |||
| github.com/solo-io/skv2 v0.8.1 | |||
| github.com/solo-io/solo-apis v0.0.0-20200717214114-6a1daa5a5d05 | |||
| github.com/solo-io/solo-kit v0.13.13 | |||
| github.com/solo-io/solo-kit v0.13.14 | |||
There was a problem hiding this comment.
nothing in the changelog for the codegen related changes from this?
There was a problem hiding this comment.
I can add a DEPENDENCY_BUMP. This fixed some errors in the rest xds server
kdorosh
reviewed
Nov 2, 2020
| @@ -15,6 +15,7 @@ spec: | |||
| {{- else }} | |||
| xdsBindAddr: "0.0.0.0:{{ .Values.gloo.deployment.xdsPort }}" | |||
| restXdsBindAddr: "0.0.0.0:{{ .Values.gloo.deployment.restXdsPort }}" | |||
| enableRestEds: {{ .Values.settings.enableRestEds | default true }} | |||
There was a problem hiding this comment.
i don't understand why we need the default -- shouldn't the generated values.go file already have the true value?
kdorosh
previously approved these changes
Nov 2, 2020
kdorosh
approved these changes
Nov 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Allow toggling of EDS to rest XDS to avoid the envoy issue where updates to CDS require new endpoints from the control-plane
Context
envoyproxy/envoy#13070.
Checklist:
make install-go-tools generated-codeto ensure there will be no code diffBOT NOTES:
resolves [Migrated] Modifying configs on a TLS enabled Upstream results in 503s solo-io/gloo#3673
resolves [Migrated] Empty upstreams solo-io/gloo#3710
resolves [Migrated] modifying healthcheck config while in unhealthy state stops future healthchecks solo-io/gloo#3219
resolves [Migrated] Enable rest XDS for endpoints to fix race condition in envoy solo-io/gloo#3805