Skip to content

Commit

Permalink
fix(api): compute labels on resource update (#11861)
Browse files Browse the repository at this point in the history
## Motivation

While writing a POC for `MeshExternalService` through a specific zone, I
noticed that the resource I used did not have a zone label. During
debugging, I found that the resource was initially created correctly in
one test, and in a subsequent test, we updated it without actually
modifying any fields. However, after this second call, the updated
resource was missing labels, while the original resource retained them.
After fixing one thing I've noticed that MeshService is missing
`kuma.io/zone` label and that caused that policy wasn't matched and had
to add it.

Create
<img width="572" alt="Pasted Graphic"
src="https://github.com/user-attachments/assets/5ff25fdb-b937-4e9e-a867-ed578c948f48">
Update
<img width="551" alt="protocol http"
src="https://github.com/user-attachments/assets/181da552-d63f-4b1f-b5ea-c23fbdf04c6c">


## Implementation information

I added a ComputeLabels call during the update, as previously we were
simply overriding all labels with those from the resource, without
recalculating them. I added a `kuma.io/zone` label on MeshService
generated on universal.

## Supporting documentation

<!-- Is there a MADR? An Issue? A related PR? -->

Fix #XX

This issue needs to be addressed at a broader level, as it requires a
design. This should be covered by [issue
#11061](#11061).

---------

Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>
  • Loading branch information
lukidzi authored Oct 30, 2024
1 parent 040318e commit ed41f00
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 2 deletions.
19 changes: 17 additions & 2 deletions pkg/api-server/resource_endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func (r *resourceEndpoints) createOrUpdateResource(request *restful.Request, res
if create {
r.createResource(request.Request.Context(), name, meshName, resourceRest, response)
} else {
r.updateResource(request.Request.Context(), resource, resourceRest, response)
r.updateResource(request.Request.Context(), resource, resourceRest, response, meshName)
}
}

Expand Down Expand Up @@ -433,6 +433,7 @@ func (r *resourceEndpoints) updateResource(
currentRes model.Resource,
newResRest rest.Resource,
response *restful.Response,
meshName string,
) {
if err := r.resourceAccess.ValidateUpdate(
ctx,
Expand All @@ -450,8 +451,22 @@ func (r *resourceEndpoints) updateResource(
if r.descriptor.HasStatus { // todo(jakubdyszkiewicz) should we always override this?
_ = currentRes.SetStatus(newResRest.GetStatus())
}
labels, err := model.ComputeLabels(
currentRes.Descriptor(),
currentRes.GetSpec(),
newResRest.GetMeta().GetLabels(),
model.GetNamespace(newResRest.GetMeta(), r.systemNamespace),
meshName,
r.mode,
r.isK8s,
r.zoneName,
)
if err != nil {
rest_errors.HandleError(ctx, response, err, "Could not compute labels for a resource")
return
}

if err := r.resManager.Update(ctx, currentRes, store.UpdateWithLabels(newResRest.GetMeta().GetLabels())); err != nil {
if err := r.resManager.Update(ctx, currentRes, store.UpdateWithLabels(labels)); err != nil {
rest_errors.HandleError(ctx, response, err, "Could not update a resource")
return
}
Expand Down
65 changes: 65 additions & 0 deletions pkg/api-server/resource_endpoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
mesh_proto "github.com/kumahq/kuma/api/mesh/v1alpha1"
api_server "github.com/kumahq/kuma/pkg/api-server"
core_mesh "github.com/kumahq/kuma/pkg/core/resources/apis/mesh"
meshexternalservice_api "github.com/kumahq/kuma/pkg/core/resources/apis/meshexternalservice/api/v1alpha1"
"github.com/kumahq/kuma/pkg/core/resources/model"
"github.com/kumahq/kuma/pkg/core/resources/model/rest"
"github.com/kumahq/kuma/pkg/core/resources/model/rest/unversioned"
Expand All @@ -27,6 +28,7 @@ import (
"github.com/kumahq/kuma/pkg/test/matchers"
test_metrics "github.com/kumahq/kuma/pkg/test/metrics"
"github.com/kumahq/kuma/pkg/test/resources/builders"
"github.com/kumahq/kuma/pkg/util/pointer"
)

var _ = Describe("Resource Endpoints", func() {
Expand Down Expand Up @@ -295,4 +297,67 @@ var _ = Describe("Resource Endpoints on Zone, label origin", func() {
mesh_proto.EnvTag: "universal",
}))
})

It("should compute labels on update of the resource", func() {
// given
apiServer, store, stop := createServer(false, false)
defer stop()
createMesh(store)
name := "ext-svc"

// when
res := &rest_v1alpha1.Resource{
ResourceMeta: rest_v1alpha1.ResourceMeta{
Name: name,
Mesh: mesh,
Type: string(meshexternalservice_api.MeshExternalServiceType),
Labels: map[string]string{
"kuma.io/origin": "zone",
},
},
Spec: &meshexternalservice_api.MeshExternalService{
Match: meshexternalservice_api.Match{
Type: pointer.To(meshexternalservice_api.HostnameGeneratorType),
Port: 9000,
Protocol: core_mesh.ProtocolHTTP,
},
Endpoints: []meshexternalservice_api.Endpoint{
{
Address: "192.168.0.1",
Port: meshexternalservice_api.Port(27017),
},
},
},
}
resp, err := put(apiServer.Address(), meshexternalservice_api.MeshExternalServiceResourceTypeDescriptor, name, res)

// then
Expect(err).ToNot(HaveOccurred())
Expect(resp.StatusCode).To(Equal(http.StatusCreated))
// and then
actualMes := meshexternalservice_api.NewMeshExternalServiceResource()
Expect(store.Get(context.Background(), actualMes, core_store.GetByKey(name, mesh))).To(Succeed())
Expect(actualMes.Meta.GetLabels()).To(Equal(map[string]string{
mesh_proto.ResourceOriginLabel: "zone",
mesh_proto.ZoneTag: "default",
mesh_proto.MeshTag: mesh,
mesh_proto.EnvTag: "universal",
}))

// after update it should have computed labels
resp, err = put(apiServer.Address(), meshexternalservice_api.MeshExternalServiceResourceTypeDescriptor, name, res)

// then
Expect(err).ToNot(HaveOccurred())
Expect(resp.StatusCode).To(Equal(http.StatusOK))
// and then
actualMes = meshexternalservice_api.NewMeshExternalServiceResource()
Expect(store.Get(context.Background(), actualMes, core_store.GetByKey(name, mesh))).To(Succeed())
Expect(actualMes.Meta.GetLabels()).To(Equal(map[string]string{
mesh_proto.ResourceOriginLabel: "zone",
mesh_proto.ZoneTag: "default",
mesh_proto.MeshTag: mesh,
mesh_proto.EnvTag: "universal",
}))
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
"name": "dp-1",
"creationTime": "0001-01-01T00:00:00Z",
"modificationTime": "0001-01-01T00:00:00Z",
"labels": {
"kuma.io/env": "universal",
"kuma.io/mesh": "default",
"kuma.io/origin": "zone",
"kuma.io/zone": "default"
},
"networking": {
"address": "10.1.2.1",
"inbound": [
Expand Down
1 change: 1 addition & 0 deletions pkg/core/resources/apis/meshservice/generate/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ func Setup(rt runtime.Runtime) error {
rt.Metrics(),
rt.ResourceManager(),
rt.MeshCache(),
rt.Config().Multizone.Zone.Name,
)
if err != nil {
return err
Expand Down
4 changes: 4 additions & 0 deletions pkg/core/resources/apis/meshservice/generate/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type Generator struct {
metric prometheus.Summary
resManager manager.ResourceManager
meshCache *mesh_cache.Cache
zone string
}

var _ component.Component = &Generator{}
Expand All @@ -51,6 +52,7 @@ func New(
metrics core_metrics.Metrics,
resManager manager.ResourceManager,
meshCache *mesh_cache.Cache,
zone string,
) (*Generator, error) {
metric := prometheus.NewSummary(prometheus.SummaryOpts{
Name: "component_meshservice_generator",
Expand All @@ -67,6 +69,7 @@ func New(
metric: metric,
resManager: resManager,
meshCache: meshCache,
zone: zone,
}, nil
}

Expand Down Expand Up @@ -259,6 +262,7 @@ func (g *Generator) generate(ctx context.Context, mesh string, dataplanes []*cor
mesh_proto.DisplayName: name,
mesh_proto.ManagedByLabel: managedByValue,
mesh_proto.EnvTag: mesh_proto.UniversalEnvironment,
mesh_proto.ZoneTag: g.zone,
mesh_proto.ResourceOriginLabel: string(mesh_proto.ZoneResourceOrigin),
})); err != nil {
log.Error(err, "couldn't create MeshService")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ var _ = Describe("MeshService generator", func() {
metrics,
resManager,
meshCache,
"zone",
)

Expect(err).ToNot(HaveOccurred())
Expand Down

0 comments on commit ed41f00

Please sign in to comment.