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

Expose collector via Ingress in Kubernetes Cluster #2275

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pranoyk
Copy link

@pranoyk pranoyk commented Jul 28, 2023

This PR will fix the issue #1048

Solution :

  • add an Ingress field to the collector .Spec.Collector.Ingress
  • add an Ingress field to the query .Spec.Query.Ingress
  • keep .Spec.Ingress for backward compatibility as a deprecated feature.

@openshift-ci
Copy link

openshift-ci bot commented Jul 28, 2023

Hi @pranoyk. Thanks for your PR.

I'm waiting for a jaegertracing member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pranoyk
Copy link
Author

pranoyk commented Jul 28, 2023

@jpkrohling @iblancasa Can you confirm the above mentioned points in the solution is what we have finalized as per what is mentioned here.
If it's a yes I will go ahead with further implementation

@iblancasa
Copy link
Collaborator

Yes, I think that approach is OK.

cc @rubenvp8510

@pranoyk
Copy link
Author

pranoyk commented Jul 31, 2023

@iblancasa I have a query. While implementing this should the assumption be that the ingress object in the query and the ingress object in the collector should be the same? If it's going to be the same should we allow the user in case they have added an ingress object in let's say only query and copy it to the collector as well in the background and vice versa?

@iblancasa
Copy link
Collaborator

@iblancasa I have a query. While implementing this should the assumption be that the ingress object in the query and the ingress object in the collector should be the same? If it's going to be the same should we allow the user in case they have added an ingress object in let's say only query and copy it to the collector as well in the background and vice versa?

I think:

  • If you specify: .spec.ingress: an Ingress object should be created for the query and the .spec.query.ingress field should contain the same values
  • If you specify: .spec.query.ingress: an Ingress object should be created for the query and the .spec.ingress field should contain the same values
  • If you specify the .spec.collector.ingress: an Ingress object should be created for the collector.

The Ingress from the collector should be different from the one from the query, IMO.

@pranoyk
Copy link
Author

pranoyk commented Jul 31, 2023

  • If you specify: .spec.query.ingress: an Ingress object should be created for the query and the .spec.ingress field should contain the same values

Do we need this? I thought we want to deprecate .spec.ingress from the discussion on the issue.

@iblancasa
Copy link
Collaborator

  • If you specify: .spec.query.ingress: an Ingress object should be created for the query and the .spec.ingress field should contain the same values

Do we need this? I thought we want to deprecate .spec.ingress from the discussion on the issue.

At some point, but not in the near future.

@pranoyk
Copy link
Author

pranoyk commented Aug 1, 2023

  • If you specify: .spec.query.ingress: an Ingress object should be created for the query and the .spec.ingress field should contain the same values

Do we need this? I thought we want to deprecate .spec.ingress from the discussion on the issue.

At some point, but not in the near future.

Ok. So in case there are both .spec.query.ingress and .spec.ingress what should we consider? I think .spec.query.ingress would make more sense since we are going to deprecate .spec.ingress in the future.

@iblancasa
Copy link
Collaborator

  • If you specify: .spec.query.ingress: an Ingress object should be created for the query and the .spec.ingress field should contain the same values

Do we need this? I thought we want to deprecate .spec.ingress from the discussion on the issue.

At some point, but not in the near future.

Ok. So in case there are both .spec.query.ingress and .spec.ingress what should we consider? I think .spec.query.ingress would make more sense since we are going to deprecate .spec.ingress in the future.

Yes.

@pranoyk pranoyk force-pushed the expose-collector-via-ingress branch from 9b5248c to e5d2a6f Compare August 3, 2023 06:58
@pranoyk
Copy link
Author

pranoyk commented Aug 4, 2023

So here is what I am planning to do. I am copying the data between .spec.query.ingress and .spec.ingress as per the above mentioned conditions.
I also plan to remove the dependency from .spec.ingress and add .spec.query.ingress throughout the code base because .spec.ingress would be deprecated.

One thing with spec.collector.ingress is that right now I cannot .spec.collector being dependent on .spec.ingress anywhere in the codebase. I am not sure how to handle this. @iblancasa can you help me with this ?

@iblancasa
Copy link
Collaborator

One thing with spec.collector.ingress is that right now I cannot .spec.collector being dependent on .spec.ingress anywhere in the codebase.

Please, could you elaborate a little bit more?

@pranoyk
Copy link
Author

pranoyk commented Aug 4, 2023

One thing with spec.collector.ingress is that right now I cannot .spec.collector being dependent on .spec.ingress anywhere in the codebase.

Please, could you elaborate a little bit more?

wrt .spec.query.ingress I can figure out how to use it in order to set the ingress, for example, we are validating and using the ingress in pkg/ingress/query.go. But with .spec.collector.ingress I cannot figure out where in the code we are already using .spec.ingress for .spec.collector.ingress.

@iblancasa
Copy link
Collaborator

One thing with spec.collector.ingress is that right now I cannot .spec.collector being dependent on .spec.ingress anywhere in the codebase.

Please, could you elaborate a little bit more?

wrt .spec.query.ingress I can figure out how to use it in order to set the ingress, for example, we are validating and using the ingress in pkg/ingress/query.go. But with .spec.collector.ingress I cannot figure out where in the code we are already using .spec.ingress for .spec.collector.ingress.

The collector is not creating any ingresses right now.

@pranoyk pranoyk force-pushed the expose-collector-via-ingress branch from f716aaa to 61de93b Compare August 8, 2023 07:37
@pranoyk pranoyk changed the title WIP : Expose collector via Ingress in Kubernetes Cluster Expose collector via Ingress in Kubernetes Cluster Aug 9, 2023
@pranoyk pranoyk force-pushed the expose-collector-via-ingress branch from 61de93b to 2ed5f73 Compare August 9, 2023 05:13
@pranoyk
Copy link
Author

pranoyk commented Aug 9, 2023

@iblancasa Could you approve the e2e-tests to be run, also the PR is ready to be reviewed.

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.

2 participants