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

feature(h2c): optional certificates for TLS #542

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cv65kr
Copy link
Contributor

@cv65kr cv65kr commented Jul 23, 2024

Reason for This PR

In case of h2c there a no need to provide certificates with enabled TLS option

Description of Changes

If TLS is enabled certificates files are optional.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

@rustatian rustatian self-requested a review July 23, 2024 07:43
@rustatian rustatian added the C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. label Jul 23, 2024
@rustatian
Copy link
Collaborator

Hey @cv65kr 👋

To use gRPC with H2C, IIRC, we also need to update the DialContext method for the temporal client to use DialH2CContext instead.

@cv65kr
Copy link
Contributor Author

cv65kr commented Jul 23, 2024

@rustatian Will take a look, I tested it without that and works as expected

@rustatian
Copy link
Collaborator

Just curious, is there a h2c option for the temporal server?

@cv65kr
Copy link
Contributor Author

cv65kr commented Jul 23, 2024

for Temporal itself I don't think so, but if you use any ingress like treafik or istio you can enable h2c option for 7233 port

@rustatian
Copy link
Collaborator

I'm not quite how this works, since the certificates are required here: https://github.com/cv65kr/roadrunner-temporal/blob/patch-1/tls.go#L55

From what I know, h2c for the http and gRPC is a special handler, it should not be activated by skipping the certs... So, could you please write a test for your change?

@rustatian rustatian changed the title Optional certificates for TLS feature(h2c): optional certificates for TLS Jul 25, 2024
@rustatian rustatian marked this pull request as draft July 25, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants