set cluster/route/listener/vhost name len based on CLI opt by rshriram · Pull Request #1871 · envoyproxy/envoy

rshriram · 2017-10-16T21:54:16Z

Automatically set the max allowed length for cluster, route_config_name and listener name based on the max_stat_name_len specified. The name len is max_stat_name_len - 67 (which is the suffix reserved for internally generated stats).

Signed-off-by: Shriram Rajagopalan shriram@us.ibm.com

cc @hennna / @ggreenway FYI.

@mattklein123 this is per discussion on Slack.

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 · 2017-10-16T21:56:27Z

Quick pre-review comments: Will need doc changes. Also, we should do this for RDS and LDS also.

ggreenway · 2017-10-16T22:33:57Z

include/envoy/server/options.h

+  /**
+   * @return uint64_t the maximum name length of a cluster/route/listener.
+   */
+  virtual uint64_t maxUserSuppliedNameLength() PURE;


I think that it would be better to have this value be derived from the maxStatNameLen, or possibly have that one be derived from this one. Being able to specify both seems confusing.

Yup exactly. No more options are needed. Please rework as @ggreenway suggests.

Yeah good point. Does it suffice to just derive max cluster name length as max_stat_name_len - reserved_stat_suffix_len (which happens to be 67 at the moment) ? That would make things considerably simple.

Yup that's exactly what I would do.

htuch · 2017-10-16T22:35:18Z

@hennna please review this from the Google side (I will do the final pass from us).

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

ggreenway · 2017-10-17T22:19:46Z

test/server/hot_restart_impl_test.cc

  for (uint64_t i = 0; i < num_stats_; i++) {
    std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzz", i)
-                           .substr(0, Stats::RawStatData::maxNameLength());
+                           .substr(0, Stats::RawStatData::maxObjNameLength());


I think this should stay maxNameLength(), because it's directly creating the stat name

ggreenway · 2017-10-17T22:20:02Z

test/server/hot_restart_impl_test.cc

    // If this isn't true then the hard coded part of the name isn't long enough to make the test
    // valid.
-    EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxNameLength());
+    EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxObjNameLength());


Same here: maxNameLength()

ggreenway · 2017-10-17T22:22:47Z

test/server/hot_restart_impl_test.cc

-                                         options_.maxStats(), options_.maxStatNameLength()));
+  EXPECT_EQ(hot_restart_->version(),
+            Envoy::Server::SharedMemory::version(options_.maxStats(),
+                                                 options_.maxObjNameLength() +


I think this is correct, but as per @mattklein123's request, can you make sure the version string is the same before and after your change (as a manual check)?

with the new build its 9.200.16384.127 and with master build, its 9.200.16384.127 . so it matches!

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 · 2017-10-17T23:59:17Z

bazel/README.md


-The default maximum number of stats in shared memory, and the default maximum length of a stat name,
-can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH`,
+The default maximum number of stats in shared memory, and the default maximum length of a cluster/


nit: reflow text.

mattklein123 · 2017-10-18T00:00:09Z

source/common/config/json_utility.h

+// Set a string field in a protobuf message with the corresponding string value
+#define JSON_UTIL_SET_STRING_VALUE(json, message, field_name, value)                               \
+  do {                                                                                             \
+    (message).set_##field_name(value);                                                             \


What does this have to do with JSON? Why is this macro needed at all?

mattklein123 · 2017-10-18T00:01:00Z

source/server/options_impl.cc

-  TCLAP::ValueArg<uint64_t> max_stat_name_len("", "max-stat-name-len",
-                                              "Maximum name length for a stat", false,
-                                              ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH, "uint64_t", cmd);
+  TCLAP::ValueArg<uint64_t> max_obj_name_len("", "max-obj-name-len",


needs doc update

ack. Sorry missed that totally.

mattklein123 · 2017-10-18T00:01:43Z

test/common/upstream/cluster_manager_impl_test.cc

-                            "JSON at lines 4-6 does not conform to schema.\n Invalid schema: "
-                            "#/properties/name\n Schema violation: maxLength\n Offending "
-                            "document key: #/name");
+  EXPECT_THROW(create(parseBootstrapFromJson(json)), EnvoyException);


Why does this throw now? What's the new error?

This was always throwing an error. Earlier, it threw a JSON schema error with some message like "Invalid #/properties/name...." .. I merely changed this to use EnvoyException, as we are no longer validating the length using JSON schema.

In other words, UX wise, you would still get an exception when max length is exceeded, except it will be worded differently.

Right, please keep EXPECT_THROW_WITH_MESSAGE and switch it to the new error.

hennna

Thanks! Looking forward to using this feature.

hennna · 2017-10-17T22:30:56Z

source/common/stats/stats_impl.h

 private:
  static const size_t DEFAULT_MAX_NAME_SIZE = 127;
+  // total length must be equal to DEFAULT_MAX_NAME_SIZE
+  static const size_t MAX_STAT_SUFFIX_LENGTH = 67;


Maybe document where this number comes from? It seems from #667 it was from a cluster stats name plus some buffer.

hennna · 2017-10-18T02:59:56Z

source/common/stats/stats_impl.h

+  /**
+   * Returns the maximum length of a user supplied object (route/cluster/listener)
+   * name field in a stat. This length does not include a trailing NULL-terminator.
+   */


Should we instead have maxNameLength be derived from maxObjNameLength?

hennna · 2017-10-18T03:01:59Z

source/common/stats/stats_impl.h

@@ -103,6 +115,8 @@ struct RawStatData {

 private:
  static const size_t DEFAULT_MAX_NAME_SIZE = 127;


Should this be DEFAULT_MAX_OBJ_NAME_SIZE? Wondering if it would be better to have all max name size parameters be derived.

hennna · 2017-10-18T03:06:16Z

source/server/options_impl.cc

-#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < 127
-#error "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= 127"
+#if ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH < 60
+#error "ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH must be >= 60"


Outside the scope of this PR... Will there come a time when we also want to have MAX_STAT_SUFFIX_LENGTH be configurable? Or is that better left hard coded and bumped up when necessary?

rshriram · 2017-10-18T12:54:54Z

On Tue, Oct 17, 2017 at 11:08 PM hennna ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks! Looking forward to using this feature. ------------------------------ In source/common/stats/stats_impl.h <#1871 (comment)>: > @@ -103,6 +115,8 @@ struct RawStatData { private: static const size_t DEFAULT_MAX_NAME_SIZE = 127; + // total length must be equal to DEFAULT_MAX_NAME_SIZE + static const size_t MAX_STAT_SUFFIX_LENGTH = 67; Maybe document where this number comes from? It seems from #667 <#667> it was from a cluster stats name plus some buffer. ------------------------------ In source/common/stats/stats_impl.h <#1871 (comment)>: > @@ -68,6 +68,18 @@ struct RawStatData { } /** + * Returns the maximum length of a user supplied object (route/cluster/listener) + * name field in a stat. This length does not include a trailing NULL-terminator. + */ + static size_t maxObjNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } Should we instead have maxNameLength be derived from maxObjNameLength?

We could. I was trying to avoid touching the functions that were playing tricks with static vars, as the maxNameLen function is used everywhere! But let me see if it’s easy to change.

------------------------------ In source/common/stats/stats_impl.h <#1871 (comment)>: > @@ -103,6 +115,8 @@ struct RawStatData { private: static const size_t DEFAULT_MAX_NAME_SIZE = 127; Should this be DEFAULT_MAX_OBJ_NAME_SIZE? Wondering if it would be better to have all max name size parameters be derived.

This is max length of a stat entry. I agree it’s worded in a bit confusing manner.

------------------------------ In source/server/options_impl.cc <#1871 (comment)>: > #endif -#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < 127 -#error "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= 127" +#if ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH < 60 +#error "ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH must be >= 60" Outside the scope of this PR... Will there come a time when we also want to have MAX_STAT_SUFFIX_LENGTH be configurable? Or is that better left hard coded and bumped up when necessary?

@mattklein123 says we can bump it when necessary. I am surprised that 67 characters are enough to hold our current stat names.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1871 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qdwZDap0dY9z28VKZ1rTKU3HTS0Qlks5stWuzgaJpZM4P7TBJ> .

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram · 2017-10-18T14:25:24Z

@mattklein123 / @junr03 I noticed this in the docs:

*vhost.<virtual host name>.vcluster.<virtual cluster name>.* namespace and include the following
statistics:

Given that this PR allows for configurable name lengths for clusters, how does it affect the above stat, which is concatenating two different user supplied identities?

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 · 2017-10-18T15:53:20Z

Given that this PR allows for configurable name lengths for clusters, how does it affect the above stat, which is concatenating two different user supplied identities?

This occurred to me also. I think there are probably a bunch of random places where we should be guarding against user supplied stat names, etc. that might be too large. I would probably just open up a follow up ticket to audit.

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram · 2017-10-18T16:45:49Z

@mattklein123 I moved all the objNameLength checks into a separate file. Also added name length checks for virtual host (but not the virtual host + virtual cluster).
@hennna addressed your comments as well. PTAL

mattklein123

LGTM. few small things. @ggreenway can you take another look also?

mattklein123 · 2017-10-18T16:55:45Z

docs/operations/cli.rst

  information. Defaults to 900 seconds (15 minutes).

-.. option:: --max-stat-name-len <uint64_t>
+.. _operations_cli_max_obj_name_len:


doc nit: You don't need this anchor, you can reference options directly and then they look nice inline. See how we ref other options.

mattklein123 · 2017-10-18T16:55:59Z

bazel/README.md

-can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH`,
-respectively, to the desired value.  For example:
+The default maximum number of stats in shared memory, and the default
+maximum length of a cluster/ route config/listener name, can be


nit: extra space in "cluster/ "

mattklein123 · 2017-10-18T17:01:03Z

test/common/upstream/cluster_manager_impl_test.cc

-  }
-  )EOF";
-
-  EXPECT_THROW_WITH_MESSAGE(create(parseBootstrapFromJson(json)), Json::Exception,


Isn't this test still valid? Or is it not anymore?

Same comment as above. Test just moved to utility_test.cc ..

Yup sounds good

mattklein123 · 2017-10-18T17:01:21Z

test/common/router/rds_impl_test.cc

+  }
+  )EOF";
+
+  EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json),


Can we do EXPECT_THROW_WITH_MESSAGE here?

Actually, can I get rid of it? We have the same test in utility_test.cc

Yup that's fine

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

ggreenway · 2017-10-18T19:35:37Z

source/server/options_impl.cc

  if (hot_restart_version_option.getValue()) {
-    std::cerr << hot_restart_version_cb(max_stats.getValue(), max_stat_name_len.getValue());
+    std::cerr << hot_restart_version_cb(max_stats.getValue(),
+                                        max_obj_name_len.getValue() +


It would be nicer if we could call RawStatData::configure(), then use RawStatData::maxNameLength() here, instead of doing the addition. But I think it's probably more effort than it's worth, because the options_impl doesn't have it's values yet at this point in the function, so probably fine to leave as is, and the tests should catch if this gets messed up somehow.

mattklein123

LGTM. @htuch or @hennna any further comments?

hennna · 2017-10-19T02:25:42Z

LGTM!

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

#1871) **Description** This bumps envoy gateway to v1.7.0 and reverts the `host.docker.internal` OTLP workaround in `.env.otel.otel-tui` and `.env.otel.phoenix`. Verified all four docker compose test profiles (chat-completion, completion, create-embeddings, mcp) work with otel-tui receiving traces over the restored container service name endpoints. **Related Issues/PRs (if applicable)** **Special notes for reviewers (if applicable)** The `.env.otel.host` file intentionally uses `host.docker.internal` (not a workaround), so it is unchanged. Signed-off-by: Adrian Cole <adrian@tetrate.io>

Shriram Rajagopalan added 21 commits October 2, 2017 17:07

Allow non ws requests on WS route

d179e90

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

More tests for WebSockets

6e84e55

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Fix test for Non WS on WS route

2cedf44

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

84f2aab

fix

37af80d

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

connect timeout test

1645eaf

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

format

928272d

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

fix timeout test

6ee6657

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

d7a4188

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

formatting

8599956

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

d2a10c2

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

fix auto host rewrite check

75ef074

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

compile fix

baf1ee1

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

connectionfailure test

dd4b8aa

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

update docs

c9a9fa4

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

dce2ce1

Merge remote-tracking branch 'upstream/master'

cd70037

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

configurable cluster name length

659e793

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

formatting

b51a666

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

9d0366e

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

a724473

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram changed the title ~~Configurable cluster name length~~ Configurable cluster name/route name/listener name length Oct 16, 2017

rshriram changed the title ~~Configurable cluster name/route name/listener name length~~ Configurable cluster name name length Oct 16, 2017

ggreenway requested changes Oct 16, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 17, 2017 00:19

remove CLI option

c5d6e8f

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

644b05c

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram changed the title ~~Configurable cluster name name length~~ Configure cluster/route/listener name len based on max_stat_name_len Oct 17, 2017

rshriram changed the title ~~Configure cluster/route/listener name len based on max_stat_name_len~~ set cluster/route/listener name len based on max_stat_name_len Oct 17, 2017

Merge remote-tracking branch 'upstream/master'

2527ed1

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram changed the title ~~set cluster/route/listener name len based on max_stat_name_len~~ set cluster/route/listener name len based on CLI opt Oct 17, 2017

ggreenway reviewed Oct 17, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 17, 2017 18:44

long string to fix hot_restart_test

cae08f0

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

remove stray comment

5afd3ba

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 reviewed Oct 18, 2017

View reviewed changes

hennna reviewed Oct 18, 2017

View reviewed changes

docs and nits

d4de240

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

max len limit for vhosts

65debb2

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

review comments

fa2d91b

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram changed the title ~~set cluster/route/listener name len based on CLI opt~~ set cluster/route/listener/vhost name len based on CLI opt Oct 18, 2017

mattklein123 reviewed Oct 18, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 18, 2017 13:46

nits

9ec016c

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

e9b0dce

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master'

b534be9

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

ggreenway approved these changes Oct 18, 2017

View reviewed changes

mattklein123 approved these changes Oct 18, 2017

View reviewed changes

mattklein123 merged commit e366bcf into envoyproxy:master Oct 19, 2017

rshriram mentioned this pull request Oct 19, 2017

Update envoy sha to incorporate ability to override server span name … istio/proxy#594

Closed

htuch added a commit to htuch/envoy-api that referenced this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster.

13fc9f2

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch mentioned this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster. envoyproxy/data-plane-api#216

Merged

htuch added a commit to envoyproxy/data-plane-api that referenced this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster. (#216)

de2537c

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

Elite1015 pushed a commit to Elite1015/data-plane-api that referenced this pull request Feb 23, 2025

cds: update cluster name restrictions in Cluster. (#216)

715273a

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

		@@ -103,6 +115,8 @@ struct RawStatData {

		private:
		static const size_t DEFAULT_MAX_NAME_SIZE = 127;

Conversation

rshriram commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Oct 16, 2017

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 Oct 16, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hennna 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!

rshriram commented Oct 18, 2017 via email

Uh oh!

rshriram commented Oct 18, 2017

Uh oh!

mattklein123 commented Oct 18, 2017

Uh oh!

rshriram commented Oct 18, 2017

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!

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

rshriram commented Oct 16, 2017 •

edited

Loading