tomMoulard / htransformation

A Traefik plugin to change on the fly header's value of a request
MIT License
77 stars 13 forks source link

fix: custom Host header. #59

Closed ISNing closed 4 months ago

ISNing commented 5 months ago

https://github.com/traefik/traefik/pull/6502

tomMoulard commented 5 months ago

Hello @ISNing,

Thanks for your interest in this Traefik plugin !

Could you add unit tests for your changes for future backward compatibility ?

ISNing commented 5 months ago

Yes for sure, I will try to add that after I made everything work :) (Since I'm haven't ever been working with go, it might cost some time)

ISNing commented 5 months ago

@tomMoulard Everything seems works well (atleast for unit tests) But have no idea how to test it in real traefik...

tomMoulard commented 5 months ago

If you opened this bug fix, you should have a use case where this PR would fix a bug, right ? Either way, to paraphrase the README, you can do docker compose up at the root of the project. Doing so will spin up a traefik with a whoami container with the plugin attached to the route whoami.localhost. Therefore, you can do curl whoami.localhost and it will use the local version of the plugin. The configuration file is the yules-htransformation.yaml file.

ISNing commented 5 months ago

@tomMoulard Thanks for your remindness, I've tested it with the docker image. It works as expected. The bug that it fixes is just like it described in the pr from traefik's header plugin. This pr enables people change there real Host header.

(Sorry for didn't read the readme)

tomMoulard commented 5 months ago

Okay, I will do further real life tests on my side!

Do you mind refacto a little the code? I am thinking at a module that wraps each header modifications (add, set, delete,...) Using pseudo go code, it could look like this:

func deleteHeader(request, header) {
    if strings.EqualFold(rule.Header, "Host") {
        req.Host = ""
    } else {
        header.Del(rule.Header)
    }
} 

that could be called like deleteHeader(req, rule.Header). WDYT?

ISNing commented 5 months ago

Okay, I will do further real life tests on my side!

Do you mind refacto a little the code? I am thinking at a module that wraps each header modifications (add, set, delete,...) Using pseudo go code, it could look like this:

func deleteHeader(request, header) {
  if strings.EqualFold(rule.Header, "Host") {
      req.Host = ""
  } else {
      header.Del(rule.Header)
  }
} 

that could be called like deleteHeader(req, rule.Header). WDYT?

In fact, I thought about it when I first started writing the code, but I didn't do it because I wasn't familiar with go and didn't know which file to put them is better.

So, how would you like to name the file? Maybe helper.go?

tomMoulard commented 5 months ago

Personally, I would create a pkg/utils package with a header.go file inside.

tomMoulard commented 4 months ago

Indeed, this seems to be a typo, thanks for catching it up!

ISNing commented 4 months ago

Personally, I would create a pkg/utils package with a header.go file inside.

I finished and pushed this minutes before.

@tomMoulard It seems to be a typo of the original code, I think you may should give it a look.

https://github.com/tomMoulard/htransformation/blob/814426f0be3f5382fdd9090ea88d7d87c4c1cd4c/pkg/handler/deleter/delete.go#L15

And do you want me to open another pr to correct this? Or you may prefer to correct it by yourself?

tomMoulard commented 4 months ago

Please do correct it! 💯

ISNing commented 4 months ago

Please do correct it! 💯

Done here: https://github.com/tomMoulard/htransformation/pull/63

ISNing commented 4 months ago

(sorry for my misoperation deleting the branch

tomMoulard commented 4 months ago

I am trying to push a review commit, but I get a permission denied when I try to push.

I get the following:

$ git push
To github.com:ISNing/htransformation.git
 ! [remote rejected] ISNing/main -> ISNing/main (permission denied)
error: failed to push some refs to 'github.com:ISNing/htransformation.git'

Do you mind allowing me to push to your repository ? Otherwise I will get forced to open another PR that will overseed this one.

ISNing commented 4 months ago

I am trying to push a review commit, but I get a permission denied when I try to push.

I get the following:

$ git push
To github.com:ISNing/htransformation.git
 ! [remote rejected] ISNing/main -> ISNing/main (permission denied)
error: failed to push some refs to 'github.com:ISNing/htransformation.git'

Do you mind allowing me to push to your repository ? Otherwise I will get forced to open another PR that will overseed this one.

I've invite you to be collaborator of my repository. You should be able to access the branch as soon as you accepted the invitation.

tomMoulard commented 4 months ago

Thanks a lot ! the latest release v0.3.0 contains this fix if you want to try it out !