-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[receiver/tlscheck] Inital Commit of TLS Check Receiver #35441
[receiver/tlscheck] Inital Commit of TLS Check Receiver #35441
Conversation
77ab636
to
b1818e0
Compare
4732a4a
to
2ebf9ee
Compare
9ef2ddd
to
b2e523a
Compare
c43a81f
to
48103c1
Compare
if cfg.URL == "" { | ||
err = multierr.Append(err, errMissingURL) | ||
} else { | ||
_, parseErr := url.ParseRequestURI(cfg.URL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the expected behavior (of validate && the receiver) for a non-https URL? I would expect it to be invalid at validate time, as I don't see a good mechanism for returning that information at runtime in a way that would be consumable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point and something definitely overlooked.
For the sake of the PR being open so long and becoming harder to merge, I would love to see this as a follow up PR instead updating the current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should allow for scheme. We do not need to send an http request here, only establish tcp connection, so scheme doesn't make sense. Related, I think we should change url
to host
since we are connecting to a host address via tcp, not sending a http request to a url.
Description: Initial commit of TLS Check Receiver, a new component aimed at emitting metrics about the expoiry of x.509 certs.
Link to tracking Issue: 35423
Testing: Test files added
Documentation:
README.md
&metadata.yaml
added