docs(MADR): resource identifier format

[lobkovilya]

Motivation

The goal is to improve Inspect API and introduce an identifier as part of the URL path, i.e :5681/_rules/<identifier>. See the discussion

Better to review the rendered version as it contains tables.

Fix #12044

[github-actions]

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

• Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
• PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
• This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
• IPv6 is taken into account (.e.g: no string concatenation of host port)
• Tests (Unit test, E2E tests, manual test on universal and k8s)
  • Don't forget ci/ labels to run additional/fewer tests
• Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
• Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

[Automaat]

what about matches section in new inbound policies api? We might not have a route as a resource but create route from policy

[lobkovilya]

Even for outbound routes it says Correlates with a set of MeshHTTPRoutes. So it's not really clear how to name the route without hashing

[lahabana]

Ho that's quite an important issue no? AFAIR there's no "merging" in routes no? Can't we use the child route that matches this branch with the sectionName?

[lahabana]

Also routes are specific to a listener IIRC no?

[lobkovilya]

There is merging in routes, but I think we're still good and we can name routes in envoy by the name of the most specific MeshHTTPRoute (I guess you were referring to it as "child route"?).

For example, there are producer and consumer routes:

kind: MeshHTTPRoute
metadata:
  name: producer-route
  namespace: backend-ns
spec:
  to:
    - targetRef:
        kind: MeshService
        name: backend
      rules:
        - matches:
            - path:
                type: PathPrefix
                value: "/api"
          default: $conf1
        - matches:
            - path:
                type: PathPrefix
                value: "/gui"
          default: $conf2
---
kind: MeshHTTPRoute
metadata:
  name: consumer-route
  namespace: frontend-ns
spec:
  to:
    - targetRef:
        kind: MeshService
        name: backend
        namespace: backend-ns
      rules:
        - matches:
            - path:
                type: PathPrefix
                value: "/api"
          default: $conf3
        - matches:
            - path:
                type: PathPrefix
                value: "/path-of-consumers-interest"
          default: $conf4

In consumer's Envoy they're going to result in:

routes:
  - name: kri_default_us-east-2_frontend-ns_consumer-route
    match:
      path: "/api"
    route: merge([$conf1, $conf3]).to_envoy_route()
  - name: kri_default_us-east-2_frontend-ns_consumer-route
    match:
      path: "/path-of-consumers-interest"
    route: merge([$conf4]).to_envoy_route()
  - name: kri_default_us-west-2_backend-ns_producer-route
    match:
      path: "/gui"
    route: merge([$conf2]).to_envoy_route()

Also routes are specific to a listener IIRC no?

Yes, routes are specific to a listener and routes[].names don't have to be unique.

I'm going to update the table.

[Automaat]

did we considered having some placeholder for missing values? to avoid multiple _ which can be hard to read?

[lahabana]

You need to be explicit about resource-type. Is it the plural/camlCase...?

[lahabana]

Why not have the resource type before the mesh or at least before the zone?

[lobkovilya]

You need to be explicit about resource-type. Is it the plural/camlCase...?

It should be a lowercased singular name as we use in kumactl, i.e. meshservice or meshtimeout.

Why not have the resource type before the mesh or at least before the zone?

I kind of like how <resource-type> is standing next to <resource-name>, i.e. kri_default___meshservice_backend. Type and name are always present and I think it's easier to catch what identifier is referring to. Compare with kri_meshservice_default__backend. You might think for a sec the meshservice is called default

[lobkovilya]

did we considered having some placeholder for missing values? to avoid multiple _ which can be hard to read?

what would you use as a placeholder?

[johncowen]

Why not have the resource type before the mesh or at least before the zone?

I kinda agree with is, if the resource-type is before anything else, then you can immediately tell whether to expect say an empty mesh in the case of the kri pointing to a resource type that doesn't have a mesh

[lahabana]

Makes sense for the ordering

[lobkovilya]

I kinda agree with is, if the resource-type is before anything else, then you can immediately tell whether to expect say an empty mesh in the case of the kri pointing to a resource type that doesn't have a mesh

Yeah, probably you're right, it's better to have the first parameter that can't be empty, i.e. kri_zoneingress__us-east-1_kuma-system_zi-1. I'm going to change the order then to

kri_<resource-type>_<mesh>_<zone>_<namespace>_<resource-name>_<section-name>

[lahabana]

Even more when there are multiple IPs right?

[lahabana]

Great stuff

[lahabana]

Even in length?

[lobkovilya]

Envoy doesn't specify a length limit. I tried to create a cluster with the max expected length of resource identifier

253(name) + 253(zone) + 63(mesh) + 63(namespace) + 15(sectionName) + 30(resourcetype) + 3(kri) + 6(_) = 686

and it worked as expected.

[lahabana]

And I think there's no lims for prom label length either: https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

[lahabana]

otel has a length limit defined in the SDK api: https://opentelemetry.io/docs/specs/otel/common/#attribute-limits

However, it states the default in infinity.
We should add these in this MADR for reference.

[lahabana]

You need to be explicit about resource-type. Is it the plural/camlCase...?

[lahabana]

Why not have the resource type before the mesh or at least before the zone?

[schogges]

Could it be that the resource identifier is also being used in URL search query, i.e. for filtering?

[lobkovilya]

As it's not a hard requirement, I added as a note what resource identifier and delimiter charset would look like if we wanted to support URL query:

resource-identifier = *(ALPHA / DIGIT / "-" / "." / "_" / "~" )
delimiter           = "_" / "~" 

they're significantly smaller than those without query support.

But the good news is if we go with _ then the resource identifier can be used in a query. So I added this to the Pros list

[schogges]

Sounds good, thank you 🙂

[johncowen]

Maybe I misunderstood the conversation here, but would it be the case that if we were using these in a URL anywhere we would URL encode them first anyway? i.e. any non-URL safe chars would be %ified?

[johncowen]

(P.S. but one small benefit of not having non-URL safe characters is that it keeps the identifier "pretty" i.e. more recognisable, so a benefit but not super important I would say)

actually I guess if the one of the primary usecases is for people to type these to get things (rather than usage within the GUI), we don't want them having to URL encode things manually.

kinda swung back and forth on opinion here, sorry for the noise! 😅 please ignore me!

[schogges]

Still good point @johncowen, but as you said I also think generally it'd be better to not use any non-URL safe chars.

Also depending on what people/tools are using, there might be a difference on which chars are encoded (i.e. encodeURI vs encodeURIComponent):

encodeURI("http://localhost:1234?filter[foo&bar]=baz") // -> 'http://localhost:1234?filter%5Bfoo&bar%5D=baz'

encodeURIComponent("filter[foo&bar]=baz") // -> 'filter%5Bfoo%26bar%5D%3Dbaz'

encodeURI

encodeURI() escapes all characters except:

A–Z a–z 0–9 - _ . ! ~ * ' ( )

; / ? : @ & = + $ , #

The characters on the second line are characters that may be part of the URI syntax, and are only escaped by encodeURIComponent(). Both encodeURI() and encodeURIComponent() do not encode the characters -.!~*'(), known as "unreserved marks", which do not have a reserved purpose but are allowed in a URI "as is". (See RFC2396)

encodeURIComponent

encodeURIComponent() uses the same encoding algorithm as described in encodeURI(). It escapes all characters except:

A–Z a–z 0–9 - _ . ! ~ * ' ( )

Compared to encodeURI(), encodeURIComponent() escapes a larger set of characters. Use encodeURIComponent() on user-entered fields from forms POST'd to the server — this will encode & symbols that may inadvertently be generated during data entry for character references or other characters that require encoding/decoding. For example, if a user writes Jack & Jill, without encodeURIComponent(), the ampersand could be interpreted on the server as the start of a new field and jeopardize the integrity of the data.

[johncowen]

Yeah gotcha, couple of follow ups:

I'd say people should always use encodeURIComponent otherwise they are "holding it wrong" and should change implementation, and I suppose this is my point on expecting people to do always do this how we expect/correctly, there will always be cases where some folks might be "holding it wrong". To be fair, all "values" used in a URL should be using encodeURIComponent (or non-JS equivalent) anyway, whether they are expected to be safe or not. So there's also something to be said for not worrying about URL safety, if someone isn't using a correct encoder, they should be. But I'm thinking more about informal "I just want to curl the thing to get a response in my terminal" type of usage. There's benefit in not forcing people to have to encode the thing if for example we chose to use / in this case.

So if we want URL safe if only for reasons of "don't make it hard for people to just curl the thing" all in all according to the MADR, that leaves us with:

delimiter = "_" / "~"

And it sounds like we've landed on a _, which is URL safe which is super duper. It's probably a good idea to note that I have seen instances of people using these in hostnames even though a _ shouldn't be used in hostnames.

@lobkovilya I'm not sure if we validate things like mesh names and zone names to not have _, I might be misremembering but do I remember that at least at one point this was possible? Is it definitely not possible to have a mesh/zone name with a _ in it now?

Just a little side note that's just occurred to me, I'm kinda glad we still have this last "safe character" available ~, which in a past life has been super useful to have as a usable/meaningful character (i.e. similar to ~/johncowen), which kinda means "expand ~ to a common string we know about". You never know we might hit a thing at somepoint where we need the same "trick".

[lahabana]

I'm not sure if we validate things like mesh names and zone names to not have _, I might be misremembering but do I remember that at least at one point this was possible? Is it definitely not possible to have a mesh/zone name with a _ in it now?

I believe it's no longer supported as this was needed for correct k8s support. But @lobkovilya should check/confirm. I think @jijiechen just removed the back compat in fact

[jijiechen]

I don't remember in which PR that I removed this compatibility. I've just tested on 2.9.3 and confirmed that we now have this validation:

When I was trying to create a Mesh object with name default_2, I got this error:

Error: Could not process a resource (Resource is not valid)
* name: invalid characters. A lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character

[johncowen]

Maybe not part of the MADR and might just be an informal example, but!

The goal is to improve Inspect API and introduce an identifier as part of the URL path, i.e :5681/_rules/. See #12713 (comment)

When we eventually come to define the endpoint should we include the fact that we are specifically requesting via a kri i.e. :5681/_rules/kri/<identifier> or :5681/kri/_rules/<identifier> or :5681/_rules/<identifier>/kri, basically include kri in another segment. That way if we ever add another way/identifier to request things we can distinguish via normal URL routing rather than having a single route check for the type of identifier.

Super edge case, but who knows if we are ever gonna change the way we define identifiers. but maybe I'm over thinking it 🤷

[lahabana]

That'd be the kri no?

[lahabana]

So what does this actually look like?

[lahabana]

This is good. I think the only gap for getting an approval are:

1. showing actual new names in your table once the whole migration is done
2. telling the future of stat_prefix/alt_stat_name

The rest are mostly small questions/nits

[lahabana]

This list is interesting. But should you actually show what the names would become if we were using kri everywhere?

[lahabana]

In the past there were stuff in the metric name, I remember we even had some code to change things around. My is that this is the main reason no?

Also when not using prom format doesn't it actually use : as a separator see:

[lobkovilya]

In the past there were stuff in the metric name, I remember we even had some code to change things around. My is that this is the main reason no?

Yeah maybe, but then it's still unclear, why don't they replace : in stat_prefix? IMO it's just odd that stat_prefix and alt_stat_name don't have consistent behavior.

Also when not using prom format doesn't it actually use : as a separator see:

Even though envoy uses : as a separator in /clusters endpoint, it still takes cluster.name, not cluster.alt_stat_name:

The cluster in the example named kuma:readiness

[lahabana]

A use case could be that stat_prefix and alt_stat_name parameters can be used to consolidate statistics from multiple similar entities under a single name.

For example, if you have separate IPv4 and IPv6 clusters named my-cluster:v4 and my-cluster:v6, you can use alt_stat_name: my-cluster for both clusters to aggregate their statistics under the common name my-cluster when exporting metrics. This allows you to track related entities as a unified group while maintaining distinct names for configuration purposes.

WDYT?

[lobkovilya]

Ah no, I don't question why there are an actual name and stats name, I just don't understand why stat_prefix doesn't behave the same way as alt_stat_name.

[lahabana]

Whatever the reason for this, with our new format, do we end up without the need for specifying stat_prefix and alt_stat_name ? If that's the case that's probably something we should mention and decide whether we still set it (in case) or we don't

[lahabana]

Ho yeah? Where is this defined/validated?

[lobkovilya]

Found it in Kubernetes API reference for ContainerPort:

name – If specified, this must be an IANA_SVC_NAME and unique within the pod. Each named port in a pod must have a unique name. Name for the port that can be referred to by services.

And IANA_SVC_NAME is specified in RFC 6335:

5.1. Service Name Syntax

Valid service names are hereby normatively defined as follows:

o MUST be at least 1 character and no more than 15 characters long

o MUST contain only US-ASCII [ANSI.X3.4-1986] letters 'A' - 'Z' and
'a' - 'z', digits '0' - '9', and hyphens ('-', ASCII 0x2D or
decimal 45)

o MUST contain at least one letter ('A' - 'Z' or 'a' - 'z')

o MUST NOT begin or end with a hyphen

o hyphens MUST NOT be adjacent to other hyphens

The reason for requiring at least one letter is to avoid service
names like "23" (could be confused with a numeric port) or "6000-
6063" (could be confused with a numeric port range). Although
service names may contain both upper-case and lower-case letters,
case is ignored for comparison purposes, so both "http" and "HTTP"
denote the same service.

I checked, and it's indeed validated by the Kubernetes. I'll open an issue so we also validate this on our side

upd: interesting that it doesn't allow names like "8080", at least one letter is required. But we don't have to make sectionName as strict, especially given the fact port's name on Service has a different requirement:

name – The name of this port within the service. This must be a DNS_LABEL. All ports within a ServiceSpec must have unique names. When considering the endpoints for a Service, this must match the 'name' field in the EndpointPort. Optional if only one ServicePort is defined on this service.

So we actually can't use 15 char limit on the sectionName. I'll update the doc

[johncowen]

o MUST contain at least one letter ('A' - 'Z' or 'a' - 'z')

upd: interesting that it doesn't allow names like "8080", at least one letter is required. But we don't have to make sectionName as strict, especially given the fact port's name on Service has a different requirement:

wow! yes very interesting! Nice spot

Optional if only one ServicePort is defined on this service.

I think this ^ is interesting also, i.e. required if more than one

[lahabana]

Makes sense for the ordering

[johncowen]

I just had a thought/question which is maybe just for my understanding, but how come something like SPIFFE IDs aren't an option here? I think that even from my level of understanding, I guess the /'s etc in SPIFFE URIs are problematic for all the reasons why we've landed on a _? Are there more reasons?

[lahabana]

I just had a thought/question which is maybe just for my understanding, but how come something like SPIFFE IDs aren't an option here? I think that even from my level of understanding, I guess the /'s etc in SPIFFE URIs are problematic for all the reasons why we've landed on a _? Are there more reasons?

Spiffe Id only exist for Dataplane identity.

[jijiechen]

This format is definitely more readable by human.

[jijiechen]

This MADR is so well authored.
I approve it with the preference of option 1 as it's more short and most usage scenarios are in intermediate software layers instead of by human.