endpoint: create endpoint without labels

[oblazek]

When endpoint API is used to create an endpoint and EndpointChangeRequest contains labels, it will not allocate an identity to the endpoint.

During NewEndpointFromChangeModel() labels are stored in the endpoint model, causing the followup call ep.UpdateLabels() to not bump revision during this single createEndpoint() call. This means the folloup call to e.runIdentityResolver() never happens and the endpoint ends up without identity and with state waiting-for-identity.

ENDPOINT   IDENTITY        LABELS (source:key[=value])               IPv4         STATUS

3236       <no label id>   k8s:app=incubator-mynetns3                10.247.1.1   waiting-for-identity
                           k8s:io.cilium.k8s.policy.cluster=default
                           k8s:io.kubernetes.pod.namespace=default

This should not happen, otherwise user can only not set the labels during createEndpoint() call and do a followup call patchEndpoint() where labels will be set which then triggers regeneration to be triggered and identity allocated.

Instead epTemplate.Labels don't need to be set which means the above issue never happens as updateLabels() will now bump revision -> i.e. the regeneration is triggered.

With these changes one can createEndpoint() with labels in a single call:

ENDPOINT   IDENTITY   LABELS (source:key[=value])                IPv4         STATUS

331        21864      k8s:app=incubator-mynetns3                 10.247.1.1   ready
                      k8s:io.cilium.k8s.policy.cluster=default
                      k8s:io.kubernetes.pod.namespace=default

Fixes: #29776

endpoint: fix inability to create endpoint with labels in a single API call

Signed-off-by: Ondrej Blazek ondrej.blazek@firma.seznam.cz

[oblazek]

/test

[oblazek]

/test

[aditighag]

I'm not familiar with this API as I was only aware of the custom health endpoint being created within the agent, so I'm removing my review assignment. /cc @cilium/sig-agent and @christarazi from the linked issue for review.

[squeed]

I'm not sure this is the right solution. Wouldn't it just make more sense to unconditionally start the identity resolver? I don't know why we skip it when rev = 0.

[oblazek]

I'm not sure this is the right solution. Wouldn't it just make more sense to unconditionally start the identity resolver? I don't know why we skip it when rev = 0.

tbh I have no idea, very good point

[oblazek]

when I am looking closely at the runIdentityResolver().. seems to me it can be safely run unconditionaly as https://github.com/cilium/cilium/blob/main/pkg/endpoint/endpoint.go#L2067 will make sure to run the ID allocation only when really needed

[oblazek]

diff --git a/pkg/endpoint/endpoint.go b/pkg/endpoint/endpoint.go
index 616c2020e6..a6e2764abe 100644
--- a/pkg/endpoint/endpoint.go
+++ b/pkg/endpoint/endpoint.go
@@ -1743,11 +1743,7 @@ func (e *Endpoint) UpdateLabels(ctx context.Context, identityLabels, infoLabels
        // replace identity labels and update the identity if labels have changed
        rev := e.replaceIdentityLabels(identityLabels)
        e.unlock()
-       if rev != 0 {
-               return e.runIdentityResolver(ctx, rev, blocking)
-       }
-
-       return false
+       return e.runIdentityResolver(ctx, rev, blocking)
 }

[oblazek]

/test

[github-actions]

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

[oblazek]

/test

[aanm]

Thanks for the PR. Sorry for taking so much time to review this but now I understood the full picture. I think this patch would be more appropriate. We should prevent executing runIdentityResolver unnecessarily.

diff --git a/daemon/cmd/endpoint.go b/daemon/cmd/endpoint.go
index d995eda77f..ed064bec20 100644
--- a/daemon/cmd/endpoint.go
+++ b/daemon/cmd/endpoint.go
@@ -376,6 +376,16 @@ func (d *Daemon) createEndpoint(ctx context.Context, owner regeneration.Owner, e
                "sync-build":                 epTemplate.SyncBuildEndpoint,
        }).Info("Create endpoint request")
 
+       // We don't need to create the endpoint with the labels. This might cause
+       // the endpoint regeneration to not be triggered further down, with the
+       // ep.UpdateLabels or the ep.RunMetadataResolver, because the regeneration
+       // is only triggered in case the labels are changed, which they might not
+       // change because NewEndpointFromChangeModel would contain the
+       // epTemplate.Labels, the same labels we would be calling ep.UpdateLabels or
+       // the ep.RunMetadataResolver.
+       apiLabels := labels.NewLabelsFromModel(epTemplate.Labels)
+       epTemplate.Labels = nil
+
        ep, err := endpoint.NewEndpointFromChangeModel(d.ctx, owner, d, d.ipcache, d.l7Proxy, d.identityAllocator, epTemplate)
        if err != nil {
                return invalidDataError(ep, fmt.Errorf("unable to parse endpoint parameters: %s", err))
@@ -414,7 +424,6 @@ func (d *Daemon) createEndpoint(ctx context.Context, owner regeneration.Owner, e
                return invalidDataError(ep, err)
        }
 
-       apiLabels := labels.NewLabelsFromModel(epTemplate.Labels)
        infoLabels := labels.NewLabelsFromModel([]string{})
 
        if len(apiLabels) > 0 {

[oblazek]

yeah that totally works!!