From 949dc9d46be2b7f8e8ec85d5edd6e2a9bb5b06f3 Mon Sep 17 00:00:00 2001 From: Jonathon Anderson Date: Sat, 12 Jul 2025 11:43:59 -0600 Subject: [PATCH] Restore idempotency of PUT /api/nodes/{id} HTTP PUT is supposed to be idempotent, so PUT for an existing node or profile should replace the node or profile, not throw an error. Generating an error on duplicate can now be requested using the If-None-Match header with a `*` value. Signed-off-by: Jonathon Anderson --- CHANGELOG.md | 5 ++ internal/pkg/warewulfd/api/node.go | 12 ++-- internal/pkg/warewulfd/api/node_test.go | 82 +++++++++++++++++++++- internal/pkg/warewulfd/api/profile.go | 10 ++- internal/pkg/warewulfd/api/profile_test.go | 47 ++++++++++++- 5 files changed, 147 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3b288d0..bf11d2f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Added - Added system and runtime overlay built time to REST API. +- Support If-None-Match header in `PUT /api/nodes/{id}` + +### Changed + +- Restore default idempotency of `PUT /api/nodes/{id}` ### Changed diff --git a/internal/pkg/warewulfd/api/node.go b/internal/pkg/warewulfd/api/node.go index f596e349..b0b14d52 100644 --- a/internal/pkg/warewulfd/api/node.go +++ b/internal/pkg/warewulfd/api/node.go @@ -170,18 +170,20 @@ func getNodeFields() usecase.Interactor { func addNode() usecase.Interactor { type addNodeInput struct { - ID string `path:"id" required:"true" description:"ID of node to be added"` - Node node.Node `json:"node" required:"true" description:"Field values in JSON format for added node"` + ID string `path:"id" required:"true" description:"ID of node to be added"` + Node node.Node `json:"node" required:"true" description:"Field values in JSON format for added node"` + IfNoneMatch string `header:"If-None-Match" description:"Set to '*' to indicate that the node should only be created if it does not already exist"` } u := usecase.NewInteractor(func(ctx context.Context, input addNodeInput, output *node.Node) error { wwlog.Debug("api.addNode(ID:%v, Node:%+v)", input.ID, input.Node) - // registry is the warewulf node "db" yaml file. if registry, err := node.New(); err != nil { return err } else { - if _, ok := registry.Nodes[input.ID]; ok { - return status.Wrap(fmt.Errorf("node name '%s' already exists", input.ID), status.InvalidArgument) + if input.IfNoneMatch == "*" { + if _, ok := registry.Nodes[input.ID]; ok { + return status.Wrap(fmt.Errorf("node '%s' already exists", input.ID), status.InvalidArgument) + } } for _, profile := range input.Node.Profiles { if _, ok := registry.NodeProfiles[profile]; !ok { diff --git a/internal/pkg/warewulfd/api/node_test.go b/internal/pkg/warewulfd/api/node_test.go index 8c2daa9f..05732708 100644 --- a/internal/pkg/warewulfd/api/node_test.go +++ b/internal/pkg/warewulfd/api/node_test.go @@ -74,6 +74,60 @@ func TestNodeAPI(t *testing.T) { assert.JSONEq(t, `{"node1": {}, "test": {"system overlay": ["so1"], "runtime overlay": ["ro1"], "kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}}`, string(body)) }) + t.Run("test idempotency (put same node again)", func(t *testing.T) { + // prepareration + + testNode := `{ + "node":{ + "system overlay": ["so1"], + "runtime overlay": ["ro1"], + "kernel": { + "version": "v1.0.0", + "args": ["kernel-args"] + } + } +}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/nodes/test", bytes.NewBuffer([]byte(testNode))) + assert.NoError(t, err) + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + + assert.JSONEq(t, `{"system overlay": ["so1"], "runtime overlay": ["ro1"], "kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) + }) + + t.Run("fail if node already exists (given appropriate header)", func(t *testing.T) { + // prepareration + + testNode := `{ + "node":{ + "system overlay": ["so1"], + "runtime overlay": ["ro1"], + "kernel": { + "version": "v1.0.0", + "args": ["kernel-args"] + } + } +}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/nodes/test", bytes.NewBuffer([]byte(testNode))) + assert.NoError(t, err) + req.Header.Set("If-None-Match", "*") + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Contains(t, string(body), "node 'test' already exists") + }) + t.Run("get one specific node", func(t *testing.T) { req, err := http.NewRequest(http.MethodGet, srv.URL+"/api/nodes/test", nil) assert.NoError(t, err) @@ -228,6 +282,32 @@ func TestNodeAPI(t *testing.T) { } }) + t.Run("replace a node", func(t *testing.T) { + // prepareration + + testNode := `{ + "node":{ + "system overlay": ["so1"], + "runtime overlay": ["ro1"], + "kernel": { + "version": "v1.0.0", + "args": ["kernel-args"] + } + } +}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/nodes/test", bytes.NewBuffer([]byte(testNode))) + assert.NoError(t, err) + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + + assert.JSONEq(t, `{"system overlay": ["so1"], "runtime overlay": ["ro1"], "kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) + }) + t.Run("test delete nodes", func(t *testing.T) { req, err := http.NewRequest(http.MethodDelete, srv.URL+"/api/nodes/test", nil) assert.NoError(t, err) @@ -239,6 +319,6 @@ func TestNodeAPI(t *testing.T) { assert.NoError(t, err) assert.NoError(t, resp.Body.Close()) - assert.JSONEq(t, `{"system overlay": ["so1"], "runtime overlay": ["ro1"], "kernel": {"version": "v1.0.1-newversion", "args": ["kernel-args"]}}`, string(body)) + assert.JSONEq(t, `{"system overlay": ["so1"], "runtime overlay": ["ro1"], "kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) }) } diff --git a/internal/pkg/warewulfd/api/profile.go b/internal/pkg/warewulfd/api/profile.go index a62a9a03..7e8f125a 100644 --- a/internal/pkg/warewulfd/api/profile.go +++ b/internal/pkg/warewulfd/api/profile.go @@ -57,8 +57,9 @@ func getProfileByID() usecase.Interactor { func addProfile() usecase.Interactor { type addProfileInput struct { - ID string `path:"id" required:"true" description:"ID of profile to add"` - Profile node.Profile `json:"profile" required:"true" description:"Field values in JSON format for added profile"` + ID string `path:"id" required:"true" description:"ID of profile to add"` + Profile node.Profile `json:"profile" required:"true" description:"Field values in JSON format for added profile"` + IfNoneMatch string `header:"If-None-Match" description:"Set to '*' to indicate that the profile should only be created if it does not already exist"` } u := usecase.NewInteractor(func(ctx context.Context, input addProfileInput, output *node.Profile) error { @@ -66,6 +67,11 @@ func addProfile() usecase.Interactor { if registry, err := node.New(); err != nil { return err } else { + if input.IfNoneMatch == "*" { + if _, ok := registry.NodeProfiles[input.ID]; ok { + return status.Wrap(fmt.Errorf("profile '%s' already exists", input.ID), status.InvalidArgument) + } + } for _, profile := range input.Profile.Profiles { if _, ok := registry.NodeProfiles[profile]; !ok { return status.Wrap(fmt.Errorf("profile '%s' does not exist", profile), status.InvalidArgument) diff --git a/internal/pkg/warewulfd/api/profile_test.go b/internal/pkg/warewulfd/api/profile_test.go index 68e2754e..d0882f53 100644 --- a/internal/pkg/warewulfd/api/profile_test.go +++ b/internal/pkg/warewulfd/api/profile_test.go @@ -54,6 +54,37 @@ func TestProfileAPI(t *testing.T) { assert.JSONEq(t, `{"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) }) + t.Run("test idempotency", func(t *testing.T) { + testProfile := `{"profile": {"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/profiles/test", bytes.NewBuffer([]byte(testProfile))) + assert.NoError(t, err) + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, resp.Body.Close()) + assert.NoError(t, err) + assert.JSONEq(t, `{"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) + }) + + t.Run("fail if profile already exists (given appropriate header)", func(t *testing.T) { + testProfile := `{"profile": {"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/profiles/test", bytes.NewBuffer([]byte(testProfile))) + assert.NoError(t, err) + req.Header.Set("If-None-Match", "*") + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, resp.Body.Close()) + assert.NoError(t, err) + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Contains(t, string(body), "profile 'test' already exists") + }) + t.Run("re-read all profiles", func(t *testing.T) { req, err := http.NewRequest(http.MethodGet, srv.URL+"/api/profiles", nil) assert.NoError(t, err) @@ -107,6 +138,20 @@ func TestProfileAPI(t *testing.T) { assert.JSONEq(t, `{"kernel": {"version": "v1.0.1-newversion", "args": ["kernel-args"]}}`, string(body)) }) + t.Run("test overwrite", func(t *testing.T) { + testProfile := `{"profile": {"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}}` + req, err := http.NewRequest(http.MethodPut, srv.URL+"/api/profiles/test", bytes.NewBuffer([]byte(testProfile))) + assert.NoError(t, err) + + resp, err := http.DefaultTransport.RoundTrip(req) + assert.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + assert.NoError(t, resp.Body.Close()) + assert.NoError(t, err) + assert.JSONEq(t, `{"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) + }) + t.Run("test delete a profile", func(t *testing.T) { req, err := http.NewRequest(http.MethodDelete, srv.URL+"/api/profiles/test", nil) assert.NoError(t, err) @@ -117,6 +162,6 @@ func TestProfileAPI(t *testing.T) { body, err := io.ReadAll(resp.Body) assert.NoError(t, resp.Body.Close()) assert.NoError(t, err) - assert.JSONEq(t, `{"kernel": {"version": "v1.0.1-newversion", "args": ["kernel-args"]}}`, string(body)) + assert.JSONEq(t, `{"kernel": {"version": "v1.0.0", "args": ["kernel-args"]}}`, string(body)) }) }