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

[extension/observer/k8sobserver] add k8s.ingress endpoint #33005

Merged
merged 12 commits into from
Jun 27, 2024

Conversation

M0NsTeRRR
Copy link
Contributor

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:

  • Endpoint unit test
  • Handler unit test

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 :)

Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
@M0NsTeRRR M0NsTeRRR requested a review from a team May 11, 2024 22:46
@M0NsTeRRR M0NsTeRRR requested a review from dmitryax as a code owner May 11, 2024 22:46
@M0NsTeRRR M0NsTeRRR changed the title [extension/k8sobserver] add k8s.ingress endpoint [extension/observer/k8sobserver] add k8s.ingress endpoint May 11, 2024
@M0NsTeRRR
Copy link
Contributor Author

/label extension/observer/k8sobserver

Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
@M0NsTeRRR M0NsTeRRR requested a review from ChrsMark May 20, 2024 14:25
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
Copy link
Contributor

github-actions bot commented Jun 4, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 4, 2024
@M0NsTeRRR
Copy link
Contributor Author

Not stale

@ChrsMark
Copy link
Member

ChrsMark commented Jun 4, 2024

FYI @rmfitzpatrick, @dmitryax

Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
@M0NsTeRRR M0NsTeRRR requested a review from ChrsMark June 4, 2024 13:17
@github-actions github-actions bot removed the Stale label Jun 5, 2024
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 20, 2024
@M0NsTeRRR
Copy link
Contributor Author

Not stale

@github-actions github-actions bot removed the Stale label Jun 21, 2024
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

extension/observer/k8sobserver/ingress_endpoint.go Outdated Show resolved Hide resolved
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know; that was my initial question. How do other components deal with that ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

receiver/receivercreator/README.md Show resolved Hide resolved
M0NsTeRRR and others added 2 commits June 27, 2024 12:03
Co-authored-by: Chris Mark <chrismarkou92@gmail.com>
Signed-off-by: Ludovic Ortega <ludovic.ortega@adminafk.fr>
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dmitryax dmitryax merged commit 7c573a9 into open-telemetry:main Jun 27, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants