Skip to content

bug fix: return bootstrap when validating config#17499

Merged
mattklein123 merged 7 commits intoenvoyproxy:mainfrom
qinggniq:valid_panic
Aug 2, 2021
Merged

bug fix: return bootstrap when validating config#17499
mattklein123 merged 7 commits intoenvoyproxy:mainfrom
qinggniq:valid_panic

Conversation

@qinggniq
Copy link
Contributor

@qinggniq qinggniq commented Jul 27, 2021

Signed-off-by: qinggniq livewithblank@gmail.com

fix #17344

Commit Message: fix unimplemented panic when validating config, ref #17344
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: qinggniq <livewithblank@gmail.com>
@qinggniq qinggniq changed the title bug fix: return bootstrap when valid config bug fix: return bootstrap when validating config Jul 27, 2021
@asraa asraa self-assigned this Jul 27, 2021
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Do you mind adding a regression test with a config containing ext_authz ?

Signed-off-by: qinggniq <livewithblank@gmail.com>
Signed-off-by: qinggniq <livewithblank@gmail.com>
@qinggniq qinggniq requested a review from asraa July 29, 2021 12:17
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! But if the regression test is never run, then it's not quite a test.

Maybe you could add it to ext_authz_integration_test?

TEST_P(ExtAuthzLocalReplyIntegrationTest, DeniedHeaderTest) {
config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
auto* ext_authz_cluster = bootstrap.mutable_static_resources()->add_clusters();
ext_authz_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]);
ext_authz_cluster->set_name("ext_authz");
envoy::extensions::filters::http::ext_authz::v3::ExtAuthz proto_config;
const std::string ext_authz_config = R"EOF(
transport_api_version: V3
http_service:
server_uri:
uri: "ext_authz:9000"
cluster: "ext_authz"
timeout: 300s
)EOF";
TestUtility::loadFromYaml(ext_authz_config, proto_config);
envoy::config::listener::v3::Filter ext_authz_filter;
ext_authz_filter.set_name("envoy.filters.http.ext_authz");
ext_authz_filter.mutable_typed_config()->PackFrom(proto_config);
config_helper_.addFilter(MessageUtil::getJsonStringFromMessageOrDie(ext_authz_filter));
});
const std::string local_reply_yaml = R"EOF(
body_format:
json_format:
code: "%RESPONSE_CODE%"
message: "%LOCAL_REPLY_BODY%"
)EOF";
envoy::extensions::filters::network::http_connection_manager::v3::LocalReplyConfig
local_reply_config;
TestUtility::loadFromYaml(local_reply_yaml, local_reply_config);
config_helper_.setLocalReply(local_reply_config);
HttpIntegrationTest::initialize();

(Since extensions shouldn't be loaded into test/server:server_test. Please check that it failed before the fix and is fixed after.

@qinggniq
Copy link
Contributor Author

qinggniq commented Jul 29, 2021

@asraa Sorry for not explain this clearly. I think this PR is very similar with #15932. This PR fixed that an unimplemented method in ValidateInstance at source/server/config_validation/server.h causing crash when validating config which contain ext_authz. So I added envoy-ext_authz.yaml to let //test/server/config_validation:server_test to validate this config.

@qinggniq
Copy link
Contributor Author

But the new added config seems not really be validated, I'll take a further look.

qinggniq added 3 commits July 30, 2021 11:14
Signed-off-by: qinggniq <livewithblank@gmail.com>
Signed-off-by: qinggniq <livewithblank@gmail.com>
Signed-off-by: qinggniq <livewithblank@gmail.com>
@qinggniq qinggniq requested a review from asraa July 30, 2021 13:50
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing the test and adding the comment to the issue link! LGTM

@asraa
Copy link
Contributor

asraa commented Aug 2, 2021

@envoyproxy/senior-maintainers just a very quick pass for a final merge. LGTM

@mattklein123 mattklein123 merged commit 0bd670a into envoyproxy:main Aug 2, 2021
@travisgroth
Copy link

/backport

Nominating for backport as a stability fix. I believe it only needs to land on 1.19.

@repokitteh-read-only repokitteh-read-only bot added the backport/review Request to backport to stable releases label Aug 3, 2021
baojr added a commit to baojr/envoy that referenced this pull request Aug 4, 2021
…bridge-stream

* upstream/main: (32 commits)
  tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334)
  conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577)
  api: LEDS api introduction (envoyproxy#17419)
  kafka: add support for api versions request in mesh-filter (envoyproxy#17475)
  ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531)
  tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505)
  xds: restructure CertificateProvider fields (envoyproxy#17201)
  Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530)
  listener: move active connection collection out of active tcp listener (envoyproxy#16947)
  tools: format checks for backticks (envoyproxy#17566)
  coverage: set lower limit for common/quic and common (envoyproxy#17573)
  v2: final source removal (envoyproxy#17565)
  test: bumping coverage (envoyproxy#17564)
  quic: enforcing header size and contents (envoyproxy#17520)
  Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137)
  listener: add a stat for transport socket connect timeout (envoyproxy#17458)
  listener: add listen() error handling (envoyproxy#17427)
  http: return per route config when direct response is set (envoyproxy#17449)
  removing most v2 references from source/ (envoyproxy#17415)
  bug fix: return bootstrap when validating config (envoyproxy#17499)
  ...

Signed-off-by: Garrett Bourg <bourg@squareup.com>
@ggreenway ggreenway added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Aug 9, 2021
ggreenway pushed a commit to ggreenway/envoy that referenced this pull request Aug 9, 2021
(cherry picked from commit 0bd670a, PR envoyproxy#17499)
Co-authored-by: qinggniq <livewithblank@gmail.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
@qinggniq qinggniq deleted the valid_panic branch August 10, 2021 09:20
ggreenway added a commit that referenced this pull request Aug 12, 2021
(cherry picked from commit 0bd670a, PR #17499)
Co-authored-by: qinggniq <livewithblank@gmail.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: qinggniq <livewithblank@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/approved Approved backports to stable releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

envoy validate Panics /source/server/config_validation/server.h

5 participants