Inline Submodules Part 3 #466
Open
Conversation
This file contains hidden or 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
Assertions for table tests need to happen in the function argument, which gets run once for each entry. The prior implementation had the assertion in a separate It block, which only ran once, after all the entry tests were run first. The entry tests, however, had no actual assertions in them.
Signed-off-by: Josh Russett <[email protected]>
It's unclear if this behavior is actually desired, but let's at least encode current behavior in a test. If we want to change this, we can update the test to describe what we'd actually like to happen under these circumstances.
…egardless of AZ) Prior to this change, if all AZ-local endpoints had errors, roundrobin was guaranteed to return a non-AZ-local endpoint. With this change, we reset the failedAt field on all the endpoints as soon as AZ-local endpoints have been exhausted, meaning there is a possibility an AZ-local endpoint is still returned. Signed-off-by: Josh Russett <[email protected]>
Signed-off-by: David Sabeti <[email protected]>
This drove out a fairly large refactor, mostly because the logic for checking whether the endpoints were all overloaded needed to happen at the end of the for-loop along with the other logic for running out of endpoints (in the other case, due to all endpoints having been failed). We pushed on and added additiona refactors, including helper functions and rejiggering some logic for improved legibility Signed-off-by: Josh Russett <[email protected]>
We are not expecting determinstic behavior, so we can remove this line as per the inline documentation: ``` // Deprecated: As of Go 1.20 there is no reason to call Seed with // a random value. Programs that call Seed with a known value to get // a specific sequence of results should use New(NewSource(seed)) to // obtain a local random generator. ``` Source: https://cs.opensource.google/go/go/+/refs/tags/go1.21.6:src/math/rand/rand.go;l=390-393
- Calculate `curIsLocal` before incrementing `curIdx` for legibility - Add more comments I tried to make the code more legible by adding a `nextIndex` variable and not pre-incrementing the index before the end of the for loop but it added a bit more of a performance hit than I would have liked. Instead I settled on adding a comment to hopefully make the logic clearer.
Actually move the curIsLocal calculation up this time.
Signed-off-by: David Sabeti <[email protected]>
* Fix ability to start gorouter with the default config - No longer require health listtener TLS cert configs when launching gorouter with a default config in test-mode - Throw errors in config parsing + router logic if neither http nor https health listeners are enabled. - Fix test names and bad URL mgmt for health listener tests. routing-release requires the TLS certificate to be set still. cf-deployment generates this and passes it in. This change should only affect testing of gorouter. * Update router/router.go Co-authored-by: Dominik Froehlich <[email protected]> --------- Co-authored-by: Dominik Froehlich <[email protected]>
See issue: golang/go#65123 ReverseProxy configures Got1xxResponse trace hook. We configure ReverseProxy with our ProxyRoundTripper. ProxyRoundTripper eventually calls http.Transport. http.Transport runs readLoop for each connection in a separate goroutine. When RoundTrip is called readLoop will run Got1xxResponse hook. If there are no errors during request handling, RoundTrip waits for readLoop to finish. If there is an error though RoundTrip exits early and does not wait for readLoop to finish. This results in readLoop goroutine running in parallel and we get a data race in our ErrorHandler which modifies response headers at the same time as Got1xxResponse hook. This error results in concurrent map writes and not panic, which is not caught by panic handler making Gorouter fail and drop all connections. This code can be removed once the ReverseProxy issue is fixed.
Make sure that the callback and error handler are executed not concurrently.
…oundry#391) Use golang's supported CipherSuites() and InsecureCipherSuites() to populate our mapping of cipher suite names to ID. However we have supported legacy cipher suite names, as well as openssl cipher suite names in the past, so those remain explicitly added.
…foundry#392) * Setup sticky session for Kerberos and NTML HTTP Authentication When server responds with `WWW-Authenticate: Negotiate`, save VCAP_ID cookie on response to client so that subsequent request with `Authorization: Negotiate ...` will be directed to the same application instance. See [RFC-4559](https://www.ietf.org/rfc/rfc4559.txt) Signed-off-by: Josh Russett <[email protected]> * Set default VCAP_ID cookie properties during Authenticate Negotiate Signed-off-by: Geoff Franks <[email protected]> * Add debug logs when choosing an endpoint Signed-off-by: Maria Shaldybin <[email protected]> --------- Signed-off-by: Josh Russett <[email protected]> Signed-off-by: Geoff Franks <[email protected]> Signed-off-by: Maria Shaldybin <[email protected]> Co-authored-by: Josh Russett <[email protected]> Co-authored-by: Geoff Franks <[email protected]>
…zation: negotiate' flows (cloudfoundry#393)
The `Partitioned` flag is used for cookies that are set on web sites embedded via iframes. The cookie is then available only in combination of the host site and the embedded site. Golang's `http.Cookie` type does not yet support the `Partitioned` flag, but Google Chrome is already testing mandatory support and rejecting/omitting cookies without it for 1% of users via A/B roll-out. The implementation wraps the `http.Cookie` and extends it with the `Partitioned` field. This field is then used to forward the raw cookie string when creating the derived VCAP_ID cookies for sticky sessions. Once the Golang standard library supports the `Partitioned` flag, this wrapper can just be removed. A test that checks the `Unparsed` section of the `http.Cookie` will ensure that the tests will fail once the `Partitioned` flag is supported by the Golang standard library.
To allow concurrent reading from the request and writing the response. * Add Unwrap to ResponseWriter to satisfy http.ResponseController support (Go 1.20+) https://pkg.go.dev/net/http#NewResponseController https://www.pivotaltracker.com/story/show/182988664
By default Go HTTP server consumes any unread request portion before writing the response for HTTP/1. This prevents handlers from reading request and writing response concurrently. This is set to false by default since it might be an unexpected behavior and cause deadlock for some handlers. See golang/go#15527 (comment)
… headers" This reverts commit 31a2bec.
1 task
cf-foundation-community-automation
bot
moved this to Inbox
in Application Runtime Platform Working Group
ameowlia
previously approved these changes
Apr 17, 2025
github-project-automation
bot
moved this from Inbox
to Pending Merge | Prioritized
in Application Runtime Platform Working Group
maxmoehl
force-pushed
the
maxmoehl/inline-submodules-3
branch
from
56cad1a to
27e43e7
Compare
maxmoehl
force-pushed
the
maxmoehl/inline-submodules-3
branch
from
27e43e7 to
b68fa10
Compare
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.
Note
If you are looking at this because the CLA bot mentioned you, feel free to ignore it and press the "Unsubscribe" button on the right to silence notifications for this PR.
Summary
See: #459
gorouter
route-registrar
There are still some smaller pull-requests open, either they get merged before this PR is and I bump one more time or the PRs are migrated via this command (in a post-merge routing-release):
This will create a merge commit on your branch from which you can then open a PR to routing-release. If you do so, please also reference the original PR on the gorouter repo.
Backward Compatibility
Breaking Change? No