Bazel: add define flag for exporting symbols by taion809 · Pull Request #2331 · envoyproxy/envoy

taion809 · 2018-01-09T17:06:20Z

Bazel: add define flag for exporting symbols

Description:
The current problem with lua is that it is unable to use external
libraries such as lrexlib installed via luarocks.

The solution is to export the symbols needed by those libraries.

Risk Level: Low

Testing:
I tested this manually. Here is an example lua script and envoy configuration.

{
    "listeners": [
      {
        "address": "tcp://0.0.0.0:5555",
        "filters": [
          {
            "name": "http_connection_manager", 
            "config": {
              "codec_type": "auto", 
              "add_user_agent": true, 
              "use_remote_address": true,
              "idle_timeout_s": 840,
              "stat_prefix": "frontrouter",
              "route_config": {
                "virtual_hosts": [
                  {
                    "name": "backend",
                    "domains": ["*"],
                    "routes": [
                      {
                        "prefix": "",
                        "cluster": "echoer-backend",
                        "auto_host_rewrite": false
                      }
                    ]
                  }
                ]
              }, 
              "filters": [
                  {
                    "name": "lua",
                    "config": {
                        "inline_code": "dofile(\"script.lua\")"
                    }
                  },
                {
                  "name": "router", 
                  "config": {}
                }
              ]
            }
          }
        ]
      }
    ], 
    "cluster_manager": {
      "clusters": [
        {
          "name": "echoer-backend",
          "connect_timeout_ms": 250,
          "type": "static",
          "lb_type": "round_robin",
          "hosts": [
            {
              "url": "tcp://127.0.0.1:3000"
            }
          ]
        }
      ]
    }
  }

function envoy_on_request(request)
    local rex = require "rex_pcre"
    headers = request:headers()
    referrerHeader = headers:get("Referrer")

    if referrerHeader ~= nil then
        match = rex.match(referrerHeader, '^\\/search\\?.*category=.*')
        request:logInfo("Match: "..match)
    end
end

function envoy_on_response(response)
end

Docs Changes:
envoyproxy/data-plane-api#405

Fixes issue #2251

Signed-off-by: Nicholas J nicholas.a.johns5@gmail.com

htuch

Thanks for the contribution.

htuch · 2018-01-09T17:27:45Z

bazel/envoy_build_system.bzl

            # See note here: http://luajit.org/install.html
            "-pagezero_size 10000", "-image_base 100000000",
        ],
+        "//bazel:enable_exported_symbols": [


I think this could be a separate select statement which is + with the existing one. You can actually factor out a bunch of common list entries into a separate item that is + with this as well.

htuch · 2018-01-09T17:28:18Z

bazel/envoy_build_system.bzl

            "-pagezero_size 10000", "-image_base 100000000",
        ],
+        "//bazel:enable_exported_symbols": [
+            '-L/usr/local/lib/lua/5.1/',


This won't work in many environments. Is there a more generic way to handle this?

oops, Let me relook into this I was doing a lot of troubleshooting before and this just slipped by me :|

htuch · 2018-01-09T17:29:15Z

bazel/envoy_build_system.bzl

+            # --build-id and avoid doing the following.
+            '-Wl,--build-id=md5',
+            '-Wl,--hash-style=gnu',
+            '-Wl,-E',


Can you add a comment above this line also explaining what it does and why needed?

mattklein123

Thank you for doing this! Few comments. @jmillikin-stripe or @htuch do you mind taking a look also?

mattklein123 · 2018-01-09T17:30:34Z

bazel/envoy_build_system.bzl

+            "-static-libgcc",
+        ],
        "//conditions:default": [
+            '-L/usr/local/lib/lua/5.1/',


Is this needed in the default static linking case?

mattklein123 · 2018-01-09T17:30:59Z

bazel/envoy_build_system.bzl

            "-pagezero_size 10000", "-image_base 100000000",
        ],
+        "//bazel:enable_exported_symbols": [
+            '-L/usr/local/lib/lua/5.1/',


Is this path going to be consistent across machines? Does it also need to be configurable?

mattklein123 · 2018-01-09T17:31:52Z

bazel/envoy_build_system.bzl

+            # --build-id and avoid doing the following.
+            '-Wl,--build-id=md5',
+            '-Wl,--hash-style=gnu',
+            '-Wl,-E',


Instead of replicating the entire block just to add -E could we possibly have this be the output of a function that keys off of whether this feature is enabled or not? Then we can just call the function below?

mattklein123 · 2018-01-09T17:32:21Z

bazel/README.md

+
+The following optional features can be enabled on the Bazel build command-line:
+
+* Exported symbols during linking with `--define exported_symbols=enabled`


nit: Could we add a brief description of why someone would want to do this?

htuch

LGTM modulo minor comments.

htuch · 2018-01-10T19:05:32Z

bazel/envoy_build_system.bzl

This comment isn't very descriptive :) Probably best to say what exported symbols are..

embarrassing, i did some work on this and pushed it up before i left the office 😱

htuch · 2018-01-10T19:06:03Z

bazel/envoy_build_system.bzl

More Pythonic to write if not linkopts: here.

htuch · 2018-01-10T19:06:40Z

RAW_RELEASE_NOTES.md

Nit: s/envoy/Envoy/

htuch · 2018-01-10T19:07:37Z

Also, needs DCO fixup (https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco).

jmillikin-stripe · 2018-01-10T22:02:46Z

bazel/envoy_build_system.bzl

I believe you can use a hardcoded @envoy for the repository -- the only place where that wouldn't work (the Google internal fork) rewrites this file entirely (per htuch).

I'll give that a shot thanks

htuch

Great, thanks. Still needs DCO fixup, see https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco.

taion809 · 2018-01-16T20:06:41Z

Just an update to this, while testing i ran into an error so i'll be doing some more work on this before its ready to go.

htuch · 2018-01-17T17:25:07Z

@taion809 this looks good to merge. Are you suggesting we hold back or GTG?

taion809 · 2018-01-17T18:06:20Z

@htuch good to go, i did some more manual tests to make sure it was working as expected 👍

htuch · 2018-01-17T20:06:59Z

@taion809 looks like it needs to be merged again with master.

The current problem with lua is that it is unable to use external libraries such as lrexlib installed via luarocks. The solution is to export the symbols needed by those libraries. Signed-off-by: Nicholas J <nicholas.a.johns5@gmail.com>

* initial commit * Add common as a dep * add common context * WIP3 * WIP5 * WIP7 * WIP3 * unblock client config * write mapper * add mapper * added all 4 http metrics * fix fast cache key * Temporarily disable test * enable common test * review comments * add explicit initialization to mappings * more review comments * more comments2 * make cache return reference instead of a pointer * remove copy * WIP direct mapping * more direct code * more simplification * refactor common, and StatsGen * add httbin backend for testing, add better logging * fix test namespace * prepare to add custom hash * add trivial local httpbin impl for testing * which to absl:hashValue * add missing test file * fix nits * add license header * fix function name * set max cache size * make istio_dimensions_ a member of rootContext. The mappings are overwritten before evaluation * elimiate Node abstraction from NodeInfoCache * try V4 only testing for prow * Fix getMetadataStruct return values

Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>

taion809 force-pushed the bugfix-lua-linking branch from 6e667f4 to b4dca3e Compare January 9, 2018 17:08

htuch reviewed Jan 9, 2018

View reviewed changes

mattklein123 reviewed Jan 9, 2018

View reviewed changes

htuch reviewed Jan 10, 2018

View reviewed changes

jmillikin-stripe reviewed Jan 10, 2018

View reviewed changes

htuch previously approved these changes Jan 12, 2018

View reviewed changes

mattklein123 assigned htuch Jan 12, 2018

taion809 dismissed htuch’s stale review via 8fb4876 January 17, 2018 16:03

taion809 force-pushed the bugfix-lua-linking branch 2 times, most recently from 8fb4876 to c1778c6 Compare January 17, 2018 16:05

Bazel: add define flag for exporting symbols

cb177f6

The current problem with lua is that it is unable to use external libraries such as lrexlib installed via luarocks. The solution is to export the symbols needed by those libraries. Signed-off-by: Nicholas J <nicholas.a.johns5@gmail.com>

taion809 force-pushed the bugfix-lua-linking branch from c1778c6 to cb177f6 Compare January 17, 2018 21:07

mattklein123 approved these changes Jan 17, 2018

View reviewed changes

mattklein123 merged commit 61b3852 into envoyproxy:master Jan 17, 2018

taion809 deleted the bugfix-lua-linking branch January 18, 2018 14:11

jpsim added a commit that referenced this pull request Nov 28, 2022

Bump Lyft Support Rotation (#2331)

0d1fcdd

Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim added a commit that referenced this pull request Nov 29, 2022

Bump Lyft Support Rotation (#2331)

d58eea5

Signed-off-by: GitHub Action <noreply@github.com> Co-authored-by: jpsim <jpsim@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>


		The following optional features can be enabled on the Bazel build command-line:

		* Exported symbols during linking with `--define exported_symbols=enabled`

Conversation

taion809 commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

taion809 commented Jan 16, 2018

Uh oh!

htuch commented Jan 17, 2018

Uh oh!

taion809 commented Jan 17, 2018

Uh oh!

htuch commented Jan 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taion809 commented Jan 9, 2018 •

edited

Loading