Skip to content

Commit a463577

Browse files
Merge pull request #7034 from devtron-labs/fix-link-app-rbac
feat: resolve RBAC inconsistencies for helm apps with unique identifiers
2 parents 5916f39 + 1679300 commit a463577

4 files changed

Lines changed: 285 additions & 19 deletions

File tree

‎internal/sql/repository/app/AppRepository.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,18 @@ func (app *App) IsAppJobOrExternalType() bool {
5353
return len(app.DisplayName) > 0
5454
}
5555

56+
// GetAppNameForRbac returns the app name that rbac objects are built with.
57+
// For an external helm app linked to chart store, app_name holds the unique identifier
58+
// (<releaseName>-<namespace>-<clusterId>) while display_name holds the release name. Rbac policies are
59+
// always created against the release name, as that is what is shown to the user, so display_name must
60+
// win whenever it is set.
61+
func (app *App) GetAppNameForRbac() string {
62+
if len(app.DisplayName) > 0 {
63+
return app.DisplayName
64+
}
65+
return app.AppName
66+
}
67+
5668
type AppRepository interface {
5769
SaveWithTxn(pipelineGroup *App, tx *pg.Tx) error
5870
Update(app *App) error

‎util/rbac/EnforcerUtil.go‎

Lines changed: 56 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package rbac
1818

1919
import (
2020
"fmt"
21+
bean3 "github.com/devtron-labs/devtron/api/helm-app/service/bean"
2122
"github.com/devtron-labs/devtron/pkg/app/dbMigration"
2223
repository2 "github.com/devtron-labs/devtron/pkg/cluster/environment/repository"
2324
bean2 "github.com/devtron-labs/devtron/pkg/k8s/application/bean"
@@ -464,6 +465,8 @@ func (impl EnforcerUtilImpl) GetHelmObject(appId int, envId int) (string, string
464465
impl.logger.Errorw("error on fetching data for rbac object", "err", err)
465466
return fmt.Sprintf("%s/%s/%s", "", "", ""), ""
466467
}
468+
appNameForRbac := application.GetAppNameForRbac()
469+
teamName := getTeamNameForHelmRbac(application)
467470
clusterName := env.Cluster.ClusterName
468471
namespace := env.Namespace
469472

@@ -486,31 +489,26 @@ func (impl EnforcerUtilImpl) GetHelmObject(appId int, envId int) (string, string
486489
}*/
487490

488491
if environmentIdentifier2 == "" {
489-
return fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName), ""
492+
return fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac), ""
490493
}
491494

492-
return fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName),
493-
fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier2, application.AppName)
495+
return fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac),
496+
fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier2, appNameForRbac)
494497
}
495498

496499
func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envId int) (string, string) {
497-
application, err := impl.appRepo.FindAppAndProjectByAppName(appName)
498-
if err == pg.ErrMultiRows {
499-
application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(appName)
500-
if err != nil {
501-
impl.logger.Errorw("error on fetching data for rbac object", "appName", appName, "err", err)
502-
return fmt.Sprintf("%s/%s/%s", "", "", ""), ""
503-
}
504-
}
500+
env, err := impl.environmentRepository.FindById(envId)
505501
if err != nil {
506502
impl.logger.Errorw("error on fetching data for rbac object", "err", err)
507503
return fmt.Sprintf("%s/%s/%s", "", "", ""), ""
508504
}
509-
env, err := impl.environmentRepository.FindById(envId)
505+
application, err := impl.getAppForHelmRbac(appName, env.ClusterId, env.Namespace)
510506
if err != nil {
511-
impl.logger.Errorw("error on fetching data for rbac object", "err", err)
507+
impl.logger.Errorw("error on fetching data for rbac object", "appName", appName, "err", err)
512508
return fmt.Sprintf("%s/%s/%s", "", "", ""), ""
513509
}
510+
appNameForRbac := application.GetAppNameForRbac()
511+
teamName := getTeamNameForHelmRbac(application)
514512
clusterName := env.Cluster.ClusterName
515513
namespace := env.Namespace
516514
environmentIdentifier := env.EnvironmentIdentifier
@@ -524,16 +522,58 @@ func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envI
524522
}
525523
}
526524
if environmentIdentifier2 == "" {
527-
return strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName)), ""
525+
return strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac)), ""
528526
}
529527

530528
//TODO - FIX required for futuristic permission for cluster__* all environment for migrated environment identifier only
531529
/*//here cluster, env, namespace must not have double underscore in names, as we are using that for separator.
532530
if !strings.HasPrefix(env.EnvironmentIdentifier, fmt.Sprintf("%s__", env.Cluster.ClusterName)) {
533531
environmentIdentifier = fmt.Sprintf("%s__%s", env.Cluster.ClusterName, env.EnvironmentIdentifier)
534532
}*/
535-
return strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName)),
536-
strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier2, application.AppName))
533+
return strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac)),
534+
strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier2, appNameForRbac))
535+
}
536+
537+
// getTeamNameForHelmRbac returns the project name to be used while building a helm rbac object.
538+
// An external helm app can be linked to chart store without a project being assigned to it, in which case
539+
// team_id stays 0 and the joined Team row is empty. Permissions for such apps are granted under the
540+
// "unassigned" project, so that literal has to be used instead of an empty project name.
541+
func getTeamNameForHelmRbac(application *app.App) string {
542+
if application.TeamId == 0 {
543+
return repository3.UNASSIGNED_PROJECT
544+
}
545+
return application.Team.Name
546+
}
547+
548+
// getAppForHelmRbac resolves the app row for a helm rbac object, given a name that may either be the
549+
// app_name stored in db or the display_name shown to the user.
550+
// For an external helm app linked to chart store, app_name is the unique identifier
551+
// (<releaseName>-<namespace>-<clusterId>) while callers here generally only have the release name, so the
552+
// identifier is reconstructed from the environment and tried first. The plain-name lookup is kept as a
553+
// fallback for regular chart store apps, for legacy rows created before the unique identifier existed, and
554+
// for callers that already pass the identifier.
555+
func (impl EnforcerUtilImpl) getAppForHelmRbac(appName string, clusterId int, namespace string) (*app.App, error) {
556+
appIdentifier := &bean3.AppIdentifier{
557+
ClusterId: clusterId,
558+
Namespace: namespace,
559+
ReleaseName: appName,
560+
}
561+
uniqueAppNameIdentifier := appIdentifier.GetUniqueAppNameIdentifier()
562+
application, err := impl.appRepo.FindAppAndProjectByAppName(uniqueAppNameIdentifier)
563+
if err == pg.ErrMultiRows {
564+
application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(uniqueAppNameIdentifier)
565+
}
566+
if application == nil || err == pg.ErrNoRows {
567+
impl.logger.Debugw("app not found by unique identifier, falling back to app name", "appIdentifier", uniqueAppNameIdentifier, "appName", appName)
568+
application, err = impl.appRepo.FindAppAndProjectByAppName(appName)
569+
if err == pg.ErrMultiRows {
570+
application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(appName)
571+
}
572+
}
573+
if err != nil {
574+
return nil, err
575+
}
576+
return application, nil
537577
}
538578

539579
func (impl EnforcerUtilImpl) GetHelmObjectByProjectIdAndEnvId(teamId int, envId int) (string, string) {

‎util/rbac/EnforcerUtilHelm.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,9 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppId(installedAppVers
177177
impl.logger.Errorw("error in fetching installed app version data", "err", err)
178178
return fmt.Sprintf("%s/%s/%s", "", "", ""), fmt.Sprintf("%s/%s/%s", "", "", "")
179179
}
180-
rbacOne := fmt.Sprintf("%s/%s/%s", InstalledApp.App.Team.Name, InstalledApp.Environment.EnvironmentIdentifier, InstalledApp.App.AppName)
180+
appNameForRbac := InstalledApp.App.GetAppNameForRbac()
181+
teamName := getTeamNameForHelmRbac(&InstalledApp.App)
182+
rbacOne := fmt.Sprintf("%s/%s/%s", teamName, InstalledApp.Environment.EnvironmentIdentifier, appNameForRbac)
181183

182184
if InstalledApp.Environment.IsVirtualEnvironment {
183185
return rbacOne, ""
@@ -186,7 +188,7 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppId(installedAppVers
186188
var rbacTwo string
187189
if !InstalledApp.Environment.IsVirtualEnvironment {
188190
if InstalledApp.Environment.EnvironmentIdentifier != InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace {
189-
rbacTwo = fmt.Sprintf("%s/%s/%s", InstalledApp.App.Team.Name, InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace, InstalledApp.App.AppName)
191+
rbacTwo = fmt.Sprintf("%s/%s/%s", teamName, InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace, appNameForRbac)
190192
return rbacOne, rbacTwo
191193
}
192194
}
@@ -205,6 +207,6 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppIdAndTeamId(install
205207
impl.logger.Errorw("error in fetching project by teamID", "err", err)
206208
return fmt.Sprintf("%s/%s/%s", "", "", "")
207209
}
208-
rbac := fmt.Sprintf("%s/%s/%s", project.Name, installedApp.Environment.EnvironmentIdentifier, installedApp.App.AppName)
210+
rbac := fmt.Sprintf("%s/%s/%s", project.Name, installedApp.Environment.EnvironmentIdentifier, installedApp.App.GetAppNameForRbac())
209211
return rbac
210212
}
Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
/*
2+
* Copyright (c) 2020-2024. Devtron Inc.
3+
*/
4+
5+
package rbac
6+
7+
import (
8+
"testing"
9+
10+
"github.com/devtron-labs/devtron/internal/sql/repository/app"
11+
environmentRepository "github.com/devtron-labs/devtron/pkg/cluster/environment/repository"
12+
clusterRepository "github.com/devtron-labs/devtron/pkg/cluster/repository"
13+
teamRepo "github.com/devtron-labs/devtron/pkg/team/repository"
14+
"github.com/go-pg/pg"
15+
"go.uber.org/zap"
16+
)
17+
18+
// stubAppRepo overrides only the lookups used while building helm rbac objects; every other method of the
19+
// interface is left nil and will panic if a code path unexpectedly reaches it.
20+
type stubAppRepo struct {
21+
app.AppRepository
22+
byName map[string]*app.App
23+
byId map[int]*app.App
24+
}
25+
26+
func (s *stubAppRepo) FindAppAndProjectByAppName(appName string) (*app.App, error) {
27+
if a, ok := s.byName[appName]; ok {
28+
return a, nil
29+
}
30+
// mirrors the real repository, which returns an empty (non nil) app alongside ErrNoRows
31+
return &app.App{}, pg.ErrNoRows
32+
}
33+
34+
func (s *stubAppRepo) FindAppAndProjectByAppId(appId int) (*app.App, error) {
35+
if a, ok := s.byId[appId]; ok {
36+
return a, nil
37+
}
38+
return &app.App{}, pg.ErrNoRows
39+
}
40+
41+
type stubEnvRepo struct {
42+
environmentRepository.EnvironmentRepository
43+
env *environmentRepository.Environment
44+
}
45+
46+
func (s *stubEnvRepo) FindById(id int) (*environmentRepository.Environment, error) {
47+
if s.env == nil || s.env.Id != id {
48+
return nil, pg.ErrNoRows
49+
}
50+
return s.env, nil
51+
}
52+
53+
const (
54+
testClusterName = "default_cluster"
55+
testNamespace = "devtron-demo"
56+
testEnvId = 1
57+
testClusterId = 1
58+
)
59+
60+
// linkedExternalApp is how an external helm app looks in the app table once it is linked to chart store:
61+
// app_name carries the unique identifier while display_name carries the release name.
62+
func linkedExternalApp(id int, releaseName string, teamId int, teamName string) *app.App {
63+
return &app.App{
64+
Id: id,
65+
AppName: releaseName + "-" + testNamespace + "-1",
66+
DisplayName: releaseName,
67+
TeamId: teamId,
68+
Team: teamRepo.Team{Name: teamName},
69+
}
70+
}
71+
72+
func newTestEnforcerUtil(envIdentifier string) *EnforcerUtilImpl {
73+
nginx := linkedExternalApp(1, "nginx", 2, "myproject")
74+
// linked with no project assigned -> team_id stays 0 and the joined team row is empty
75+
redis := linkedExternalApp(2, "redis", 0, "")
76+
// a regular chart store app, which has no display name at all
77+
grafana := &app.App{Id: 3, AppName: "grafana", TeamId: 2, Team: teamRepo.Team{Name: "myproject"}}
78+
79+
appRepo := &stubAppRepo{
80+
byName: map[string]*app.App{
81+
nginx.AppName: nginx,
82+
redis.AppName: redis,
83+
grafana.AppName: grafana,
84+
},
85+
byId: map[int]*app.App{1: nginx, 2: redis, 3: grafana},
86+
}
87+
envRepo := &stubEnvRepo{env: &environmentRepository.Environment{
88+
Id: testEnvId,
89+
Name: testNamespace,
90+
ClusterId: testClusterId,
91+
Namespace: testNamespace,
92+
EnvironmentIdentifier: envIdentifier,
93+
Cluster: &clusterRepository.Cluster{Id: testClusterId, ClusterName: testClusterName},
94+
}}
95+
return &EnforcerUtilImpl{logger: zap.NewNop().Sugar(), appRepo: appRepo, environmentRepository: envRepo}
96+
}
97+
98+
func TestGetHelmObjectByAppNameAndEnvId(t *testing.T) {
99+
envIdentifier := testClusterName + "__" + testNamespace
100+
tests := []struct {
101+
name string
102+
appName string
103+
wantObject string
104+
wantObject2 string
105+
}{
106+
{
107+
name: "external app linked to chart store is resolved by its display name",
108+
// this is the regression: the listing APIs hand over the release name, but app_name in db holds
109+
// the unique identifier, so the lookup used to miss and the object came back as "//"
110+
appName: "nginx",
111+
wantObject: "myproject/" + envIdentifier + "/nginx",
112+
},
113+
{
114+
name: "external app linked without a project falls under the unassigned project",
115+
appName: "redis",
116+
wantObject: teamRepo.UNASSIGNED_PROJECT + "/" + envIdentifier + "/redis",
117+
},
118+
{
119+
name: "regular chart store app keeps working via the plain app name",
120+
appName: "grafana",
121+
wantObject: "myproject/" + envIdentifier + "/grafana",
122+
},
123+
{
124+
name: "caller passing the unique identifier still gets the display name based object",
125+
// some handlers hand over installedApp.App.AppName directly
126+
appName: "nginx-" + testNamespace + "-1",
127+
wantObject: "myproject/" + envIdentifier + "/nginx",
128+
},
129+
{
130+
name: "unknown app yields an empty object",
131+
appName: "does-not-exist",
132+
wantObject: "//",
133+
},
134+
}
135+
impl := newTestEnforcerUtil(envIdentifier)
136+
for _, tt := range tests {
137+
t.Run(tt.name, func(t *testing.T) {
138+
object, object2 := impl.GetHelmObjectByAppNameAndEnvId(tt.appName, testEnvId)
139+
if object != tt.wantObject {
140+
t.Errorf("object = %q, want %q", object, tt.wantObject)
141+
}
142+
if object2 != tt.wantObject2 {
143+
t.Errorf("object2 = %q, want %q", object2, tt.wantObject2)
144+
}
145+
})
146+
}
147+
}
148+
149+
// When the environment identifier is not clusterName__namespace, both the migrated and the futuristic
150+
// permission objects are returned, and each has to carry the display name.
151+
func TestGetHelmObjectByAppNameAndEnvId_FuturisticPermissionObject(t *testing.T) {
152+
impl := newTestEnforcerUtil("devtron-demo")
153+
object, object2 := impl.GetHelmObjectByAppNameAndEnvId("nginx", testEnvId)
154+
if want := "myproject/devtron-demo/nginx"; object != want {
155+
t.Errorf("object = %q, want %q", object, want)
156+
}
157+
if want := "myproject/" + testClusterName + "__" + testNamespace + "/nginx"; object2 != want {
158+
t.Errorf("object2 = %q, want %q", object2, want)
159+
}
160+
}
161+
162+
// GetHelmObjectByAppNameAndEnvId lowercases its objects, so a mixed case release name still has to line up
163+
// with the policy that was stored for it.
164+
func TestGetHelmObjectByAppNameAndEnvId_LowerCased(t *testing.T) {
165+
envIdentifier := testClusterName + "__" + testNamespace
166+
mixed := &app.App{
167+
Id: 4,
168+
AppName: "MyRelease-" + testNamespace + "-1",
169+
DisplayName: "MyRelease",
170+
TeamId: 2,
171+
Team: teamRepo.Team{Name: "MyProject"},
172+
}
173+
impl := newTestEnforcerUtil(envIdentifier)
174+
impl.appRepo.(*stubAppRepo).byName[mixed.AppName] = mixed
175+
176+
object, _ := impl.GetHelmObjectByAppNameAndEnvId("MyRelease", testEnvId)
177+
if want := "myproject/" + envIdentifier + "/myrelease"; object != want {
178+
t.Errorf("object = %q, want %q", object, want)
179+
}
180+
}
181+
182+
func TestGetHelmObject(t *testing.T) {
183+
envIdentifier := testClusterName + "__" + testNamespace
184+
impl := newTestEnforcerUtil(envIdentifier)
185+
186+
// app id lookup always succeeded, but the object used to be built from app_name (the unique identifier)
187+
object, _ := impl.GetHelmObject(1, testEnvId)
188+
if want := "myproject/" + envIdentifier + "/nginx"; object != want {
189+
t.Errorf("object = %q, want %q", object, want)
190+
}
191+
192+
object, _ = impl.GetHelmObject(2, testEnvId)
193+
if want := teamRepo.UNASSIGNED_PROJECT + "/" + envIdentifier + "/redis"; object != want {
194+
t.Errorf("object = %q, want %q", object, want)
195+
}
196+
197+
object, _ = impl.GetHelmObject(3, testEnvId)
198+
if want := "myproject/" + envIdentifier + "/grafana"; object != want {
199+
t.Errorf("object = %q, want %q", object, want)
200+
}
201+
}
202+
203+
func TestGetAppNameForRbac(t *testing.T) {
204+
external := &app.App{AppName: "nginx-devtron-demo-1", DisplayName: "nginx"}
205+
if got := external.GetAppNameForRbac(); got != "nginx" {
206+
t.Errorf("GetAppNameForRbac() = %q, want %q", got, "nginx")
207+
}
208+
regular := &app.App{AppName: "grafana"}
209+
if got := regular.GetAppNameForRbac(); got != "grafana" {
210+
t.Errorf("GetAppNameForRbac() = %q, want %q", got, "grafana")
211+
}
212+
}

0 commit comments

Comments
 (0)