Skip to content

Envoy 1.29#312

Merged
soloio-bulldozer[bot] merged 40 commits intomainfrom
jbohanon/envoy-1.29
Mar 12, 2024
Merged

Envoy 1.29#312
soloio-bulldozer[bot] merged 40 commits intomainfrom
jbohanon/envoy-1.29

Conversation

@jbohanon
Copy link
Copy Markdown
Contributor

@jbohanon jbohanon commented Mar 1, 2024

No description provided.

@jbohanon
Copy link
Copy Markdown
Contributor Author

jbohanon commented Mar 1, 2024

WIP label not to be removed until:

  • figure out how to get region for AWS creds without STS config
  • we aren't patching envoy for env vars into do_ci.sh -- we need to set BUILD_DIR because if unset, everything else is overridden in build_setup.sh
  • figure out why we are getting libc++abi: Pure virtual function called! on the ServerFactoryContext::clusterManager() See potential invalid memory access to the ServerFactoryContext envoyproxy/envoy#26653 for details; we are accessing during server initialization it seems, where clusterManager() is not yet available on the interface (what a weird side effect of the inheritance model here)

@jbohanon
Copy link
Copy Markdown
Contributor Author

jbohanon commented Mar 4, 2024

/kick killed

@nfuden
Copy link
Copy Markdown

nfuden commented Mar 5, 2024

@jbohanon
Copy link
Copy Markdown
Contributor Author

jbohanon commented Mar 6, 2024

/kick 💀

@jbohanon
Copy link
Copy Markdown
Contributor Author

jbohanon commented Mar 7, 2024

/kick

@jbohanon jbohanon marked this pull request as draft March 12, 2024 18:31
@jbohanon jbohanon marked this pull request as ready for review March 12, 2024 18:55
rm -rf "$ENVOY_DOCKER_BUILD_DIR/envoy/x64/bin/"
else
rm -rf "/build/envoy/x64/bin/"
if [ -z "$BUILD_DIR" ]; then
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You had mentioned something about this in a comment:

we aren't patching envoy for env vars into do_ci.sh -- we need to set BUILD_DIR because if unset, everything else is overridden in build_setup.sh

Can you say more about this? I think I see the block you're talking about:

if [[ -z "${BUILD_DIR}" ]]; then
    echo "BUILD_DIR not set - defaulting to ~/.cache/envoy-bazel" >&2
    BUILD_DIR="${HOME}/.cache/envoy-bazel"
fi

But does it matter whether we build in the default location?

auto sts_factory = StsCredentialsProviderFactory::create(context.api(),
context.clusterManager());
server_context.api(), absl::nullopt /* ServerFactoryContextOptRef context */,
// We pass an empty string if we don't have a region
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we default to us-east-1?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

after some reflection I think it could be very frustrating to debug issues related to default values in this section -- I think I prefer leaving this as-is

Copy link
Copy Markdown
Contributor

@ben-taussig-solo ben-taussig-solo left a comment

Choose a reason for hiding this comment

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

All comments are nits -- this PR looks good to me

@soloio-bulldozer soloio-bulldozer bot merged commit ebdfc84 into main Mar 12, 2024
@soloio-bulldozer soloio-bulldozer bot deleted the jbohanon/envoy-1.29 branch March 12, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants