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

feat: Add support for native TLS #1354

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

mayankshah1607
Copy link
Contributor

Fixes #1353

  • uses exporter-toolkit library to add native TLS
  • introduce new flag tls-config to pass the path to TLS config file

Signed-off-by: Mayank Shah mayankshah1614@gmail.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2021
@mayankshah1607
Copy link
Contributor Author

mayankshah1607 commented Jan 17, 2021

Hi @lilic @brancz
If this approach looks good, I'd like to add relevant tests to ensure TLS is working as expected. I am not really sure why ci-validate-manifests is failing - I would really appreciate any help on that.

PTAL, thanks! :)

@dgrisonnet
Copy link
Member

Hi @mayankshah1607, it seems to be failing because of a conflict with the yaml package. Bumping gojsontoyaml to brancz/gojsontoyaml@202f76b should prevent that.

@dgrisonnet
Copy link
Member

If we want to go forward with the TLS configuration from the exporter-toolkit, we might want to document that the configuration can be found here: https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md

@mayankshah1607
Copy link
Contributor Author

Hi @mayankshah1607, it seems to be failing because of a conflict with the yaml package. Bumping gojsontoyaml to brancz/gojsontoyaml@202f76b should prevent that.

Thanks @dgrisonnet. I tried bumping it, but it did not seem to help. Could there be any other reason?

@dgrisonnet
Copy link
Member

dgrisonnet commented Jan 19, 2021

Hum yes, after taking another look, it seems that I understood the error the other way around. The current examples/prometheus-alerting-rules/alerts.yaml manifest seems to have been incorrectly generated with the breaking version v2.3.0 of the yaml package. By introducing a new transitive-dependency on v2.4.0, you fixed this problem and made it possible to regenerate the manifest properly. Hence, regenerating the manifest and pushing the changes to this PR seems to be the right solution here.
Also, since the current gojsontoyaml version is not bound to v2.3.0, there is no need to bump it. Sorry for the inconvenience on that one.

@mayankshah1607 mayankshah1607 force-pushed the mayank/native-tls branch 3 times, most recently from 8695c92 to 79fcb04 Compare January 23, 2021 06:18
@mayankshah1607
Copy link
Contributor Author

Thanks @dgrisonnet

If we want to go forward with the TLS configuration from the exporter-toolkit, we might want to document that the configuration can be found here: https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md

If everything looks fine in this PR, I could go ahead an add the required documentation.

docs/cli-arguments.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/options/options.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
pkg/options/options.go Outdated Show resolved Hide resolved
@tariq1890
Copy link
Contributor

@mayankshah1607 Thanks so much for your PR. I have provided my review comments. Let me know if you need any more clarifications around them :)

@mayankshah1607
Copy link
Contributor Author

thanks @tariq1890
I have made all the required changes. PTAL! :)

main.go Outdated Show resolved Hide resolved
@tariq1890
Copy link
Contributor

tariq1890 commented Jan 25, 2021

So the PR e2e tests are failing because of this statement in the logs

E0125 07:24:39.122249 1 main.go:63] levelinfomsgTLS is disabled.http2false

That's a klog error. I think we might need use to klog.Info instead in the new interface implementing method.

Also, that's not a well-formed log message . A tad concerning

- introduce new flag `tls-config` to pass the path to tls config file

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2021
@tariq1890
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mayankshah1607, tariq1890

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:

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

@k8s-ci-robot k8s-ci-robot merged commit b4fb3d4 into kubernetes:master Jan 25, 2021
@prasunsultania
Copy link

is there a sample --tls-config file ?

@invidian
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add native TLS support
6 participants