[extension/observer/k8sobserver] add k8s.ingress endpoint #33005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ludovic Ortega <[email protected]>
M0NsTeRRR
changed the title
|
/label extension/observer/k8sobserver |
ChrsMark
reviewed
May 20, 2024
Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale |
ChrsMark
reviewed
Jun 4, 2024
|
FYI @rmfitzpatrick, @dmitryax |
Signed-off-by: Ludovic Ortega <[email protected]>
ChrsMark
reviewed
Jun 5, 2024
Signed-off-by: Ludovic Ortega <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale |
ChrsMark
reviewed
Jun 27, 2024
There was a problem hiding this comment.
This one looks like a useful addition and overall the patch looks good to me!
@dmitryax @rmfitzpatrick @mx-psi any thoughts on this? What would be the best way to move this forward?
| if rule.HTTP != nil { | ||
| // Create endpoint for each ingress rule. | ||
| for _, path := range rule.HTTP.Paths { | ||
| endpointID := observer.EndpointID(fmt.Sprintf("%s/%s/%s%s", idNamespace, ingress.UID, rule.Host, path.Path)) |
There was a problem hiding this comment.
I think it makes sense to escape the backslashes here and be explicit with the ID format to avoid any kind of unlikely ID collision?
Suggested change
| endpointID := observer.EndpointID(fmt.Sprintf("%s/%s/%s%s", idNamespace, ingress.UID, rule.Host, path.Path)) | |
| endpointID := observer.EndpointID(fmt.Sprintf("%s/%s/%s/%s", idNamespace, ingress.UID, rule.Host, escBackslashes(path.Path))) |
There was a problem hiding this comment.
I don't know; that was my initial question. How do other components deal with that ?
There was a problem hiding this comment.
We just need to ensure the uniqueness of the endpoint. I don't think the escaping is required. We shouldn't run into any conflicts without it
Co-authored-by: Chris Mark <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
dmitryax
approved these changes
Jun 27, 2024
| if rule.HTTP != nil { | ||
| // Create endpoint for each ingress rule. | ||
| for _, path := range rule.HTTP.Paths { | ||
| endpointID := observer.EndpointID(fmt.Sprintf("%s/%s/%s%s", idNamespace, ingress.UID, rule.Host, path.Path)) |
There was a problem hiding this comment.
We just need to ensure the uniqueness of the endpoint. I don't think the escaping is required. We shouldn't run into any conflicts without it
tomasmota
pushed a commit
to SpringerPE/opentelemetry-collector-contrib
that referenced
this pull request
Jul 1, 2024
…etry#33005) **Description:** Add support for k8s.ingress endpoint As described in the issue, this will allow users to dynamically obtain Kubernetes ingress ressource, facilitating the monitoring of certificate expiration with the Blackbox Exporter for example. I didn't create a global ingress resource with sub-endpoints for each path because I don't see a relevant 'target'. I'm uncertain about the impact of my decision regarding the 'UID' as it's structured as '{namespace}/{ingress UID}/{host}{path}'. Since a path automatically starts with a '/', and can contain other '/', should I escape them ? **Link to tracking Issue:** <Issue number if applicable> open-telemetry#32971 --------- Signed-off-by: Ludovic Ortega <[email protected]> Co-authored-by: Chris Mark <[email protected]>
lalith47
pushed a commit
to lalith47/opentelemetry-collector-contrib
that referenced
this pull request
Jul 1, 2024
…etry#33005) **Description:** Add support for k8s.ingress endpoint As described in the issue, this will allow users to dynamically obtain Kubernetes ingress ressource, facilitating the monitoring of certificate expiration with the Blackbox Exporter for example. I didn't create a global ingress resource with sub-endpoints for each path because I don't see a relevant 'target'. I'm uncertain about the impact of my decision regarding the 'UID' as it's structured as '{namespace}/{ingress UID}/{host}{path}'. Since a path automatically starts with a '/', and can contain other '/', should I escape them ? **Link to tracking Issue:** <Issue number if applicable> open-telemetry#32971 --------- Signed-off-by: Ludovic Ortega <[email protected]> Co-authored-by: Chris Mark <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description: Add support for k8s.ingress endpoint
As described in the issue, this will allow users to dynamically obtain Kubernetes ingress ressource, facilitating the monitoring of certificate expiration with the Blackbox Exporter for example.
I didn't create a global ingress resource with sub-endpoints for each path because I don't see a relevant 'target'. I'm uncertain about the impact of my decision regarding the 'UID' as it's structured as '{namespace}/{ingress UID}/{host}{path}'. Since a path automatically starts with a '/', and can contain other '/', should I escape them ?
Link to tracking Issue:
#32971
Testing:
Documentation:
I've updated the documentation I've found.
PS : I don't write a lot of go and it's also my first contribution to the project I've probably missed some points :)