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

Add destination namespace check in indexer #551

Merged
merged 8 commits into from
May 11, 2020

Conversation

dtomcej
Copy link
Contributor

@dtomcej dtomcej commented May 5, 2020

What does this PR do?

This PR:

  • Adds a check for an empty destination namespace in traffictargets during load
  • Adds test to ensure empty destination namespaces are filled with the namespace of the traffictarget.

Fixes #546

Additional Notes

We may want to look at a "validate resource" method in the future, but I think for now it can be avoided as there is not a lot of validation required.

@dtomcej dtomcej added this to the v1.2 milestone May 5, 2020
@jspdown
Copy link
Contributor

jspdown commented May 6, 2020

@dtomcej Thanks for the PR. Initially I was thinking about doing this in the indexSMIResources. The sooner is the better I think.

@dtomcej dtomcej marked this pull request as ready for review May 6, 2020 19:53
@SantoDE
Copy link

SantoDE commented May 7, 2020

Just a general question first:

I see that this takes an empty namespace as default, which I think is good. However, how do we handle the case if Source or Destination don't have a namespace set. Do we infer the Namespace of the TrafficTarget?

@dtomcej
Copy link
Contributor Author

dtomcej commented May 7, 2020

Good call @SantoDE. I checked the CRD, and both source and destination have namespace as an optional field, however the entire source field itself is optional.

I will assume that no destination namespace defined would result in the default namespace being used, but that could easily be changed in the future. For the time being, we will use default, so that code functions properly.

I have opened an issue: servicemeshinterface/smi-spec#162 that should hopefully answer this question.

@SantoDE
Copy link

SantoDE commented May 8, 2020

Thanks @dtomcej.

Seeing that the issue is mostly answered by Thomas already, I can live with that. We should just document the behaviour tough :-)

@dtomcej
Copy link
Contributor Author

dtomcej commented May 8, 2020

As answered in the SMI spec issue above, we will have the namespace inherited from the TT object itself.

docs/content/configuration.md Outdated Show resolved Hide resolved
pkg/topology/builder.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TrafficTarget deployed in the default namespace does not work
5 participants