Services are accessible even if their namespace doesn’t exist locally

[skitt]

What happened:

In a two-cluster setup running Lighthouse (devel), I set up a namespace and service in cluster2, and exported it. The namespace didn’t exist in cluster1, but even so, the exported service was accessible.

What you expected to happen:

Given that the namespace containing the service doesn’t exist in cluster1, the service shouldn’t be imported there.

How to reproduce it (as minimally and precisely as possible):

1. make deploy from Lighthouse or the operator
2. create a namespace in one cluster only
3. create a service there
4. export it
5. check that it can be resolved and accessed from the other cluster

[danibachar]

@skitt - what would be the expected behavior in that case but that the DNS query specify the destination cluster (which is hosting the relevant namespace)?
For example
Service s1 is in cluster cluster1 on namespace ns1
Service s2 is present on cluster cluster2 which does not have namespace ns1 deployed on.
It releases the following DNS query:
cluster1.s1.ns1.svc.clusterset.local

I'm not quite sure why would we even block access? Do you think it is a security risk?

[skitt]

Service s1 is in cluster cluster1 on namespace ns1
Service s2 is present on cluster cluster2 which does not have namespace ns1 deployed on.
It releases the following DNS query: cluster1.s1.ns1.svc.clusterset.local

See the MCS API:

A multi-cluster service will be imported only by clusters in which the service's namespace exists.

So cluster2 shouldn’t import s1, and its DNS server shouldn’t be aware of it, so looking up cluster1.s1.ns1.svc.clusterset.local should fail.

I'm not quite sure why would we even block access? Do you think it is a security risk?

I’m not sure it’s a security risk, but it could be a surprise for the administrator — in cluster2, if the administrator hasn’t created ns1, one wouldn’t expect services in ns1 to be accessible.

[vthapar]

Ideal fix for this issue would be with #214 Namely, we would rely on aggregated import for DNS resolution but we won't aggregate the import if namespace is not present locally.

[donaldh]

@vthapar I think #214 is independent of this issue. #214 is concerned with merging multiple ServiceImports that are allowed to happen per the spec. This issue is concerned with not allowing a ServiceImport when the relevant namespace is not present in the candidate importing cluster.

A related issue: the kep strongly implies that the ServiceImport object should be created in the namespace that the Service was exported from and Submariner currently diverges from this; the ServiceImports are in the submariner-operator namespace. If we were to create ServiceImports in the correct namespace, then this issue could be resolved as part of that work. I am not familiar enough with the code to know whether syncing to multiple namespaces would end up creating sync or permissions problems though.

Another detail from the kep:

An implementation may or may not decide to create missing namespaces automatically, that behavior is out of scope of this spec.

My guess is that it could be a policy / broker configuration option to auto-create namespaces. Enabling it would mean that namespace existence could not be used as a way of controlling which clusters services get imported to.

[skitt]

A related issue: the kep strongly implies that the ServiceImport object should be created in the namespace that the Service was exported from and Submariner currently diverges from this; the ServiceImports are in the submariner-operator namespace. If we were to create ServiceImports in the correct namespace, then this issue could be resolved as part of that work. I am not familiar enough with the code to know whether syncing to multiple namespaces would end up creating sync or permissions problems though.

Yes, creating the ServiceImport objects in the right namespace would fix this, and it would also simplify non-MCS scenarios where services are imported into other namespaces than the namespace of the service itself.

[donaldh]

I have a potential patch to move ServiceImports into the appropriate namespace. Will test to see if any more work is needed.

[vthapar]

@donaldh You're correct that it is a different issue from #214 but our intention was to implement namespace checks when we aggreate service imports. Namespace checks will be implicitly done as part of creating aggregated serviceimport in the service namespace. Whether we decide to auto-create namespace or set ServiceImport status to error, either can be done when aggregating serviceimports. ServiceImports in broker namespace are more a result of way we distribute ServiceImports through broker. But yes, we can have ServiceImports created in apporpriate namespace without aggregation and if you can bring in a patch it would be great.

Note that one of the reasons we deviate from spec is that we don't implement a ClusterSetIP like spec requires.

[donaldh]

@vthapar Is your intention that the ServiceImports would be synchronised to the submariner-operator namespace and then merged into a single ServiceImport in the target namespace?

[vthapar]

@donaldh Yes. Other way would be to aggregate at source in correct namespace,keep only one copy on broker and sync to origin namespace on remotes. Both have pros and cons.

1. Aggregate at source: Easy to catch conflicts and update status on ServiceExport, lesser ServiceImports being synced across clusters. But that might lead to too many conflicts when writing from multiple clusters at same time. Not an issue during normal operation, but will be an issue during restart/upgrades or when deploying on multiple clusters using automation. This mostly means it will take longer to sync everything on startup/upgrade.
2. Aggregate at destination: All aggregation is done locally so no conflicts from multiple clusters writing to same object. But large number of service imports will be distributed across clusters, updating relevant ServiceExport for conflict will require going through list of ServiceExports.

I do think 1 will scale better in longer run with lesser data being synced across clusters.

[stale]

This issue has been automatically marked as stale because it has not had activity for 60 days. It will be closed if no further activity occurs. Please make a comment if this issue/pr is still valid. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had activity for 60 days. It will be closed if no further activity occurs. Please make a comment if this issue/pr is still valid. Thank you for your contributions.

[tpantelis]

The aggregated ServiceImports are now created in the service namespace which inherently fixes this issue. However we should verify the exact behavior in this case to ensure there's no detrimental effect. An E2E test would be ideal.