Skip to content

Commit c69bbdb

Browse files
committed
Enhance add-access-rule command UX with intuitive name-based flags
This commit improves the user experience for the add-access-rule command by replacing the positional GUID-based SELECTOR argument with intuitive flags that accept human-readable names and support cross-space/org resolution. Changes: **Command Interface:** - Remove positional SELECTOR argument (breaking change, acceptable for unreleased feature) - Add new flags: --source-app, --source-space, --source-org, --source-any, --selector - Support hierarchical name resolution: - --source-app APP_NAME (looks in current space) - --source-app APP_NAME --source-space SPACE (cross-space in current org) - --source-app APP_NAME --source-space SPACE --source-org ORG (cross-org) - --source-space SPACE (space-level rule) - --source-org ORG (org-level rule) - --source-any (allow any authenticated app) - --selector SELECTOR (raw GUID-based selector for advanced users) - Validate exactly one primary source is specified - Display verbose output showing resolved selector for transparency **Terminology Update:** - Rename all "target" terminology to "source" throughout codebase - Access rules specify the source (who can access), not the target - Update AccessRuleWithRoute.TargetName → SourceName - Update resolveAccessRuleTarget() → resolveAccessRuleSource() - Update access-rules list command table header: "target" → "source" **Error Handling:** - Provide helpful error messages when app not found in current space - Suggest using --source-space and --source-org flags for cross-space/org access - Follow CF CLI patterns from add-network-policy command **Testing:** - Add 17 comprehensive test cases for add-access-rule command - Update 19 actor tests to use new SourceName field - All tests passing (36/36) **Domain Integration:** - Add enforce_access_rules support to create-shared-domain and create-private-domain - Add --enforce-access-rules and --access-rules-scope flags - Update domain resource with new fields Examples: # Simple case - app in current space cf add-access-rule allow-frontend apps.identity --source-app frontend-app --hostname backend # Cross-space access cf add-access-rule allow-other apps.identity --source-app api-client --source-space other-space --hostname backend # Cross-org access cf add-access-rule allow-prod apps.identity --source-app client --source-space prod-space --source-org prod-org --hostname api # Space-level rule cf add-access-rule allow-monitoring apps.identity --source-space monitoring --hostname api # Org-level rule cf add-access-rule allow-platform apps.identity --source-org platform --hostname shared-api # Any authenticated app cf add-access-rule allow-all apps.identity --source-any --hostname public-api Related to: cloudfoundry/community#1438
1 parent fcb98f7 commit c69bbdb

28 files changed

Lines changed: 3388 additions & 58 deletions
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package actionerror
2+
3+
import "fmt"
4+
5+
type AccessRuleNotFoundError struct {
6+
Name string
7+
}
8+
9+
func (e AccessRuleNotFoundError) Error() string {
10+
return fmt.Sprintf("Access rule '%s' not found.", e.Name)
11+
}

actor/v7action/access_rule.go

Lines changed: 383 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,383 @@
1+
package v7action
2+
3+
import (
4+
"code.cloudfoundry.org/cli/v9/actor/actionerror"
5+
"code.cloudfoundry.org/cli/v9/api/cloudcontroller/ccv3"
6+
"code.cloudfoundry.org/cli/v9/resources"
7+
)
8+
9+
func (actor Actor) AddAccessRule(ruleName, domainName, selector, hostname, path string) (Warnings, error) {
10+
allWarnings := Warnings{}
11+
12+
// Get the domain to ensure it exists and supports access rules
13+
domain, warnings, err := actor.GetDomainByName(domainName)
14+
allWarnings = append(allWarnings, warnings...)
15+
if err != nil {
16+
return allWarnings, err
17+
}
18+
19+
// Find the route
20+
routes, routeWarnings, err := actor.GetRoutesByDomain(domain.GUID, hostname, path)
21+
allWarnings = append(allWarnings, routeWarnings...)
22+
if err != nil {
23+
return allWarnings, err
24+
}
25+
26+
if len(routes) == 0 {
27+
return allWarnings, actionerror.RouteNotFoundError{
28+
Host: hostname,
29+
DomainName: domainName,
30+
Path: path,
31+
}
32+
}
33+
34+
route := routes[0]
35+
36+
// Create the access rule
37+
accessRule := resources.AccessRule{
38+
Name: ruleName,
39+
Selector: selector,
40+
RouteGUID: route.GUID,
41+
}
42+
43+
_, apiWarnings, err := actor.CloudControllerClient.CreateAccessRule(accessRule)
44+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
45+
46+
return allWarnings, err
47+
}
48+
49+
func (actor Actor) GetAccessRulesByRoute(domainName, hostname, path string) ([]resources.AccessRule, Warnings, error) {
50+
allWarnings := Warnings{}
51+
52+
// Get the domain
53+
domain, warnings, err := actor.GetDomainByName(domainName)
54+
allWarnings = append(allWarnings, warnings...)
55+
if err != nil {
56+
return nil, allWarnings, err
57+
}
58+
59+
// Find the route
60+
routes, routeWarnings, err := actor.GetRoutesByDomain(domain.GUID, hostname, path)
61+
allWarnings = append(allWarnings, routeWarnings...)
62+
if err != nil {
63+
return nil, allWarnings, err
64+
}
65+
66+
if len(routes) == 0 {
67+
return nil, allWarnings, actionerror.RouteNotFoundError{
68+
Host: hostname,
69+
DomainName: domainName,
70+
Path: path,
71+
}
72+
}
73+
74+
route := routes[0]
75+
76+
// Get access rules for this route
77+
accessRules, _, apiWarnings, err := actor.CloudControllerClient.GetAccessRules(
78+
ccv3.Query{Key: ccv3.RouteGUIDFilter, Values: []string{route.GUID}},
79+
)
80+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
81+
82+
var rules []resources.AccessRule
83+
for _, rule := range accessRules {
84+
rules = append(rules, resources.AccessRule(rule))
85+
}
86+
87+
return rules, allWarnings, err
88+
}
89+
90+
func (actor Actor) DeleteAccessRule(ruleName, domainName, hostname, path string) (Warnings, error) {
91+
allWarnings := Warnings{}
92+
93+
// Get the domain
94+
domain, warnings, err := actor.GetDomainByName(domainName)
95+
allWarnings = append(allWarnings, warnings...)
96+
if err != nil {
97+
return allWarnings, err
98+
}
99+
100+
// Find the route
101+
routes, routeWarnings, err := actor.GetRoutesByDomain(domain.GUID, hostname, path)
102+
allWarnings = append(allWarnings, routeWarnings...)
103+
if err != nil {
104+
return allWarnings, err
105+
}
106+
107+
if len(routes) == 0 {
108+
return allWarnings, actionerror.RouteNotFoundError{
109+
Host: hostname,
110+
DomainName: domainName,
111+
Path: path,
112+
}
113+
}
114+
115+
route := routes[0]
116+
117+
// Get access rules for this route to find the one with matching name
118+
accessRules, _, apiWarnings, err := actor.CloudControllerClient.GetAccessRules(
119+
ccv3.Query{Key: ccv3.RouteGUIDFilter, Values: []string{route.GUID}},
120+
)
121+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
122+
if err != nil {
123+
return allWarnings, err
124+
}
125+
126+
// Find the rule with matching name
127+
var ruleGUID string
128+
for _, rule := range accessRules {
129+
if rule.Name == ruleName {
130+
ruleGUID = rule.GUID
131+
break
132+
}
133+
}
134+
135+
if ruleGUID == "" {
136+
return allWarnings, actionerror.AccessRuleNotFoundError{Name: ruleName}
137+
}
138+
139+
// Delete the access rule
140+
_, deleteWarnings, err := actor.CloudControllerClient.DeleteAccessRule(ruleGUID)
141+
allWarnings = append(allWarnings, Warnings(deleteWarnings)...)
142+
143+
return allWarnings, err
144+
}
145+
146+
// GetRoutesByDomain gets routes for a domain with optional hostname and path filters
147+
func (actor Actor) GetRoutesByDomain(domainGUID, hostname, path string) ([]resources.Route, Warnings, error) {
148+
queries := []ccv3.Query{
149+
{Key: ccv3.DomainGUIDFilter, Values: []string{domainGUID}},
150+
}
151+
152+
if hostname != "" {
153+
queries = append(queries, ccv3.Query{Key: ccv3.HostsFilter, Values: []string{hostname}})
154+
}
155+
156+
if path != "" {
157+
queries = append(queries, ccv3.Query{Key: ccv3.PathsFilter, Values: []string{path}})
158+
}
159+
160+
ccv3Routes, warnings, err := actor.CloudControllerClient.GetRoutes(queries...)
161+
if err != nil {
162+
return nil, Warnings(warnings), err
163+
}
164+
165+
var routes []resources.Route
166+
for _, route := range ccv3Routes {
167+
routes = append(routes, resources.Route(route))
168+
}
169+
170+
return routes, Warnings(warnings), nil
171+
}
172+
173+
// AccessRuleWithRoute combines an access rule with its associated route information
174+
type AccessRuleWithRoute struct {
175+
resources.AccessRule
176+
Route resources.Route
177+
DomainName string
178+
ScopeType string // "app", "space", "org", or "any"
179+
SourceName string // Resolved source name (app/space/org) or empty string
180+
}
181+
182+
// GetAccessRulesForSpace gets all access rules for routes in a space with optional filters
183+
func (actor Actor) GetAccessRulesForSpace(
184+
spaceGUID string,
185+
domainName string,
186+
hostname string,
187+
path string,
188+
labelSelector string,
189+
) ([]AccessRuleWithRoute, Warnings, error) {
190+
allWarnings := Warnings{}
191+
192+
// Build query for access rules filtered by space, with included routes
193+
queries := []ccv3.Query{
194+
{Key: ccv3.SpaceGUIDFilter, Values: []string{spaceGUID}},
195+
{Key: ccv3.Include, Values: []string{"route"}},
196+
}
197+
198+
// Add label selector if provided
199+
if labelSelector != "" {
200+
queries = append(queries, ccv3.Query{Key: ccv3.LabelSelectorFilter, Values: []string{labelSelector}})
201+
}
202+
203+
// Fetch access rules directly by space GUID with included routes (single API call)
204+
accessRules, includedResources, apiWarnings, err := actor.CloudControllerClient.GetAccessRules(queries...)
205+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
206+
if err != nil {
207+
return nil, allWarnings, err
208+
}
209+
210+
if len(accessRules) == 0 {
211+
// No access rules found - return empty list, not an error
212+
return []AccessRuleWithRoute{}, allWarnings, nil
213+
}
214+
215+
// Build route lookup map from included resources
216+
routeByGUID := make(map[string]resources.Route)
217+
for _, route := range includedResources.Routes {
218+
routeByGUID[route.GUID] = route
219+
}
220+
221+
// Apply optional filters to the included routes
222+
if domainName != "" {
223+
domain, warnings, err := actor.GetDomainByName(domainName)
224+
allWarnings = append(allWarnings, warnings...)
225+
if err != nil {
226+
return nil, allWarnings, err
227+
}
228+
// Filter routes by domain GUID
229+
filteredRoutes := make(map[string]resources.Route)
230+
for guid, route := range routeByGUID {
231+
if route.DomainGUID == domain.GUID {
232+
filteredRoutes[guid] = route
233+
}
234+
}
235+
routeByGUID = filteredRoutes
236+
}
237+
238+
if hostname != "" {
239+
// Filter routes by hostname
240+
filteredRoutes := make(map[string]resources.Route)
241+
for guid, route := range routeByGUID {
242+
if route.Host == hostname {
243+
filteredRoutes[guid] = route
244+
}
245+
}
246+
routeByGUID = filteredRoutes
247+
}
248+
249+
if path != "" {
250+
// Filter routes by path
251+
filteredRoutes := make(map[string]resources.Route)
252+
for guid, route := range routeByGUID {
253+
if route.Path == path {
254+
filteredRoutes[guid] = route
255+
}
256+
}
257+
routeByGUID = filteredRoutes
258+
}
259+
260+
// Build domain name cache
261+
domainCache := make(map[string]string)
262+
for _, route := range routeByGUID {
263+
if _, exists := domainCache[route.DomainGUID]; !exists {
264+
domain, warnings, err := actor.GetDomain(route.DomainGUID)
265+
allWarnings = append(allWarnings, warnings...)
266+
if err != nil {
267+
// If we can't get the domain, use the GUID as fallback
268+
domainCache[route.DomainGUID] = route.DomainGUID
269+
} else {
270+
domainCache[route.DomainGUID] = domain.Name
271+
}
272+
}
273+
}
274+
275+
// Build results with route information and resolved sources
276+
// Only include access rules whose routes match the filters
277+
var results []AccessRuleWithRoute
278+
for _, rule := range accessRules {
279+
route, exists := routeByGUID[rule.RouteGUID]
280+
if !exists {
281+
// Skip rules for routes that don't match the optional filters
282+
continue
283+
}
284+
285+
scopeType, sourceName, warnings, err := actor.resolveAccessRuleSource(rule.Selector)
286+
allWarnings = append(allWarnings, warnings...)
287+
if err != nil {
288+
// If we can't resolve the source, sourceName is already empty string
289+
// scopeType is still set correctly
290+
}
291+
292+
results = append(results, AccessRuleWithRoute{
293+
AccessRule: resources.AccessRule(rule),
294+
Route: route,
295+
DomainName: domainCache[route.DomainGUID],
296+
ScopeType: scopeType,
297+
SourceName: sourceName,
298+
})
299+
}
300+
301+
return results, allWarnings, nil
302+
}
303+
304+
// resolveAccessRuleSource resolves a selector to scope type and human-readable source name
305+
func (actor Actor) resolveAccessRuleSource(selector string) (scopeType string, sourceName string, warnings Warnings, err error) {
306+
allWarnings := Warnings{}
307+
308+
// Parse selector format: cf:app:<guid>, cf:space:<guid>, cf:org:<guid>, or cf:any
309+
if selector == "cf:any" {
310+
return "any", "", nil, nil
311+
}
312+
313+
// Split selector into parts
314+
// Expected format: cf:type:guid
315+
const prefix = "cf:"
316+
if len(selector) < len(prefix) {
317+
return "unknown", "", nil, nil
318+
}
319+
320+
selectorBody := selector[len(prefix):]
321+
parts := splitSelector(selectorBody)
322+
if len(parts) < 2 {
323+
return "unknown", "", nil, nil
324+
}
325+
326+
selectorType := parts[0]
327+
guid := parts[1]
328+
329+
switch selectorType {
330+
case "app":
331+
apps, apiWarnings, err := actor.CloudControllerClient.GetApplications(
332+
ccv3.Query{Key: ccv3.GUIDFilter, Values: []string{guid}},
333+
)
334+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
335+
if err != nil || len(apps) == 0 {
336+
return "app", "", allWarnings, err
337+
}
338+
return "app", apps[0].Name, allWarnings, nil
339+
340+
case "space":
341+
spaces, _, apiWarnings, err := actor.CloudControllerClient.GetSpaces(
342+
ccv3.Query{Key: ccv3.GUIDFilter, Values: []string{guid}},
343+
)
344+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
345+
if err != nil || len(spaces) == 0 {
346+
return "space", "", allWarnings, err
347+
}
348+
return "space", spaces[0].Name, allWarnings, nil
349+
350+
case "org":
351+
orgs, apiWarnings, err := actor.CloudControllerClient.GetOrganizations(
352+
ccv3.Query{Key: ccv3.GUIDFilter, Values: []string{guid}},
353+
)
354+
allWarnings = append(allWarnings, Warnings(apiWarnings)...)
355+
if err != nil || len(orgs) == 0 {
356+
return "org", "", allWarnings, err
357+
}
358+
return "org", orgs[0].Name, allWarnings, nil
359+
360+
default:
361+
return "unknown", "", nil, nil
362+
}
363+
}
364+
365+
// splitSelector splits a selector body by colon, handling the case where
366+
// the selector might be "type:guid" format
367+
func splitSelector(s string) []string {
368+
var parts []string
369+
current := ""
370+
for _, char := range s {
371+
if char == ':' && len(parts) == 0 {
372+
// First colon - split here
373+
parts = append(parts, current)
374+
current = ""
375+
} else {
376+
current += string(char)
377+
}
378+
}
379+
if current != "" {
380+
parts = append(parts, current)
381+
}
382+
return parts
383+
}

0 commit comments

Comments
 (0)