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

Fix statefulset cross-namespace #1139

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

Conversation

iagodvsantos
Copy link
Contributor

@iagodvsantos iagodvsantos commented Jun 13, 2023

What is it?

While testing 1.4.0-rc1 release I stumbled upon an issue while exposing a statefulset cross-namespace due to the tcp/http connector host address to be referring to the current namespace instead of the one specified by the --target-namespace flag.

@iagodvsantos iagodvsantos force-pushed the stateful-cross-namespace-fix branch from 36d6666 to 2cc9d12 Compare June 14, 2023 12:44
@iagodvsantos
Copy link
Contributor Author

@fgiorgetti @grs @nluaces
Could you guys take a look?

For some reason, the test coverage step is exceeding the time limit on Circleci, it's running normally locally though

@iagodvsantos
Copy link
Contributor Author

Hey guys! Just following up on this PR @fgiorgetti @grs @nluaces. Have you had the chance to take a look at it?

@nluaces
Copy link
Member

nluaces commented Jun 29, 2023

Hi @iagodvsantos thanks for your patience, there was a change in the circle-ci configuration (unrelated to your PR) but in order to run the build again, we need that you rebase the changes from master branch into your branch; to catch up those changes.

@iagodvsantos iagodvsantos force-pushed the stateful-cross-namespace-fix branch from 2cc9d12 to 03cb802 Compare July 3, 2023 22:06
@iagodvsantos
Copy link
Contributor Author

Thanks for replying back @nluaces !
Just rebased

@nluaces
Copy link
Member

nluaces commented Jul 18, 2023

@iagodvsantos I have seen that you have added some unit tests, and also that your changes would affect only to statefulsets exposed with --headless (if I am not mistaken). How have you tested this particular use case? could you describe the manual test that you have done for this pull request? Thanks!

@iagodvsantos
Copy link
Contributor Author

@nluaces I've stumbled upon this issue while exposing a Kafka statefulset deployed in a different namespace. I noticed that the consumer app couldn't even resolve the kafka hosts to fetch initial metadata to start consuming.
When I realized that the host returned by the proxy was including the namespace of the skupper site (instead of the namespace where the statefulset was actually deployed to).
After this, I generated a new docker image for the sake of this test, and now it's working properly.
In my last PR to expose deployments/statefulset cross-namespace I missed this particular point.

Copy link
Member

@nluaces nluaces left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@iagodvsantos iagodvsantos force-pushed the stateful-cross-namespace-fix branch from c2d988c to 7d7d5d9 Compare July 24, 2023 13:57
@fgiorgetti fgiorgetti self-requested a review October 24, 2023 20:16
Copy link
Member

@fgiorgetti fgiorgetti left a comment

Choose a reason for hiding this comment

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

LGTM

@nluaces
Copy link
Member

nluaces commented Oct 26, 2023

Hi @iagodvsantos could you rebase from the last changes from main? There are a few improvements related to CI. Thanks!

@shinji62
Copy link

shinji62 commented Jan 30, 2024

@nluaces @iagodvsantos
Just got hit by this issue, I was expected the service to be in the namespace where the pod are deployed, instead they refer to the namespace where the site(linked) is...

for example
ns-a and ns-b are link
poda are deployed in ns-a
I am expecting to resollve poda.ns-a..... from ns-b podb, but instead I can only resolve poda.ns-b.

@iagodvsantos
Copy link
Contributor Author

I'll rebase the PR. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants