Added headers manipulations capabilities to DestinationWeight

[omersadika]

added 3 new fields to DestinationWeight: append_headers, remove_response_headers and append_response_headers.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[omersadika]

This PR is needed for istio PR:
istio/istio#8340

[omersadika]

I signed it!

[rshriram]

/ok-to-test

[rshriram]

Hi. Soprry for the delay. I can't merge this until the CLA thing is set.. can you take care of the CLA and ping me again?

[rshriram]

Thanks for the test. Can you take care of the CLA please?

[rshriram]

/lgtm

[istio-testing]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rshriram

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rshriram]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sebastienvas]

@omersadika which identity did you use the sign the CLA ? I cannot find you in the database.

[omersadika]

I signed as a company, the name is Bevyx

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

[sebastienvas]

Bevyx Inc has signed the agreement

[omersadika]

Thank you a lot @sebastienvas and @rshriram

What about the PR in Istio repo?

istio/istio#8340

[rshriram]

@omersadika @frankbu pointed out an important fact. This destination weight is shared across tcp and other future protos. Instead of having append_headers, etc.. can you create a separate sub message in DestinationWeight, like http:.. within which, you can stick in any thing you want (append_headers, remove.., etc.). This leaves the door for TCP and TLS to do some things in future..

Something like this:

route:
- destination:
    host:...
    port:...
    subset:...
    httpOperations: or just http:
       requestHeaders:
          add: map<string, string>
          remove: array..
       responseHeaders:
          add: map<string,string>
          remove: <array>

Please rework the current structs accordingly.

Sorry for not spotting this earlier.

[omersadika]

Really good point @rshriram
I totally agree that since destination is shared we should take it into consideration.
The thing is that route has 3 different variations for each type - http, tls or tcp and maybe more in the future.
When I write the destination I see it as in the context of the route that I chose, http for example, and I assume that everything I do is for the same type. To add subtypes inside destination seems for me too redundant and could be more complex in the future.
The best solution that I see is to create destination per type, same as route although it won't be an small change for the API.
What do you think?

[frankbu]

A possibility is to add one new message, HTTPDestination, which has the same contents as DestinationWeight plus the xxx_headers fields, and then use it in HTTPRoute instead of DestinationWieght. The other route types can stay unchanged (for now).

I'm also wondering why we need these headers fields in both the HTTPRoute, as well as in the weighted routes. Are the HTTPRoute ones a mistake that should be deprecated, or do we really need them in both places. Also, if we need them in both places, shouldn't we add append_response_headers in HTTPRoute as well?

[omersadika]

Sounds good. It makes sense to have it also in HTTPRoute if you want to add it to all destinations at once. Actually the way it's done today is deprecated in Envoy so I can also update to the new way, I will also check to add the other xxx_headers options for route. Do I have a green light to make the changes?

[frankbu]

Now that I think about it, DestinationWeight is not a great name. Maybe we should rename DestinationWeight to RouteDestination, since it is the value of the destination field of the three "route" types. Then the new one, I'm suggesting could be HTTPRouteDestination. I think that would be a lot clearer.

Edit: Actually DestinationWeight is the value of the route field of the three route types (not the destination field), but I still think RouteDestination would be a better name.

@rshriram WDYT?

[rshriram]

As long as this does not involve any changes in teh field names (for backward compatibility), I am okay with the renaming and adding the header operations under the destination weight. Also, please have an option for adding/removing both request and response headers (envoy has a PR in flight to remove request headers).