Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: custom Host header. #59

Merged
merged 3 commits into from
May 14, 2024
Merged

fix: custom Host header. #59

merged 3 commits into from
May 14, 2024

Conversation

ISNing
Copy link
Contributor

@ISNing ISNing commented May 3, 2024

@tomMoulard
Copy link
Owner

Hello @ISNing,

Thanks for your interest in this Traefik plugin !

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

@tomMoulard tomMoulard added the enhancement New feature or request label May 3, 2024
@ISNing
Copy link
Contributor Author

ISNing commented May 3, 2024

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 ISNing force-pushed the main branch 5 times, most recently from edabbee to 225d49c Compare May 3, 2024 14:03
@ISNing
Copy link
Contributor Author

ISNing commented May 3, 2024

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

@tomMoulard
Copy link
Owner

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
Copy link
Contributor Author

ISNing commented May 3, 2024

@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)

@ISNing ISNing marked this pull request as ready for review May 3, 2024 15:57
@tomMoulard
Copy link
Owner

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
Copy link
Contributor Author

ISNing commented May 3, 2024

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
Copy link
Owner

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

Copy link
Contributor Author

@ISNing ISNing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rw.Header().Del(rule.Name)

@tomMoulard
Copy link
Owner

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

@ISNing
Copy link
Contributor Author

ISNing commented May 5, 2024

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.

rw.Header().Del(rule.Name)

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

@tomMoulard
Copy link
Owner

Please do correct it! 💯

@ISNing
Copy link
Contributor Author

ISNing commented May 5, 2024

Please do correct it! 💯

Done here: #63

@ISNing ISNing deleted the branch tomMoulard:main May 8, 2024 13:08
@ISNing ISNing closed this May 8, 2024
@ISNing ISNing deleted the main branch May 8, 2024 13:08
@ISNing ISNing restored the main branch May 8, 2024 13:08
@ISNing ISNing reopened this May 8, 2024
@ISNing
Copy link
Contributor Author

ISNing commented May 8, 2024

(sorry for my misoperation deleting the branch

@tomMoulard tomMoulard self-assigned this May 13, 2024
@tomMoulard tomMoulard self-requested a review May 13, 2024 08:11
@tomMoulard
Copy link
Owner

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.

@tomMoulard tomMoulard removed their assignment May 13, 2024
@tomMoulard tomMoulard added the contributor/waiting-for-feedback Feedback from the contributor is needed label May 13, 2024
@ISNing
Copy link
Contributor Author

ISNing commented May 13, 2024

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 tomMoulard removed the contributor/waiting-for-feedback Feedback from the contributor is needed label May 14, 2024
@tomMoulard tomMoulard merged commit d3b793a into tomMoulard:main May 14, 2024
2 checks passed
@tomMoulard
Copy link
Owner

tomMoulard commented May 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants