Add HTTP request headers to Firebase TestRuleset request. by sarvaniv · Pull Request #246 · istio/proxy

sarvaniv · 2017-04-13T18:49:20Z

Currently we send the JWT token payload to the TestRuleset. This change also sends HTTP request headers in TestRulesetRequest. This change also requires a change on ESP. The code changes are done on ESP and I am working on a t-test in ESP to test those changes.

Updated the unit test.

qiwzhang · 2017-04-13T18:56:44Z

contrib/endpoints/src/api_manager/firebase_rules/firebase_request.cc

+  }
+
+  auto *map = headers->mutable_struct_value()->mutable_fields();
+  for (auto const &kvp : req_headers) {


const auto&

istio-testing · 2017-04-13T19:44:24Z

Jenkins job proxy/presubmit passed

liminw · 2017-04-13T20:08:39Z

contrib/endpoints/include/api_manager/request.h

                                           const std::string &value) = 0;
+
+  // Gets all HTTP request headers.
+  virtual void GetHeaders(std::map<std::string, std::string> *headers) {}


Shouldn't this be "= 0"? I suppose we are going to have a follow up change in ESP repo to provide the implementation of GetHeaders()?

I have provided all the information in the PR description. This cannot be 0. If this is 0, then ESP that will point to this proxy's commit will have issues creating a request object since it does not implement a pure virtual function. Having this empty body will fix that issue.

We can convert this into a pure virtual function but we have to wait until we merged back to main whenever that is going to happen. Until then this code cannot be changed.

Makes sense. Probably you can have a comment here saying that this is a workaround needed until ESP implementation is done, plus proxy and ESP repo are synced.

On a second thought we have to very careful about making this pure virtual since Envoy also implements this class. Let's not be too eager to change this into a pure virtual function.

As discussed with Wayne, making this pure virtual right now can break other's using the latest proxy and ESP code. This is the safest thing to do. I also want to know the full Envoy story. Someone is implementing the request class that can be used on Proxy when used with Envoy. I don't know who implements this class and we use this Request class everywhere in the proxy code. Let's take the safest route and not break anyone. I think its good karma :-)

I agreed with Lizan. If you worry about the breakage of ESP, you should immediately make ESP PR with dummy implementation.

You mean in master branch? I don't care about this branch since we are just 2 people on this branch. But do you want me to first do this change on master so that we can merge both the changes together?

Otherwise, when we merge into master, we need to remember an additional step that before we do anything on ESP branch, we provide the dummy implementation in ESP and then merge firebase branch into master.

I am actually not completely sure what is the big deal since this will become a pure virtual function soon and in a way that will make everyone's lives easy and also without the pressure of having to time the merges for proxy and ESP. Just wondering the reason why this is such a concern?

istio-testing · 2017-04-13T20:58:48Z

Jenkins job proxy/presubmit passed

istio-testing · 2017-04-13T21:18:11Z

Jenkins job proxy/presubmit passed

lizan · 2017-04-13T21:22:09Z

contrib/endpoints/include/api_manager/request.h

+  // the GetHeaders method. Changing this to a pure virtual function should be
+  // done with a lot of care and making sure that all implementations of this
+  // class have implemented this method.
+  virtual void GetHeaders(std::map<std::string, std::string> *headers) {}


Actually having a empty implementation here make it harder to notice some implementation don't implement this, since the build won't break.

The build will break if someone in the team have their ESP point to the latest proxy code as I do a lot during my development/testing. Especially when we merge these changes into master. We cannot be too far off when we merge both proxy and ESP changes into the master and depending on how things go, this is not something we should be too sure about. I must disagree on this. I will make it a pure virtual function once we are in master.

I didn't notice that this is firebase branch. Feel free to merge this but I would block this when you try to merge to master.

This class here is a pure interface, so it shouldn't have any (even if it is dummy) implementations at any time.

Currently the only implementation of this interface is in ESP, and ESP depends on istio/proxy on specific commit. So I don't think making this pure virtual will break anything.

While ESP depends on a specific commit, the general guideline for merging proxy code is that we point ESP to the local proxy and make sure that ESP tests also pass before we merge changes. But this change is a breaking change if we make it pure virtual right away. Is it OK to block peoples' progress? Instead we will have this temporary fix that will allow people to keep doing their work while we can work on any blocking issues that may come up during the merge. I think the first principle is to never block people and let things go smoothly for everyone. Sometimes perfection is not the best solution :-)

No, some refactoring cannot be done without breaking ESP, (e.g. #225 and ESP change). It is OK to merge them, then to mitigate the issue, have a dummy (of course if you have real one it is better) implementation ASAP in ESP too.

This is not a big deal, but comparing 2 solutions, (A) adding a dummy implementation first hand v.s. (B) changing this method to pure virtual are almost same effort (in terms of lines of change, number of PRs), when we do (B), it is hard to track whether the fix is made as nothing is breaking anymore.

If this method does not become a pure virutal function within a few hours of merging ESP into master, please hold me responsible for that. We can convert it into a pure virtual function without breaking builds or trying to keep merges to ESP and proxy very close when there is at least 1-2 hour delay for tests to run. Given that we are merging into main from firebase, this may take longer if we find any issues. I am hoping to make a strong case that we should be able to merge this code into master.

The primary difference between A and B, is that the merges are smooth and no one is blocked since he merge cannot be instantaneous. I think making builds non-breaking is the best thing we can do to our fellow team members and should make this a strong requirement for all merges. I would personally dislike being blocked on build breaks.

the only case you're blocking with A is someone want to update the istio/proxy commit contains your change in ESP, since this is happening in firebase branch, so it doesn't block anyone.

I am not talking about firebase branch. I am talking about the case when we merge into master. As I said this will be very temporary change and I will make sure that the method becomes pure virtual ASAP once we merge to master. I still don't know what is the issue here...

Alright, I will make this pure virutal and before merging into master, I will have a dummy implementation that I will check into ESP branch. Thank you all for the input.

istio-testing · 2017-04-13T23:10:08Z

Jenkins job proxy/presubmit passed

Add HTTP request headers to Firebase TestRuleset request.

0a22eaa

sarvaniv requested review from liminw and qiwzhang April 13, 2017 18:49

qiwzhang reviewed Apr 13, 2017

View reviewed changes

Address code review comment. Fix style.

4031500

liminw reviewed Apr 13, 2017

View reviewed changes

Add comment about why the method cannot be pure virtual method.

0c0d0da

Move CheckSecurityRules before CheckServiceControl in workflow.

6769a20

liminw approved these changes Apr 13, 2017

View reviewed changes

lizan suggested changes Apr 13, 2017

View reviewed changes

Convert GetHeaders to be pure virtual.

31c582b

lizan approved these changes Apr 13, 2017

View reviewed changes

sarvaniv closed this Apr 24, 2017

Conversation

sarvaniv commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Apr 13, 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!

istio-testing commented Apr 13, 2017

Uh oh!

istio-testing commented Apr 13, 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!

lizan Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

istio-testing commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lizan Apr 13, 2017 •

edited

Loading