Allow local modification of envoy.bazelrc#6854
Conversation
|
Hi @amdonov. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: Aaron Donovan <amdonov@amazon.com>
|
Why not create a user.bazelrc? Envoy uses that pattern and user.bazelrc changes should take precedence |
|
Certainly open to other approaches. I'm just trying to reduce work for users and allow them to use envoy's configuration options directly. For my particular case, I can run a build with --@envoy//bazel:fips=True --@envoy//bazel:ssl=@boringssl_fips//:ssl --@envoy//bazel:crypto=@boringssl_fips//:crypto, but using --config=boringssl-fips is a lot easier. Specifying that config option doesn't work currently. |
I don't think it will work in a straighforward way. Basically all the configs from envoy.bazelrc and user.bazelrc are combined together - we cannot trully override anything via user.bazelrc. It works for some options, but not for others.
Maybe one way to try here is rather than doing a merge of envoy.bazelrc with local changes we can just fork the crypto library settings? E.g., instead of using Then you can build fips versions by providing a slightly differently named config I don't feel terribly good about this approach either, but it has a minor benefit that we don't need to allow local changes to envoy.bazelrc, so for everything except crypto libraries we don't diverge. It does require us to keep our own versions of those configs up-to-date still. |
|
Using the merge approach gives you options. I don't imagine the need to customize envoy.bazelrc will come up often, but it's nice to know that you can if you want. In this case, it also allows you to keep the same config names which I find cleaner. I also think it's pretty low risk. Worst case you get new/updated values from the envoy project. |
It might make things easier in the short run when you just want to fix something quickly, but it creates burden of maintaining those changes over time - we can speculate that this burden is insignificant, but ultimately we don't know. So while it might be nice in some cases to just change envoy.bazelrc, strategically it's not optimal.
I have nothing against keeping the same config names for crypto library configs, but that's not the only thing this PR does - it also allows making arbirarty other changes to the envoy.bazelrc. I don't have a terribly strong opinion here, but I don't think that allowing arbitrary divergence is a good idea. |
|
I don't view it as arbitrary divergence. It provides an opportunity to fix settings that are incorrect in this project's context. Changes would be deliberate and subject to review. |
How we review the divergence is a different question. You're saying that if we open that door, we can manage things with a careful and deliberate review of changes - and that might be the case, but what I'm saying is that we should not open that door in the first place. |
Description
This PR updates the
update_envoy.shscript to preserve local modifications toenvoy.bazelrcwhile still applying upstream changes from Envoy.This addresses the issue where PR #6840 was automatically overwritten by the
update_envoy.shautomation, which previously performed a direct file replacement.Changes
The script now performs a three-way merge of
envoy.bazelrc:ENVOY_SHA)envoy.bazelrcwith Istio-specific modificationsENVOY_SHA)This allows:
@envoy//prefix fixes from Fix FIPS build flags to reference @envoy repository #6840) to be preserved across updatesBehavior
When upstream doesn't modify a line: Local changes are preserved
When upstream modifies the same line: Upstream version wins (same as before)
This maintains backward compatibility while enabling selective local customization of bazel settings.
Fixes the automation issue that overwrote #6840