Option to set "stale" consistency by default for http requests. #3142

wojtkiewicz · 2017-06-12T12:33:25Z

Hi,
This PR introduces option to set stale consistency by default for http requests when client didn't explicitly asked for any consistency in particular. Such behaviour is possible to accomplish using dns endpoint at the moment but not when client is asking using REST api.

This feature is especially useful when consul cluster is used by few hundred micro-services and without using stale all requests are handled by cluster leader which doesn't help to scale cluster horizontally. When dealing with lots of services asking every team to specify stale consistency explicitly is doable but very impractical.

This new feature has to be explicitly turned on. By default nothing is changed in how default consistency is handled.

Sample configuration:
{"http_api_stale_by_default": true}

preetapan · 2017-06-12T15:20:21Z

agent/http.go

 	query := req.URL.Query()
 	if _, ok := query["stale"]; ok {
 		b.AllowStale = true
 	}
 	if _, ok := query["consistent"]; ok {
 		b.RequireConsistent = true
 	}
+	if !b.AllowStale && !b.RequireConsistent {


Suggestion to rewrite to make this boolean logic more readable. You could remove this block and init b.allowStale before line 364 with a single line like

b.allowStale = !b.RequireConsistent && s.agent.config.HttpAPIStaleByDefault

The if block in line 364 after that will check query["stale"] after that to override if necessary. IMO that's easier to follow than what you have now.

preetapan · 2017-06-12T15:42:08Z

agent/http_test.go

+	}
+
+	if !query.AllowStale {
+		t.Fatalf("Bad: %v", query)


Make the message be a bit more clear, maybe something like "Expected allowStale to be true but was false", rather than printing the whole query string.

preetapan

This LGTM after the couple minor changes, but @slackpad might want to do a review as well.

mterron · 2017-06-14T04:12:04Z

Should this be refactored as per the 'dns_config' object?
Something like:

"http_config": {
  "allow_stale": true,
  "max_stale": "5s",
  "api_response_headers": {
        "Access-Control-Allow-Origin": "*"
    }
}

magiconair · 2017-06-14T07:08:16Z

In general, I would prefer less boolean flags in the config since they make refactoring more difficult. Whenever you need more than two states you're stuck. Also, often you have to use negative logic like DisableFoo: false to make the default false useful. Something like default_read_consistency: (""|"stale"|"consistent") would allow for future expansion, for example.

But I think @mterron's suggestion is the better approach since it makes the configuration consistent which is more important IMO. Since I'll be touching the configuration soon I can address the boolean flag issue separately.

wojtkiewicz · 2017-06-14T09:22:04Z

@mterron max_stale feature for http endpoint doesn't seem to be implemented yet. At least I can't find it. I can extract http options to http_config struct if you want but at the moment I found only two fields that I can put there: api_response_headers and allow_stale (or default_read_consistency as @magiconair suggested).

@magiconair Seems doable but my initial intention was to touch as little consul code as possible because people usually don't accept pull requests with too invasive changes.

Let me know if you still want that http_config field. If want me to implement also that max_stale feature for http then probably I would touch a lot more code because at the moment http endpoints are reusing layer that parses requests and then they are making RPC calls directly. That makes implementing max_stale in similar way like dns not very comfortable. Maybe max_stale can be implemented directly in RPC layer to make dns and http code consistent.

magiconair · 2017-06-14T09:32:42Z

We can do this in multiple steps. Let's get the configuration right first since this will be harder to change later. I'd suggest one commit which refactor the config without adding the feature. Pls make sure that it is backwards compatible. Then a second commit which adds the feature in the new config. We can then add `max_stale` later.

wojtkiewicz · 2017-06-14T11:17:02Z

Do you prefer one pull request with multiple commits or separate pull request for every change?

magiconair · 2017-06-14T11:55:45Z

one pr with multiple commits is fine since then it is easier to track.

wojtkiewicz · 2017-06-16T09:00:18Z

@magiconair please take a look

magiconair · 2017-06-16T09:07:21Z

agent/config.go

@@ -128,6 +128,12 @@ type DNSConfig struct {
 	RecursorTimeoutRaw string        `mapstructure:"recursor_timeout" json:"-"`
 }

+// HttpApiConfig is used to fine tune the Http sub-system.
+type HttpApiConfig struct {


s/HttpApiConfig/HTTPConfig/

magiconair · 2017-06-16T09:07:39Z

agent/config.go

@@ -364,6 +370,9 @@ type Config struct {
 	// Domain is the DNS domain for the records. Defaults to "consul."
 	Domain string `mapstructure:"domain"`

+	// Http api configuration
+	HttpApiConfig HttpApiConfig `mapstructure:"http_api_config"`


s/http_api_config/http_config/

magiconair · 2017-06-16T09:13:13Z

agent/config.go

-	if len(b.HTTPAPIResponseHeaders) != 0 {
-		if result.HTTPAPIResponseHeaders == nil {
-			result.HTTPAPIResponseHeaders = make(map[string]string)
+


Can you move that functionality into DecodeConfig, please? We're doing the other arg mangling there as well.

magiconair · 2017-06-16T09:13:59Z

agent/config.go

@@ -1167,6 +1174,10 @@ func DecodeConfig(r io.Reader) (*Config, error) {
 		fmt.Fprintln(os.Stderr, "==> DEPRECATION: atlas_endpoint is deprecated and "+
 			"is no longer used. Please remove it from your configuration.")
 	}
+	if len(result.DeprecatedHTTPAPIResponseHeaders) > 0 {


When you move the mangling to DecodeConfig you can move the log statement as well. I'll take care of the other ones later.

magiconair · 2017-06-16T09:15:23Z

agent/config_test.go

@@ -327,7 +327,11 @@ func TestDecodeConfig(t *testing.T) {
 		},
 		{
 			in: `{"http_api_response_headers":{"a":"b","c":"d"}}`,
-			c:  &Config{HTTPAPIResponseHeaders: map[string]string{"a": "b", "c": "d"}},
+			c:  &Config{DeprecatedHTTPAPIResponseHeaders: map[string]string{"a": "b", "c": "d"}},


I think this should result only in the new field being set. The Atlas parameters are different since we no longer use them at all but there should be no code using the deprecated field.

magiconair · 2017-06-16T09:17:36Z

agent/config.go

@@ -132,6 +132,11 @@ type DNSConfig struct {
 type HttpApiConfig struct {
 	// ResponseHeaders are used to add HTTP header response fields to the HTTP API responses.
 	ResponseHeaders map[string]string `mapstructure:"response_headers"`
+


pls move before ResponseHeaders

magiconair · 2017-06-16T09:18:00Z

agent/config.go

@@ -914,6 +919,9 @@ func DefaultConfig() *Config {
 			MaxStale:        10 * 365 * 24 * time.Hour,
 			RecursorTimeout: 2 * time.Second,
 		},
+		HttpApiConfig: HttpApiConfig{
+			AllowStale: false,


That is the default so you can omit that.

magiconair · 2017-06-16T09:19:36Z

agent/http_test.go

@@ -433,13 +433,14 @@ func TestParseWait_InvalidIndex(t *testing.T) {
 	}
 }

-func TestParseConsistency(t *testing.T) {
+func TestParseConsigistency(t *testing.T) {


magiconair · 2017-06-16T09:26:18Z

agent/http.go

 	query := req.URL.Query()
+	b.AllowStale = !b.RequireConsistent && s.agent.config.HttpApiConfig.AllowStale


This doesn't look right since b.RequireConsistent hasn't been set at this point. I think you can write this better as

if b.AllowStale && b.RequireConsistent { error 400 } if !b.AllowStale && !b.RequireConsistent { b.AllowStale = s.agent.config.HTTPConfig.AllowStale }

@preetapan suggested that it should be this way, original version looked like this

I rearranged code so RequireConsistent is initialized before AllowStale

Maybe I'm missing something obvious but I don't think the other version is correct. @preetapan What do you think?

My original suggestion was to simplify that boolean logic because I found it hard to read, but I didn't see the full diff, so missed the fact that RequireConsistent was not yet set. Now that @wojtkiewicz has rearranged code to set RequireConsistent before, this line b.AllowStale = !b.RequireConsistent && s.agent.config.HttpApiConfig.AllowStale , is doing the right thing. The flow is: First initialize allowStale according to whether the httpApiConfig.allowStale has it set (AND if b.RequireConsistent is not set, since you shouldn't have allowStale/allowConsistent both be true).
Then in the next line, it potentially overrides the value of allowStale if the query param contains "stale"
+1 to your suggestion of failing with a 400 if both are set . It looks like line 371 is already doing that, and in the right place.

magiconair · 2017-06-16T09:28:00Z

agent/http_test.go

 	t.Parallel()
+	a := NewTestAgent(t.Name(), nil)


defer a.Shutdown()

magiconair · 2017-06-16T09:31:48Z

agent/http_test.go

@@ -462,15 +463,29 @@ func TestParseConsistency(t *testing.T) {
 	if !b.RequireConsistent {
 		t.Fatalf("Bad: %v", b)
 	}
+


I think I'd prefer if you refactor the two ParseInconsistency tests as follows to ensure that we cover all cases:

func TestParseConsistency(t *testing.T) { t.Run("no stale, no consistent, no allowStale", func(t *testing.T) { ... }) t.Run("stale, no consistent, no allowStale", func(t *testing.T) { ... }) ... tests for all eight combinations }

work in progress

I made a bunch of parametrized tests. I hope that's more readable.

magiconair · 2017-06-16T19:53:05Z

@wojtkiewicz HTTPConfig change LGTM. Waiting for the tests to review.

wojtkiewicz · 2017-06-20T07:52:35Z

Sorry for the delay, I had some other work to do. I will try to get it done today.

magiconair · 2017-06-20T10:02:37Z

That would be nice. I'd like to merge this.

wojtkiewicz · 2017-06-20T15:33:10Z

@magiconair I refactored those tests, let me know if you need anything else.

magiconair · 2017-06-20T19:09:44Z

agent/http_test.go

-	}
-	if !b.RequireConsistent {
-		t.Fatalf("Bad: %v", b)
+	var tests = []struct {


two comments:

formatting

tests := []struct { url string allowStale bool wantAllowStale bool wantRequireConsistent bool }{ {"/v1/catalog/nodes?stale", false, true, false}, {"/v1/catalog/nodes?stale", true, true, false}, {"/v1/catalog/nodes?consistent", false, false, true}, {"/v1/catalog/nodes?consistent", true, false, true}, {"/v1/catalog/nodes", false, false, false}, {"/v1/catalog/nodes", true, true, false}, } for _, tt := range tests { name := fmt.Sprintf("url=%v, HTTP.AllowStale=%v", tt.url, tt.allowStale) t.Run(name, func(t *testing.T) { a.srv.agent.config.HTTPConfig.AllowStale = tt.allowStale var q structs.QueryOptions req, _ := http.NewRequest("GET", tt.url, nil) if d := a.srv.parseConsistency(resp, req, &q); d { t.Fatalf("Failed to parse consistency.") } if got, want := q.AllowStale, tt.wantAllowStale; got != want { t.Fatalf("got allowStale %v want %v", got, want) } if got, want := q.RequireConsistent, tt.wantRequireConsistent; got != want { t.Fatalf("got requireConsistent %v want %v", got, want) } }) }

I think the fact that we need to change the server config for this test to work shows a wrong design. parseConsistency should not depend on the server configuration. This should be a utility function which is independent of the server. Can you just pass the allowStale parameter from the config into the function?

func parseConsistency(resp http.ResponseWriter, req *http.Request, allowStale bool, opts *structs.QueryOptions) {...}

magiconair · 2017-06-20T20:43:58Z

agent/http.go

@@ -359,14 +359,15 @@ func parseWait(resp http.ResponseWriter, req *http.Request, b *structs.QueryOpti

 // parseConsistency is used to parse the ?stale and ?consistent query params.
 // Returns true on error
-func parseConsistency(resp http.ResponseWriter, req *http.Request, b *structs.QueryOptions) bool {
+func (s *HTTPServer) parseConsistency(resp http.ResponseWriter, req *http.Request, allowStale bool, b *structs.QueryOptions) bool {


Then parseConsistency does not need to be a member of the HTTPServer anymore. Maybe add a comment what allowStale is used for.

magiconair · 2017-06-21T03:50:01Z

LGTM. Can you please update the documentation in website/agent/options.html.md ?

wojtkiewicz · 2017-06-21T08:29:29Z

Sure but I'm at the conference for a few days. I can push that in the evening though.

magiconair · 2017-06-21T08:36:06Z

Sounds good. Enjoy the conference :)

wojtkiewicz · 2017-06-22T06:09:02Z

@magiconair I updated documentation.

magiconair · 2017-06-22T08:35:49Z

@wojtkiewicz I've made some small changes to the docs since it was still referring to the old name. I've also made a small change to the AllowStale parameter to be a *bool to be in line with the current DNSConfig. I'm reworking that at the moment but would like to keep it consistent for now.

wojtkiewicz · 2017-06-22T09:14:26Z

@magiconair wonderful, thanks for your help

This reverts commit 1e0fd27.

magiconair · 2017-06-27T05:27:11Z

@wojtkiewicz we have decided to roll this feature back since AllowStale was on by default (which is bad) and allowing that option has much wider ranging consequences on the HTTP API than on DNS. By enabling this you can no longer read your own writes on the KV endpoint. We might allow that as a query parameter but setting a default in the config file but in any case we need to give it more thought. We will keep the http_config change though.

wojtkiewicz · 2017-06-27T06:16:17Z

I explicitly made AllowStale false by default then you suggested that it is not necessary. If you are making KV queries then you can specify consistent since you require that. We have separate KV cluster for KV storage because of that interesting db that never shrinks. Even after load test from single service.

slackpad · 2017-06-27T06:26:25Z

@wojtkiewicz sorry for the back and forth on this one. I think defaulting it to true was our mistake but in that configuration I managed to get myself into a super confusing situation while doing some integrated testing for tomorrow's release, so I want to give this more thought (maybe we should have a way to default it just on certain endpoints or something like that). We will consider this again for 0.9 for sure, but wanted to be careful with this if we can even confuse ourselves with it :-)

magiconair · 2017-06-27T06:35:51Z

@wojtkiewicz That was indeed my mistake to not consider the full implications of the change. DNS is a read-only interface and therefore it is much safer to have a global option to allow stale reads. I think allowing this as an option on the query without providing a global option might be the right compromise but as @slackpad said we want to think this through since the implications for other users could be significant.

wojtkiewicz · 2017-06-27T06:57:29Z

I understand but primary use case for consul is a discovery service. At least from my point of view. Making all services specify some query option so that your cluster can scale horizontally is just not sensible even if it protects user from some specific confusion. I hope that we can somehow address the KV case in the future for instance by having separate configuration. I never really wanted to turn on this feature by default for anybody since it might have some negative consequences but if those do not apply to you: I find that feature to be very useful.

magiconair · 2017-06-27T07:02:47Z

I agree that the feature is useful but I disagree that consul is merely a discovery service. KV and Locks are important parts of the product which are currently used with a certain expectation in mind. Allowing a global option to change the behavior of the entire API has much wider ranging consequences than it does for DNS where you can only use the service discovery part.

wojtkiewicz · 2017-06-27T07:58:01Z

Obviously I'm not going to argue about which feature is important. My intention was never to touch or break KV or Locks with this feature. I think it is possible to implement this touching only service discovery functionality. Original PR was supposed to implement this using as little code as possible but from what I understood you don't mind refactoring here and there. Let me know if you are still interested.

wojtkiewicz · 2017-06-27T11:23:19Z

@magiconair @slackpad I hope I didn't upset you too much, that wasn't my intention :-)

magiconair · 2017-06-27T15:32:53Z

Of course we're not upset. Having users who care is important. Please keep pushing! :)

wojtkiewicz force-pushed the agent-http-stale-default branch from f49b9ad to 1f8128b Compare June 12, 2017 12:39

wojtkiewicz changed the title ~~Added option to set "stale" consistency by default for http requests.~~ Option to set "stale" consistency by default for http requests. Jun 12, 2017

preetapan reviewed Jun 12, 2017

View reviewed changes

wojtkiewicz force-pushed the agent-http-stale-default branch from 1f8128b to 2d3485c Compare June 12, 2017 15:27

preetapan reviewed Jun 12, 2017

View reviewed changes

wojtkiewicz force-pushed the agent-http-stale-default branch from 2d3485c to 8823091 Compare June 12, 2017 15:47

wojtkiewicz force-pushed the agent-http-stale-default branch from 8823091 to 1b28cb8 Compare June 16, 2017 08:57

wojtkiewicz force-pushed the agent-http-stale-default branch from 2f252eb to f1bef4c Compare June 16, 2017 09:02

magiconair reviewed Jun 16, 2017

View reviewed changes

Introduced HTTPConfig.

c9cf7e0

wojtkiewicz force-pushed the agent-http-stale-default branch 2 times, most recently from 6e0c8ac to 5ee12b4 Compare June 16, 2017 12:37

wojtkiewicz force-pushed the agent-http-stale-default branch from 5ee12b4 to 8b37ef0 Compare June 20, 2017 15:13

magiconair reviewed Jun 20, 2017

View reviewed changes

wojtkiewicz force-pushed the agent-http-stale-default branch from 8b37ef0 to 3880969 Compare June 20, 2017 20:18

magiconair reviewed Jun 20, 2017

View reviewed changes

wojtkiewicz force-pushed the agent-http-stale-default branch from 3880969 to 5138faf Compare June 20, 2017 20:50

wojtkiewicz force-pushed the agent-http-stale-default branch from 5138faf to b1a6ffb Compare June 20, 2017 21:01

Added option to set "stale" consistency by default for http requests.

28ceaa6

wojtkiewicz force-pushed the agent-http-stale-default branch from b1a6ffb to 28ceaa6 Compare June 22, 2017 06:06

magiconair closed this in 1e0fd27 Jun 22, 2017

magiconair added a commit that referenced this pull request Jun 27, 2017

Revert "agent: add allowStale option for HTTP API (#3142)"

6dd68f7

This reverts commit 1e0fd27.

magiconair mentioned this pull request Jun 27, 2017

Changes HTTP allow_stale default to false. #3196

Closed

		query := req.URL.Query()
		b.AllowStale = !b.RequireConsistent && s.agent.config.HttpApiConfig.AllowStale

Option to set "stale" consistency by default for http requests. #3142

Option to set "stale" consistency by default for http requests. #3142

Conversation

wojtkiewicz commented Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

mterron commented Jun 14, 2017 • edited Loading

magiconair commented Jun 14, 2017

wojtkiewicz commented Jun 14, 2017

magiconair commented Jun 14, 2017 via email • edited Loading

wojtkiewicz commented Jun 14, 2017

magiconair commented Jun 14, 2017

wojtkiewicz commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtkiewicz Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Jun 16, 2017

wojtkiewicz commented Jun 20, 2017

magiconair commented Jun 20, 2017

wojtkiewicz commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Jun 21, 2017

wojtkiewicz commented Jun 21, 2017

magiconair commented Jun 21, 2017

wojtkiewicz commented Jun 22, 2017

magiconair commented Jun 22, 2017

wojtkiewicz commented Jun 22, 2017

magiconair commented Jun 27, 2017

wojtkiewicz commented Jun 27, 2017

slackpad commented Jun 27, 2017

magiconair commented Jun 27, 2017

wojtkiewicz commented Jun 27, 2017

magiconair commented Jun 27, 2017

wojtkiewicz commented Jun 27, 2017 • edited Loading

wojtkiewicz commented Jun 27, 2017

magiconair commented Jun 27, 2017

wojtkiewicz commented Jun 12, 2017 •

edited

Loading

mterron commented Jun 14, 2017 •

edited

Loading

magiconair commented Jun 14, 2017 via email •

edited

Loading

wojtkiewicz Jun 16, 2017 •

edited

Loading

magiconair Jun 20, 2017 •

edited

Loading

wojtkiewicz commented Jun 27, 2017 •

edited

Loading