Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: env var protection for xds fallback #7483

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 6, 2024

Fixes #7479

#a71-xds-fallback

RELEASE NOTES: none

@easwars easwars requested a review from dfawley August 6, 2024 14:57
@easwars easwars added the Type: Feature New features or improvements in behavior label Aug 6, 2024
@easwars easwars added this to the 1.66 Release milestone Aug 6, 2024
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.54%. Comparing base (e524655) to head (53b0487).

Files Patch % Lines
internal/xds/bootstrap/bootstrap.go 66.66% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7483      +/-   ##
==========================================
+ Coverage   81.40%   81.54%   +0.14%     
==========================================
  Files         357      357              
  Lines       27243    27259      +16     
==========================================
+ Hits        22176    22229      +53     
+ Misses       3850     3826      -24     
+ Partials     1217     1204      -13     
Files Coverage Δ
internal/envconfig/envconfig.go 100.00% <ø> (ø)
internal/xds/bootstrap/bootstrap.go 68.05% <66.66%> (+0.37%) ⬆️

... and 23 files with indirect coverage changes

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +1205 to +1207
// TODO(easwars): Default value of "GRPC_EXPERIMENTAL_XDS_FALLBACK"
// env var is currently false. When the default is changed to true,
// explicitly set it to false here.
Copy link
Member

Choose a reason for hiding this comment

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

optional: explicitly state what the test expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole test is expected to be run with the env var set to false. This is stated in the name of the test and the top-level comment.

Copy link
Member

@arvindbr8 arvindbr8 Aug 7, 2024

Choose a reason for hiding this comment

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

i meant

	origFallbackEnv := envconfig.XDSFallbackSupport
	envconfig.XDSFallbackSupport = false
	defer func() { envconfig.XDSFallbackSupport = origFallbackEnv }()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. Should have done that. But now, I will just wait for the default value to change before doing that :)

@arvindbr8 arvindbr8 assigned easwars and unassigned dfawley Aug 7, 2024
@easwars easwars merged commit d00dd8f into grpc:master Aug 7, 2024
13 checks passed
@easwars easwars deleted the xds_fallback_env_var branch August 7, 2024 21:27
infovivek2020 pushed a commit to infovivek2020/grpc-go that referenced this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xdsclient: support environment variable protection for fallback
3 participants