Change resource name to store if duplicate found #2230
Conversation
concaf
force-pushed
the
concaf/fix/mappings-same-name
branch
from
68b5810 to
966d59a
Compare
Currently, resources stored in Ambassador are stored as
{resource_name: resource object, ...} treating resource name as
a unique identifier to look up resources of a given kind. However,
this causes issues becuase this does not allow different resources
in different namespace to have the same name, hence resulting in
errors or ignoring these resources completely.
In this commit, if two (or more) resources have the same name (in
different namespaces), then the duplicate resources are stored
as {name.namespace: resource} instead of just {name: resource}.
An ideal solution would be to idenitfy all resources with
{
name: resource_name,
namespace: resource_namespace
}
instead of flat strings, but that comes later.
concaf
force-pushed
the
concaf/fix/mappings-same-name
branch
from
7f4d41c to
8e4aee3
Compare
concaf
changed the title
concaf
force-pushed
the
concaf/fix/mappings-same-name
branch
from
8e4aee3 to
8fad8d3
Compare
|
This PR now also fixes updating multiple Ingress' status if they happen to have the same same in different namespaces. |
|
Fixes #2226 |
| # namespaces. Our current data structure to store resources is a flat string. Till we move to | ||
| # identifying resources with both, name and namespace, we change names of any subsequent resources with | ||
| # the same name here. | ||
| resource.name = f'{resource.name}.{resource.namespace}' |
There was a problem hiding this comment.
Is there a reason why changing the resource name done in an else block? It currently feels like we handle an edge-case, yet I'm pretty sure all resource.name could include the namespace from the moment they are handled.
Following the logic here, it seems given resources: {name: something, namespace: a} and {name: something, namespace: b}, we would end up with 2 resources named: something and something.b where we don't associate the namespace to the first resource we put in storage.
There was a problem hiding this comment.
You're correct. This is what I tried to explain at #2230 (comment) that we need to identify all resources with both name and namespace, and that's what I tried to do in the first go - but it turned out to be a much more intrusive change for the following reasons -
- there were test failures everywhere
- there are parts of code that rely on the resource name coming from aconf.config instead of aconf.config['name'], so that approach broke it
So, yes, this is more or less handling of an edge case without breaking the status quo.
There was a problem hiding this comment.
We'll definitely need to handle the larger case, but, yeah, got it.
|
@concaf tests LGTM:
|
| # namespaces. Our current data structure to store resources is a flat string. Till we move to | ||
| # identifying resources with both, name and namespace, we change names of any subsequent resources with | ||
| # the same name here. | ||
| resource.name = f'{resource.name}.{resource.namespace}' |
There was a problem hiding this comment.
We'll definitely need to handle the larger case, but, yeah, got it.
* update director.proto * run `make generate` * move the snapshot stuff around * agent should use raw snapshot * lint fixup * oops actually replace the secret * small proto change and tests for Snapshot.Sanitize * lots o tests * MOAR COMMENTS
Fix #2226 #2198