Parse datacenter from request #12370
Conversation
thisisnotashwin
force-pushed
the
ashwin/parse-dc-for-authmethod-and-binding-create
branch
from
e363809
to
79b009b
Compare
- Parse the value of the datacenter from the create/delete requests for AuthMethods and BindingRules so that they can be created in and deleted from the datacenters specified in the request.
thisisnotashwin
force-pushed
the
ashwin/parse-dc-for-authmethod-and-binding-create
branch
from
79b009b
to
c05e37c
Compare
agent/acl_endpoint_test.go
Outdated
| @@ -1341,7 +1361,7 @@ func TestACL_LoginProcedure_HTTP(t *testing.T) { | |||
| BindName: "web", | |||
| } | |||
|
|
|||
| req, _ := http.NewRequest("PUT", "/v1/acl/binding-rule?token=root", jsonBody(ruleInput)) | |||
There was a problem hiding this comment.
hm, are those breaking changes?
as in, if I'm a practitioner on 1.10.x or 1.11.x and don't URL encode the datacenter, will my existing code still work?
It doesn't seem like it right?
There was a problem hiding this comment.
parseDC should also default the datacenter to the current one:
// parseDC is used to parse the ?dc query param
func (s *HTTPHandlers) parseDC(req *http.Request, dc *string) {
if other := req.URL.Query().Get("dc"); other != "" {
*dc = other
} else if *dc == "" {
*dc = s.agent.config.Datacenter
}
}
I don't think these test assertions should need &dc=dc1
There was a problem hiding this comment.
It is not a breaking change! If no datacenter is provided, it will continue to behave as it previously did. In case someone was passing a query param that referenced a datacenter that was incorrect, that will fail, but that would be desired behavior.
This actually ensures consistent behavior between the read/list requests and the write and delete requests!
There was a problem hiding this comment.
Updated the tests @rboyer
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
|
|
||
| req, _ := http.NewRequest("PUT", "/v1/acl/auth-method?token=root&dc=invalid", jsonBody(methodInput)) | ||
| resp := httptest.NewRecorder() | ||
| _, err := a.srv.ACLAuthMethodCRUD(resp, req) |
There was a problem hiding this comment.
Not a blocker, just an FYI that we've started to change our tests to do this: #11396
Right now the url path on line 1237 is ignored (the query params are used though), and the test doesn't exercise any of the wrappers around endpoints.
Instead of calling ACLAuthMethodCRUD we are moving to call a.srv.h.ServeHTTP, which makes the test closer to a real API call.
There was a problem hiding this comment.
🙏 thanks for the heads up! I don't mind making this change across the file.
agent/acl_endpoint_test.go
Outdated
| @@ -1222,6 +1222,26 @@ func TestACL_LoginProcedure_HTTP(t *testing.T) { | |||
| methodMap[method.Name] = method | |||
| }) | |||
|
|
|||
| t.Run("Create in invalid datacenter", func(t *testing.T) { | |||
There was a problem hiding this comment.
Not a blocker, but I believe the purpose of this test is to show we accept a dc query parameter, right? Since we don't have a backend interface we can fake, and we don't want to have to setup a whole second server in a different DC the test happens to fail with invalid DC. But I believe the underlying thing we are testing is that parseDC is called and sets the DC value correct. The current name suggests it is more about testing validation of the dc query parameter, but I think that might be more incidental than the primary purpose of the case.
I would suggest changing the name to something like this:
| t.Run("Create in invalid datacenter", func(t *testing.T) { | |
| t.Run("Create in remote datacenter", func(t *testing.T) { |
There was a problem hiding this comment.
That is a good catch. I was testing incidental behavior purely for the reasons you have described. Ill rename these tests to reflect that behavior they do test.
There was a problem hiding this comment.
i might have spoken too soon! the refactor seems large, and ill punt on it for now. Thanks so much for the heads up!
thisisnotashwin
force-pushed
the
ashwin/parse-dc-for-authmethod-and-binding-create
branch
from
c1fe002
to
dd40882
Compare
thisisnotashwin
deleted the
ashwin/parse-dc-for-authmethod-and-binding-create
branch
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/588266. |
|
🍒✅ Cherry pick of commit 6e6cd92 onto |
* Parse datacenter from request - Parse the value of the datacenter from the create/delete requests for AuthMethods and BindingRules so that they can be created in and deleted from the datacenters specified in the request.
|
🍒✅ Cherry pick of commit 6e6cd92 onto |
* Parse datacenter from request - Parse the value of the datacenter from the create/delete requests for AuthMethods and BindingRules so that they can be created in and deleted from the datacenters specified in the request.
|
🍒✅ Cherry pick of commit 6e6cd92 onto |
* Parse datacenter from request - Parse the value of the datacenter from the create/delete requests for AuthMethods and BindingRules so that they can be created in and deleted from the datacenters specified in the request.
Parse the value of the datacenter from the create/delete requests for AuthMethods and BindingRules so that they can be created in and deleted from the datacenters specified in the request.