Skip to content

Allow local modification of envoy.bazelrc#6854

Open
amdonov wants to merge 2 commits intoistio:masterfrom
amdonov:three-way-merge
Open

Allow local modification of envoy.bazelrc#6854
amdonov wants to merge 2 commits intoistio:masterfrom
amdonov:three-way-merge

Conversation

@amdonov
Copy link
Copy Markdown
Contributor

@amdonov amdonov commented Feb 26, 2026

Description

This PR updates the update_envoy.sh script to preserve local modifications to envoy.bazelrc while still applying upstream changes from Envoy.

This addresses the issue where PR #6840 was automatically overwritten by the update_envoy.sh automation, which previously performed a direct file replacement.

Changes

The script now performs a three-way merge of envoy.bazelrc:

  • Base: Previous upstream version (from old ENVOY_SHA)
  • Local: Current envoy.bazelrc with Istio-specific modifications
  • Upstream: New upstream version (from new ENVOY_SHA)

This allows:

  • ✅ Local modifications (like the @envoy// prefix fixes from Fix FIPS build flags to reference @envoy repository #6840) to be preserved across updates
  • ✅ Upstream changes to be automatically applied
  • ✅ Conflicts to be auto-resolved by taking the upstream version (preserving existing behavior)

Behavior

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

@amdonov amdonov requested a review from a team as a code owner February 26, 2026 18:08
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Feb 26, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Feb 26, 2026
@istio-testing
Copy link
Copy Markdown
Collaborator

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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>
@keithmattix
Copy link
Copy Markdown
Contributor

Why not create a user.bazelrc? Envoy uses that pattern and user.bazelrc changes should take precedence

@amdonov
Copy link
Copy Markdown
Contributor Author

amdonov commented Feb 27, 2026

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.

@krinkinmu
Copy link
Copy Markdown
Contributor

Why not create a user.bazelrc? Envoy uses that pattern and user.bazelrc changes should take precedence

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.

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.

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 common:fips-common, common:boringssl-fips and common:aws-lc-fips configs directly from Envoy, we can add to .bazelrc istio own versions of those:

common:istio-fips-common ...
...

common:istio-boringssl-fips --config=fips-common
...

common:istio-aws-lc-fips --config=fips-common
...

Then you can build fips versions by providing a slightly differently named config --config=istio-boringssl-fips.

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.

@amdonov
Copy link
Copy Markdown
Contributor Author

amdonov commented Feb 27, 2026

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.

@krinkinmu
Copy link
Copy Markdown
Contributor

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.

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.

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.

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.

@amdonov
Copy link
Copy Markdown
Contributor Author

amdonov commented Feb 27, 2026

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.

@krinkinmu
Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants