style: automate and enforce header ordering guidelines. by htuch · Pull Request #793 · envoyproxy/envoy

htuch · 2017-04-19T21:47:32Z

Script to automate header ordering check/fix (tools/header_order.py) and resulting reorderings. See
the comment at the top of header_order.py for why the header order looks the way it does.

Also made check_format.py standalone and introduced a check_format CI target in addition to
fix_format in anticipation of removing the old CI targets.

There is considerable overlap with what this does and clang-format's IncludeCategories (see
https://clang.llvm.org/docs/ClangFormatStyleOptions.html). But, clang-format doesn't seem smart
enough to handle block splitting and correctly detecting the main header subject to the Envoy
canonical paths.

Script to automate header ordering check/fix (tools/header_order.py) and resulting reorderings. See the comment at the top of header_order.py for why the header order looks the way it does. Also made check_format.py standalone and introduced a check_format CI target in addition to fix_format in anticipation of removing the old CI targets. There is considerable overlap with what this does and clang-format's IncludeCategories (see https://clang.llvm.org/docs/ClangFormatStyleOptions.html). But, clang-format doesn't seem smart enough to handle block splitting and correctly detecting the main header subject to the Envoy canonical paths.

mattklein123

very nice, small nits.

mattklein123 · 2017-04-20T03:35:06Z

tools/header_order.py

+    with open(path, 'w') as f:
+      f.write(reorderd_source)
+    sys.exit(0)
+  print 'Usage: %s [--rewrite] <.h path>' % sys.argv[0]


nit: .h path is kind of confusing. It's just a source file path right?

mattklein123 · 2017-04-20T03:38:35Z

tools/header_order.py

+
+  after_includes_lines += list(all_lines)
+
+  def file_header_filter():


nit: it took me 60s to figure out that this is figuring out the associated files header, add comment?

Since Bazel doesn't support precompiled headers (bazelbuild/bazel#1215), we actually suffer a slowdown rather than speedup by having all files implicitly include precompiled.h in the Bazel build. I found ~10% performance improvement on my workstation, going from 181s to 163s for a "bazel build //test/..." from a "bazel clean" state. This patch makes all includes now explicit. This will slowdown the cmake build, but since this is not long for this world it seems alright. The header reordering in this patch was achieved using envoyproxy#793. The addition of the missing headers in each file was done using a modified version of the tool in envoyproxy#793, which substituted in headers based on simple substring matches (e.g. whenever it saw std::string in a file, it added #include <string>). Here's the patterns I ended up using that seemed to work: sys_extra_includes = [ ('std::runtime_error', 'stdexcept'), ('strcmp', 'string.h'), ('strcasecmp', 'strings.h'), ('std::unique_ptr', 'memory'), ('std::shared_ptr', 'memory'), ('std::unordered_set', 'unordered_set'), ('std::transform', 'algorithm'), ('std::vector', 'vector'), ('std::regex', 'regex'), ('spdlog::', 'spdlog/spdlog.h'), ('fmt::format', 'spdlog/spdlog.h'), ('std::string', 'string'), ('uint32_t', 'cstdint'), ('uint64_t', 'cstdint'), ('std::array', 'array'), ('sockaddr_un', 'sys/un.h'), ('sockaddr_in', 'netinet/ip.h'), ('std::chrono', 'chrono'), ('std::list', 'list'), ('std::multimap', 'map'), ('std::unordered_map', 'unordered_map'), ('std::stringstream', 'sstream'), ('std::condition_variable', 'condition_variable'), ('std::mutex', 'mutex'), (' ::close', 'unistd.h'), ('inet_ntop', 'arpa/inet.h'), ('inet_pton', 'arpa/inet.h'), ('htonl', 'arpa/inet.h'), ('setsockopt', 'sys/types.h'), ('setsockopt', 'sys/socket.h'), ('TCP_NODELAY', 'netinet/tcp.h'), ('SIGTERM', 'signal.h'), ('SIGPIPE', 'signal.h'), ('std::function', 'functional'), ('std::ifstream', 'fstream'), ('std::ofstream', 'fstream'), ('std::forward_list', 'forward_list'), ('std::atomic', 'atomic'), ('std::cerr', 'iostream'), ('hostent', 'netdb.h'), (' bind(', 'sys/types.h'), (' bind(', 'sys/socket.h'), ('std::ostream', 'iostream'), ] extra_includes = [ ('MOCK_METHOD', 'gmock/gmock.h'), ('MOCK_CONST_METHOD', 'gmock/gmock.h'), ('MATCHER_P', 'gmock/gmock.h'), ('ON_CALL', 'gmock/gmock.h'), ('using testing::_', 'gmock/gmock.h'), ('testing::Test', 'gtest/gtest.h'), ('EXPECT_', 'gtest/gtest.h'), ('\nTEST(', 'gtest/gtest.h'), ('\nTEST_F(', 'gtest/gtest.h'), ('\nTEST_P(', 'gtest/gtest.h'), ('HeaderMapImpl', 'test/test_common/printers.h'), ('Buffer::', 'test/test_common/printers.h'), ('RespValue', 'test/test_common/printers.h'), ]

**Description** This commit removes the handwritten header matching code from the extproc, and instead starts utilizing the hardened envoy native router. Historically, we had only one giant extproc filter where we did all logics including model name extraction, routing and then body transformation & upstream authorization. Since #599, we split into two external processor filters; one sits at the normal HTTP router and the other is configured at the per-cluster upstream HTTP filter. In theory, the one at HTTP router has only one job on request path: extracting model name from the request body. However, due to the historical reason, the handwritten router logic component remained, and that comes with not only a maintenance cost (forcing a complex extproc & control plane orchestration) but also a potential security vulnerability. In fact, writing header matching logic can be an easy attack surface, so if it's possible, we should avoid writing our own header matching (routing logic) but should rely on the battle-tested hardened envoy native router. With this commit, now a regex matching is available as well as there's no difference between HTTPRoute's matching and AIGatewayRoute's matching implementation. This also opens up a possibility to support path matching in our rule. **Related Issues/PRs (if applicable)** Ref #612 Ref #73 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

**Description** Before #793, the case where no matching route found was handled in the extproc and the 404 immediate response was returned from there, but after that, it naturally results in the "unreachable" default route and swallowed the indication of no matching and it made it impossible to reason about the 500 error on that case. In other words, this fixes the regression in #793 to return the proper 404 response. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

htuch added 2 commits April 19, 2017 17:45

Fix cmake targets by not having precompiled_test.h reordered.

ee0a4f1

htuch mentioned this pull request Apr 20, 2017

build: eliminate precompiled headers from bazel build. #799

Closed

mattklein123 reviewed Apr 20, 2017

View reviewed changes

mattklein123@ review feedback and some more comments.

00f3186

mattklein123 approved these changes Apr 20, 2017

View reviewed changes

mattklein123 merged commit 33a765b into envoyproxy:master Apr 20, 2017

htuch mentioned this pull request Jul 5, 2017

format: upgrade to clang-format-5.0 #1212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: automate and enforce header ordering guidelines.#793

style: automate and enforce header ordering guidelines.#793
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
htuch:header-cleanup

htuch commented Apr 19, 2017

Uh oh!

mattklein123 left a comment

Uh oh!

mattklein123 Apr 20, 2017

Uh oh!

mattklein123 Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		after_includes_lines += list(all_lines)

		def file_header_filter():

Conversation

htuch commented Apr 19, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

mattklein123 Apr 20, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants