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

Create Ingress for extensions #3441

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ankit152
Copy link
Contributor

@Ankit152 Ankit152 commented Nov 10, 2024

@Ankit152
Copy link
Contributor Author

Ankit152 commented Nov 10, 2024

It's a POC and work is still in progress.
The discussion around the implementation is still on-going.

Once the code for extension service is merged, this branch will be re-based.

@Ankit152 Ankit152 marked this pull request as ready for review November 30, 2024 15:46
@Ankit152 Ankit152 requested a review from a team as a code owner November 30, 2024 15:46
@Ankit152 Ankit152 changed the title Ingress Create Ingress for extensions Nov 30, 2024
@Ankit152
Copy link
Contributor Author

Folks since the service for extensions is merged, I would like to discuss about the implementation of Ingress.

cc @pavolloffay @iblancasa @swiatekm @jaronoff97 @yurishkuro

@swiatekm
Copy link
Contributor

swiatekm commented Dec 2, 2024

The change looks fine to me. What I'm not convinced of is whether we need a whole separate configuration vs a boolean switch to include extension ports in the existing Ingress. I'd like to discuss that in the issue.

@Ankit152
Copy link
Contributor Author

Ankit152 commented Dec 2, 2024

Sure @swiatekm
It will be very great if we discuss the implementation on the issue itself.

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Folks since the service for extensions is merged, I would like to discuss about the implementation of Ingress.

We can put it on the agenda for the SIG call if you like.

@frzifus frzifus added the discuss-at-sig This issue or PR should be discussed at the next SIG meeting label Dec 5, 2024
@frzifus
Copy link
Member

frzifus commented Dec 5, 2024

The change looks fine to me. What I'm not convinced of is whether we need a whole separate configuration vs a boolean switch to include extension ports in the existing Ingress. I'd like to discuss that in the issue.

Can a boolean work when the extension may changes?

@swiatekm
Copy link
Contributor

swiatekm commented Dec 5, 2024

The change looks fine to me. What I'm not convinced of is whether we need a whole separate configuration vs a boolean switch to include extension ports in the existing Ingress. I'd like to discuss that in the issue.

Can a boolean work when the extension may changes?

Not sure what you mean? A boolean switch would mean we'd just add the extension paths to the existing receiver Ingress.

@frzifus
Copy link
Member

frzifus commented Dec 19, 2024

hey @Ankit152, the contribution was discussed in the SIG call today.

There three things came up:

  • Can this lead to unwanted extensions being exposed by mistake? (e.g. pprof and other things? Even if there is no port-parser implementation for something specific, it may gets added in the future and would be exposed automatically.)
  • Do we need a 2nd ingress entry? Can we maybe reuse the exsting one with a different sub domain? (e.g. customXYZ.extension.*?)
  • Would it be sufficent to just provide a extensionWhitelist that will be exposed? (OT: might be also a good idea to add a receiver whitelist then. But thats another topic to discuss)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss-at-sig This issue or PR should be discussed at the next SIG meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Collector] Consume extension service with Ingress to give users a way to interact with them
4 participants