Fix h2c support

[anivanovic]

About

This solves #786 issue in krakend-ce repo.

Move support for h2c from gin router init to http server
init.

Description

If we initialize h2c using gin router it will use h2c handler
to wrap handler with all routes registered on gin router.
The problem is that it is possible to construct router factory
with RunServer function which will wrap h2c handler with additional
handler. Any logic in this handler will not be able to understand
http2 cleartext requests.

We have this example in krakend-ce repository where CORS support
is added through RunServer function. Then we have CORS handler ->
h2c handler -> Gin router.

Simplest fix is to move h2c support to server initialization
and add h2c handler as the outermost layer.

[dhontecillas]

Hi @anivanovic , thanks for your contribution!

The PR looks good, and fixes the issue: however there is a minor detail, and is that when we move the use_h2c wrapper up the stack it no longer depends on gin framework, so the flag should not be inside the extra_config under the "github_com/luraproject/lura/router/gin" namespace. On the other side, to not break backwards compatibility (or change behaviour) for the lura framework, we need to maintain that config option there.

So, one option I am thinking of is:

• we add the use_h2c at the service level, and we maintain also the old use_h2c under the gin namespace with a deprecation warning.
• in the CE code, we check for the existence of the old flag, and if it exists we copy the value to the new use_h2c at the service level before all the setup process (and thus, it will fix the behaviour for existing configuration)

But before doing anything, let me check with some mates about this approach.

Thanks.

[dhontecillas]

@anivanovic , I've double checked with a mate, and we think that in order to be able to merge this PR we would need those minor changes.

Once this is merged, we apply the required changes at the krakend-ce level to finish the fix, and we can work on doing a new release to have the fixe available for everyone.

Again, thank you very much for your contribution.

[anivanovic]

@anivanovic , I've double checked with a mate, and we think that in order to be able to merge this PR we would need those minor changes.

Once this is merged, we apply the required changes at the krakend-ce level to finish the fix, and we can work on doing a new release to have the fixe available for everyone.

Again, thank you very much for your contribution.

Sounds good to me. Thanks on taking time reviewing the PR.

[dhontecillas]

Looks good, thanks ! :)

[dhontecillas]

the test requires an update to the provided service config , and will be ready to go.

[dhontecillas]

@anivanovic , sorry for bothering, but in order to merge the PR, we also need the commits to be signed: perhaps you will need to amend the commits and add the -s -S flags to sign the commits.

[anivanovic]

I signed off all commits with -s.

[taik0]

I signed off all commits with -s.

Hi @anivanovic ,
The -s is for the sign-off, -S is to sign the commits using your SSH/PGP Key.
See this article: https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key

[anivanovic]

I signed off all commits with -s.

Hi @anivanovic , The -s is for the sign-off, -S is to sign the commits using your SSH/PGP Key. See this article: https://docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key

@taik0 first time doing it so I need some time to set it up. I signed just the last commit. Is this enough?

[taik0]

You should sign all commits :(

Look for git resign commits, and you will find ways of creating a new branch and sign those commits.

[anivanovic]

@dhontecillas, I just need your reapproval. Everything is signed-off and signed.

[dhontecillas]

Great!

We will update also the documentation for contributions, so people are aware of the required signing process.

Thank you very much for your contribution.

[github-actions]

This pull request was marked as resolved a long time ago and now has been automatically locked as there has not been any recent activity after it. You can still open a new issue and reference this link.