Skip to content

Commit b4ee1fc

Browse files
liangwen12yearliangwen12year
authored
feat(maas): add BFF CRUD endpoints for MaaSAuthPolicy resources (opendatahub-io#7007)
* feat(maas): add BFF CRUD endpoints for MaaSAuthPolicy resources Add standalone policy management endpoints mirroring the existing subscription CRUD pattern: list, view, create, update, delete policies, plus a shared form-data endpoint that now includes policies in its response. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * feat(maas): add subscriptions to policy form data endpoint Enhance the subscription-policy-form-data endpoint to return all subscriptions on the cluster. This supports the priority feature by allowing users to see which subscriptions a model is associated with and avoid overlaps when creating policies. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * test(maas): add comprehensive policy handler tests Add integration tests for all policy CRUD endpoints following the existing Ginkgo/Gomega BDD pattern from subscription tests. Tests cover: - CreatePolicyHandler: success (201), duplicate (409), validation (400) - ListPoliciesHandler: success (200) with verification - GetPolicyInfoHandler: success (200), not found (404), empty name (400) - UpdatePolicyHandler: success (200), not found (404), validation (400) - DeletePolicyHandler: success (200), not found (404), empty name (400) - Form data endpoint: includes policies and subscriptions Total: 16 test cases using envtest with real K8s API server. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * fix(maas): Address CodeRabbit review comments for policy CRUD endpoints This commit addresses all CodeRabbit review comments from PR opendatahub-io#7007: 1. Use ReadJSON helper to enforce 1MB request body limit - Updated CreatePolicyHandler to use app.ReadJSON instead of json.NewDecoder - Updated UpdatePolicyHandler to use app.ReadJSON instead of json.NewDecoder - Removed unused "encoding/json" import from policy_handlers.go 2. Surface conversion errors instead of silently dropping policies - Changed ListPolicies to return error immediately when policy conversion fails - Prevents silent data loss from malformed CRDs or conversion bugs 3. Fix tests that pass nil to handlers - Updated GetPolicyInfoHandler test to create minimal App instance instead of nil - Updated DeletePolicyHandler test to create minimal App instance instead of nil - Added necessary imports (log/slog, os) to policy_handlers_test.go 4. Use typed errors instead of string matching for conflict detection - Created repositories/errors.go with ErrAlreadyExists and ErrNotFound sentinels - Updated CreatePolicy to wrap errors with ErrAlreadyExists - Updated DeletePolicy to wrap errors with ErrNotFound - Updated handlers to use errors.Is() instead of strings.Contains() - More robust error handling that won't break if error messages change 5. Merge spec fields instead of overwriting to preserve unknown CRD fields - Fixed updateAuthPolicySpec to get existing spec and merge fields - Preserves any unknown fields in the spec when updating - Prevents data loss from future CRD schema extensions Signed-off-by: Wen Liang <liangwen12year@gmail.com> * fix(maas): wrap sentinel errors in mock policy repository MockPoliciesRepository was returning plain fmt.Errorf strings instead of wrapping ErrAlreadyExists/ErrNotFound, causing handlers to return 500 instead of 409/404 in mock mode. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * docs(maas): add policy CRUD endpoints to OpenAPI spec Add missing path entries for the 6 policy endpoints (list, get, create, update, delete, form-data) and their request/response schemas. Also update SubscriptionFormDataResponse with the new policies and subscriptions fields. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * chore(maas): remove OPENAPI_CHANGES.md Changes are now documented directly in the OpenAPI spec. Signed-off-by: Wen Liang <liangwen12year@gmail.com> * refactor(maas): address Griffin's review comments on policy endpoints - Wrap all policy handler responses in Envelope for consistency - Rename GetSubscriptionFormDataHandler to GetSubscriptionPolicyFormDataHandler - Merge SubscriptionFormDataPath and PolicyFormDataPath into single SubscriptionPolicyFormDataPath under General MaaS routes section - Move policy request/response types to dedicated models/policy.go - Add optional DisplayName and Description to CreatePolicyRequest, UpdatePolicyRequest, and MaaSAuthPolicy model - Update all tests for Envelope responses and new route paths Signed-off-by: Wen Liang <liangwen12year@gmail.com> * fix(maas): address Griffin's second review on policy endpoints - Remove GET /api/v1/new-subscription from OpenAPI (handler already removed) - Move /api/v1/subscription-policy-form-data to "general" tag - Add displayName and description to CreatePolicyRequest and UpdatePolicyRequest in OpenAPI spec - Set openshift.io/display-name and openshift.io/description annotations on MaaSAuthPolicy K8s resources during create and update - Read annotations back in convertUnstructuredToAuthPolicy Signed-off-by: Wen Liang <liangwen12year@gmail.com> * fix(maas): delete displayName/description annotations on empty update When updating a policy with empty displayName or description, delete the corresponding openshift.io/display-name or openshift.io/description annotation from the K8s resource instead of silently keeping the old value. Signed-off-by: Wen Liang <liangwen12year@gmail.com> --------- Signed-off-by: Wen Liang <liangwen12year@gmail.com>
1 parent 33ad8dc commit b4ee1fc

20 files changed

+1543
-74
lines changed

‎packages/maas/bff/internal/api/app.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,17 +135,20 @@ func NewApp(cfg config.EnvConfig, logger *slog.Logger) (*App, error) {
135135

136136
// Decide mock vs real implementations for K8s-backed repositories
137137
var subscriptionsRepo repositories.SubscriptionsRepositoryInterface
138+
var policiesRepo repositories.PoliciesRepositoryInterface
138139
var modelRefsRepo repositories.MaaSModelRefsRepositoryInterface
139140

140141
if cfg.MockK8Client {
141142
subscriptionsRepo = repositories.NewMockSubscriptionsRepository(logger)
143+
policiesRepo = repositories.NewMockPoliciesRepository(logger)
142144
modelRefsRepo = repositories.NewMockMaaSModelRefsRepository(logger)
143145
} else {
144146
subscriptionsRepo = repositories.NewSubscriptionsRepository(logger, k8sFactory, cfg.MaaSSubscriptionNamespace)
147+
policiesRepo = repositories.NewPoliciesRepository(logger, k8sFactory, cfg.MaaSSubscriptionNamespace)
145148
modelRefsRepo = repositories.NewMaaSModelRefsRepository(logger, k8sFactory)
146149
}
147150

148-
repos, err := repositories.NewRepositories(logger, k8sFactory, cfg, subscriptionsRepo, modelRefsRepo)
151+
repos, err := repositories.NewRepositories(logger, k8sFactory, cfg, subscriptionsRepo, policiesRepo, modelRefsRepo)
149152
if err != nil {
150153
return nil, fmt.Errorf("failed to create repositories: %w", err)
151154
}
@@ -192,6 +195,7 @@ func (app *App) Routes() http.Handler {
192195
attachTierHandlers(apiRouter, app)
193196
attachAPIKeyHandlers(apiRouter, app)
194197
attachSubscriptionHandlers(apiRouter, app)
198+
attachPolicyHandlers(apiRouter, app)
195199
attachMaaSModelRefHandlers(apiRouter, app)
196200
apiRouter.GET(constants.ApiPathPrefix+"/models", handlerWithApp(app, ListModelsHandler))
197201
apiRouter.GET(constants.IsMaasAdminPath, handlerWithApp(app, IsMaasAdminHandler))

‎packages/maas/bff/internal/api/app_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ var _ = Describe("Static File serving Test", func() {
2323
envConfig := config.EnvConfig{
2424
StaticAssetsDir: "../../static",
2525
}
26-
repos, err := repositories.NewRepositories(logger, k8Factory, envConfig, nil, nil)
26+
repos, err := repositories.NewRepositories(logger, k8Factory, envConfig, nil, nil, nil)
2727
Expect(err).NotTo(HaveOccurred())
2828
app := &App{
2929
kubernetesClientFactory: k8Factory,

‎packages/maas/bff/internal/api/healthcheck_handler_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import (
1515
)
1616

1717
func TestHealthCheckHandler(t *testing.T) {
18-
repos, err := repositories.NewRepositories(nil, nil, config.EnvConfig{}, nil, nil)
18+
repos, err := repositories.NewRepositories(nil, nil, config.EnvConfig{}, nil, nil, nil)
1919
assert.NoError(t, err)
2020
app := App{config: config.EnvConfig{
2121
Port: 4000,

‎packages/maas/bff/internal/api/namespaces_handler_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ var _ = Describe("TestNamespacesHandler", func() {
2424

2525
BeforeAll(func() {
2626
By("setting up the test app in dev mode")
27-
repos, err := repositories.NewRepositories(logger, k8Factory, envConfig, nil, nil)
27+
repos, err := repositories.NewRepositories(logger, k8Factory, envConfig, nil, nil, nil)
2828
Expect(err).NotTo(HaveOccurred())
2929

3030
testApp = App{
@@ -137,7 +137,7 @@ var _ = Describe("TestNamespacesHandler", func() {
137137
By("setting up the test app in dev mode")
138138
kubernetesMockedTokenClientFactory, err := k8mocks.NewTokenClientFactory(clientset, restConfig, logger)
139139
Expect(err).NotTo(HaveOccurred())
140-
repos, err := repositories.NewRepositories(logger, kubernetesMockedTokenClientFactory, envConfig, nil, nil)
140+
repos, err := repositories.NewRepositories(logger, kubernetesMockedTokenClientFactory, envConfig, nil, nil, nil)
141141
Expect(err).NotTo(HaveOccurred())
142142
testApp = App{
143143
config: config.EnvConfig{DevMode: true},

‎packages/maas/bff/internal/api/policy_handlers.go‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
package api
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"net/http"
7+
"strings"
8+
9+
"github.com/julienschmidt/httprouter"
10+
11+
"github.com/opendatahub-io/maas-library/bff/internal/constants"
12+
"github.com/opendatahub-io/maas-library/bff/internal/models"
13+
"github.com/opendatahub-io/maas-library/bff/internal/repositories"
14+
)
15+
16+
// attachPolicyHandlers registers the policy routes.
17+
func attachPolicyHandlers(apiRouter *httprouter.Router, app *App) {
18+
apiRouter.GET(constants.PolicyListPath, handlerWithApp(app, ListPoliciesHandler))
19+
apiRouter.GET(constants.PolicyViewPath, handlerWithApp(app, GetPolicyInfoHandler))
20+
apiRouter.GET(constants.SubscriptionPolicyFormDataPath, handlerWithApp(app, GetSubscriptionPolicyFormDataHandler))
21+
apiRouter.POST(constants.PolicyCreatePath, handlerWithApp(app, CreatePolicyHandler))
22+
apiRouter.PUT(constants.PolicyUpdatePath, handlerWithApp(app, UpdatePolicyHandler))
23+
apiRouter.DELETE(constants.PolicyDeletePath, handlerWithApp(app, DeletePolicyHandler))
24+
}
25+
26+
// ListPoliciesHandler handles GET /api/v1/all-policies
27+
// K8s calls: GET /k8s/v1/maasauthpolicy
28+
func ListPoliciesHandler(app *App, w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
29+
ctx := r.Context()
30+
31+
policies, err := app.repositories.Policies.ListPolicies(ctx)
32+
if err != nil {
33+
app.serverErrorResponse(w, r, err)
34+
return
35+
}
36+
37+
response := Envelope[[]models.MaaSAuthPolicy, None]{
38+
Data: policies,
39+
}
40+
41+
if err := app.WriteJSON(w, http.StatusOK, response, nil); err != nil {
42+
app.serverErrorResponse(w, r, err)
43+
}
44+
}
45+
46+
// GetPolicyInfoHandler handles GET /api/v1/view-policy/:name
47+
// K8s calls: GET /k8s/v1/maasauthpolicy/:name, GET /k8s/v1/maasmodelref
48+
func GetPolicyInfoHandler(app *App, w http.ResponseWriter, r *http.Request, params httprouter.Params) {
49+
ctx := r.Context()
50+
name := params.ByName("name")
51+
if name == "" {
52+
app.badRequestResponse(w, r, errors.New("policy name is required"))
53+
return
54+
}
55+
56+
policy, err := app.repositories.Policies.GetPolicy(ctx, name)
57+
if err != nil {
58+
app.serverErrorResponse(w, r, err)
59+
return
60+
}
61+
if policy == nil {
62+
app.notFoundResponse(w, r)
63+
return
64+
}
65+
66+
// Convert policy ModelRefs to ModelSubscriptionRefs for the summary lookup
67+
subRefs := make([]models.ModelSubscriptionRef, len(policy.ModelRefs))
68+
for i, mr := range policy.ModelRefs {
69+
subRefs[i] = models.ModelSubscriptionRef{Name: mr.Name, Namespace: mr.Namespace}
70+
}
71+
72+
modelRefSummaries, err := app.repositories.Subscriptions.GetModelRefSummaries(ctx, subRefs)
73+
if err != nil {
74+
app.serverErrorResponse(w, r, err)
75+
return
76+
}
77+
78+
response := Envelope[models.PolicyInfoResponse, None]{
79+
Data: models.PolicyInfoResponse{
80+
Policy: *policy,
81+
ModelRefs: modelRefSummaries,
82+
},
83+
}
84+
85+
if err := app.WriteJSON(w, http.StatusOK, response, nil); err != nil {
86+
app.serverErrorResponse(w, r, err)
87+
}
88+
}
89+
90+
// CreatePolicyHandler handles POST /api/v1/new-policy
91+
// K8s calls: CREATE /k8s/v1/maasauthpolicy
92+
func CreatePolicyHandler(app *App, w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
93+
ctx := r.Context()
94+
95+
var envelope Envelope[models.CreatePolicyRequest, None]
96+
if err := app.ReadJSON(w, r, &envelope); err != nil {
97+
app.badRequestResponse(w, r, err)
98+
return
99+
}
100+
request := envelope.Data
101+
102+
if strings.TrimSpace(request.Name) == "" {
103+
app.badRequestResponse(w, r, errors.New("name is required"))
104+
return
105+
}
106+
if len(request.ModelRefs) == 0 {
107+
app.badRequestResponse(w, r, errors.New("at least one modelRef is required"))
108+
return
109+
}
110+
111+
policy, err := app.repositories.Policies.CreatePolicy(ctx, request)
112+
if err != nil {
113+
if errors.Is(err, repositories.ErrAlreadyExists) {
114+
app.errorResponse(w, r, &HTTPError{
115+
StatusCode: http.StatusConflict,
116+
Error: ErrorPayload{Code: "409", Message: err.Error()},
117+
})
118+
} else {
119+
app.serverErrorResponse(w, r, err)
120+
}
121+
return
122+
}
123+
124+
response := Envelope[*models.MaaSAuthPolicy, None]{
125+
Data: policy,
126+
}
127+
128+
if err := app.WriteJSON(w, http.StatusCreated, response, nil); err != nil {
129+
app.serverErrorResponse(w, r, err)
130+
}
131+
}
132+
133+
// UpdatePolicyHandler handles PUT /api/v1/update-policy/:name
134+
// K8s calls: PUT /k8s/v1/maasauthpolicy/:name
135+
func UpdatePolicyHandler(app *App, w http.ResponseWriter, r *http.Request, params httprouter.Params) {
136+
ctx := r.Context()
137+
name := params.ByName("name")
138+
if name == "" {
139+
app.badRequestResponse(w, r, errors.New("policy name is required"))
140+
return
141+
}
142+
143+
var envelope Envelope[models.UpdatePolicyRequest, None]
144+
if err := app.ReadJSON(w, r, &envelope); err != nil {
145+
app.badRequestResponse(w, r, err)
146+
return
147+
}
148+
request := envelope.Data
149+
150+
if len(request.ModelRefs) == 0 {
151+
app.badRequestResponse(w, r, errors.New("at least one modelRef is required"))
152+
return
153+
}
154+
155+
policy, err := app.repositories.Policies.UpdatePolicy(ctx, name, request)
156+
if err != nil {
157+
app.serverErrorResponse(w, r, err)
158+
return
159+
}
160+
if policy == nil {
161+
app.errorResponse(w, r, &HTTPError{
162+
StatusCode: http.StatusNotFound,
163+
Error: ErrorPayload{Code: "404", Message: fmt.Sprintf("MaaSAuthPolicy '%s' not found", name)},
164+
})
165+
return
166+
}
167+
168+
response := Envelope[*models.MaaSAuthPolicy, None]{
169+
Data: policy,
170+
}
171+
172+
if err := app.WriteJSON(w, http.StatusOK, response, nil); err != nil {
173+
app.serverErrorResponse(w, r, err)
174+
}
175+
}
176+
177+
// DeletePolicyHandler handles DELETE /api/v1/delete-policy/:name
178+
// K8s calls: DELETE /k8s/v1/maasauthpolicy/:name
179+
func DeletePolicyHandler(app *App, w http.ResponseWriter, r *http.Request, params httprouter.Params) {
180+
ctx := r.Context()
181+
name := params.ByName("name")
182+
if name == "" {
183+
app.badRequestResponse(w, r, errors.New("policy name is required"))
184+
return
185+
}
186+
187+
if err := app.repositories.Policies.DeletePolicy(ctx, name); err != nil {
188+
if errors.Is(err, repositories.ErrNotFound) {
189+
app.errorResponse(w, r, &HTTPError{
190+
StatusCode: http.StatusNotFound,
191+
Error: ErrorPayload{Code: "404", Message: err.Error()},
192+
})
193+
} else {
194+
app.serverErrorResponse(w, r, err)
195+
}
196+
return
197+
}
198+
199+
response := Envelope[map[string]string, None]{
200+
Data: map[string]string{
201+
"message": fmt.Sprintf("MaaSAuthPolicy '%s' deleted successfully", name),
202+
},
203+
}
204+
205+
if err := app.WriteJSON(w, http.StatusOK, response, nil); err != nil {
206+
app.serverErrorResponse(w, r, err)
207+
}
208+
}

0 commit comments

Comments
 (0)