Skip to content

Commit d767e20

Browse files
committed
Add client-only helm dry-run to helm applier
Adds a check that makes sure the installer service account has the necessary get permissions to get all the resources it will need to inspect for a server-connected dry-run and returns errors detailing all the missing get permissions. This prevents a hung server-connected dry-run getting caught on individual missing get permissions. Signed-off-by: Tayler Geiger <[email protected]>
1 parent a46ff7d commit d767e20

File tree

4 files changed

+153
-0
lines changed

4 files changed

+153
-0
lines changed

cmd/operator-controller/main.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,9 +355,12 @@ func main() {
355355
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
356356
}
357357

358+
acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig())
359+
358360
applier := &applier.Helm{
359361
ActionClientGetter: acg,
360362
Preflights: preflights,
363+
AuthClientMapper: acm,
361364
}
362365

363366
cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())

go.mod

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ require (
3535
k8s.io/client-go v0.32.0
3636
k8s.io/component-base v0.32.0
3737
k8s.io/klog/v2 v2.130.1
38+
k8s.io/kubernetes v1.31.2
3839
k8s.io/utils v0.0.0-20241210054802-24370beab758
3940
sigs.k8s.io/controller-runtime v0.19.4
4041
sigs.k8s.io/yaml v1.4.0
@@ -243,6 +244,7 @@ require (
243244
gopkg.in/inf.v0 v0.9.1 // indirect
244245
gopkg.in/warnings.v0 v0.1.2 // indirect
245246
gopkg.in/yaml.v3 v3.0.1 // indirect
247+
k8s.io/component-helpers v0.32.0 // indirect
246248
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect
247249
k8s.io/kubectl v0.32.0 // indirect
248250
oras.land/oras-go v1.2.5 // indirect

go.sum

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,12 +1006,16 @@ k8s.io/client-go v0.32.0 h1:DimtMcnN/JIKZcrSrstiwvvZvLjG0aSxy8PxN8IChp8=
10061006
k8s.io/client-go v0.32.0/go.mod h1:boDWvdM1Drk4NJj/VddSLnx59X3OPgwrOo0vGbtq9+8=
10071007
k8s.io/component-base v0.32.0 h1:d6cWHZkCiiep41ObYQS6IcgzOUQUNpywm39KVYaUqzU=
10081008
k8s.io/component-base v0.32.0/go.mod h1:JLG2W5TUxUu5uDyKiH2R/7NnxJo1HlPoRIIbVLkK5eM=
1009+
k8s.io/component-helpers v0.32.0 h1:pQEEBmRt3pDJJX98cQvZshDgJFeKRM4YtYkMmfOlczw=
1010+
k8s.io/component-helpers v0.32.0/go.mod h1:9RuClQatbClcokXOcDWSzFKQm1huIf0FzQlPRpizlMc=
10091011
k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
10101012
k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
10111013
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f h1:GA7//TjRY9yWGy1poLzYYJJ4JRdzg3+O6e8I+e+8T5Y=
10121014
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f/go.mod h1:R/HEjbvWI0qdfb8viZUeVZm0X6IZnxAydC7YU42CMw4=
10131015
k8s.io/kubectl v0.32.0 h1:rpxl+ng9qeG79YA4Em9tLSfX0G8W0vfaiPVrc/WR7Xw=
10141016
k8s.io/kubectl v0.32.0/go.mod h1:qIjSX+QgPQUgdy8ps6eKsYNF+YmFOAO3WygfucIqFiE=
1017+
k8s.io/kubernetes v1.31.2 h1:VNSu4O7Xn5FFRsh9ePXyEPg6ucR21fOftarSdi053Gs=
1018+
k8s.io/kubernetes v1.31.2/go.mod h1:9xmT2buyTYj8TRKwRae7FcuY8k5+xlxv7VivvO0KKfs=
10151019
k8s.io/utils v0.0.0-20241210054802-24370beab758 h1:sdbE21q2nlQtFh65saZY+rRM6x6aJJI8IUa1AmH/qa0=
10161020
k8s.io/utils v0.0.0-20241210054802-24370beab758/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
10171021
oras.land/oras-go v1.2.5 h1:XpYuAwAb0DfQsunIyMfeET92emK8km3W4yEzZvUbsTo=

internal/applier/helm.go

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,24 @@ import (
1616
"helm.sh/helm/v3/pkg/release"
1717
"helm.sh/helm/v3/pkg/storage/driver"
1818
corev1 "k8s.io/api/core/v1"
19+
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1920
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2021
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
22+
"k8s.io/apiserver/pkg/authorization/authorizer"
23+
"k8s.io/client-go/rest"
2124
"sigs.k8s.io/controller-runtime/pkg/client"
2225
"sigs.k8s.io/controller-runtime/pkg/log"
2326

2427
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
28+
authv1 "k8s.io/api/authorization/v1"
29+
rbacv1 "k8s.io/api/rbac/v1"
30+
authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1"
2531

2632
ocv1 "github.com/operator-framework/operator-controller/api/v1"
2733
"github.com/operator-framework/operator-controller/internal/rukpak/convert"
2834
"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
2935
"github.com/operator-framework/operator-controller/internal/rukpak/util"
36+
rbacauthorizer "k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"
3037
)
3138

3239
const (
@@ -52,9 +59,38 @@ type Preflight interface {
5259
Upgrade(context.Context, *release.Release) error
5360
}
5461

62+
type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error)
63+
64+
type AuthClientMapper struct {
65+
rcm RestConfigMapper
66+
baseCfg *rest.Config
67+
}
68+
69+
func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (*authorizationv1client.AuthorizationV1Client, error) {
70+
authcfg, err := acm.rcm(ctx, ext, acm.baseCfg)
71+
if err != nil {
72+
return nil, err
73+
}
74+
75+
authclient, err := authorizationv1client.NewForConfig(authcfg)
76+
if err != nil {
77+
return nil, err
78+
}
79+
80+
return authclient, nil
81+
}
82+
5583
type Helm struct {
5684
ActionClientGetter helmclient.ActionClientGetter
5785
Preflights []Preflight
86+
AuthClientMapper AuthClientMapper
87+
}
88+
89+
func NewAuthClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthClientMapper {
90+
return AuthClientMapper{
91+
rcm: rcm,
92+
baseCfg: baseCfg,
93+
}
5894
}
5995

6096
// shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND
@@ -93,6 +129,16 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
93129
labels: objectLabels,
94130
}
95131

132+
authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext)
133+
if err != nil {
134+
return nil, "", err
135+
}
136+
137+
err = h.checkGetPermissions(ctx, authclient, ac, ext, chrt, values, post)
138+
if err != nil {
139+
return nil, "", err
140+
}
141+
96142
rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post)
97143
if err != nil {
98144
return nil, "", err
@@ -151,8 +197,101 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
151197
return relObjects, state, nil
152198
}
153199

200+
// re-org this to take in just the resulting manifest so I can use it for checking both the client-only and server-connnected dry-runs
201+
func (h *Helm) checkGetPermissions(ctx context.Context, authcl *authorizationv1client.AuthorizationV1Client, cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) error {
202+
// client-only dry run
203+
// plainObjs, err := convert.Convert(chrt, "", []string)
204+
clientDryRunRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
205+
i.DryRun = true
206+
i.DryRunOption = "client"
207+
return nil
208+
}, helmclient.AppendInstallPostRenderer(post))
209+
if err != nil {
210+
return err
211+
}
212+
objects, err := util.ManifestObjects(strings.NewReader(clientDryRunRelease.Manifest), fmt.Sprintf("%s-release-manifest", clientDryRunRelease.Name))
213+
214+
if err != nil {
215+
return err
216+
}
217+
218+
ssrr := &authv1.SelfSubjectRulesReview{
219+
Spec: authv1.SelfSubjectRulesReviewSpec{
220+
Namespace: ext.Spec.Namespace,
221+
},
222+
}
223+
224+
ssrr, err = authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, v1.CreateOptions{})
225+
if err != nil {
226+
return err
227+
}
228+
229+
rules := []rbacv1.PolicyRule{}
230+
for _, rule := range ssrr.Status.ResourceRules {
231+
rules = append(rules, rbacv1.PolicyRule{
232+
Verbs: rule.Verbs,
233+
APIGroups: rule.APIGroups,
234+
Resources: rule.Resources,
235+
ResourceNames: rule.ResourceNames,
236+
})
237+
}
238+
239+
for _, rule := range ssrr.Status.NonResourceRules {
240+
rules = append(rules, rbacv1.PolicyRule{
241+
Verbs: rule.Verbs,
242+
NonResourceURLs: rule.NonResourceURLs,
243+
})
244+
}
245+
246+
resAttrs := []authorizer.AttributesRecord{}
247+
errs := []error{}
248+
249+
for _, o := range objects {
250+
resAttrs = append(resAttrs, authorizer.AttributesRecord{
251+
Namespace: o.GetNamespace(),
252+
Verb: "get",
253+
APIGroup: o.GetObjectKind().GroupVersionKind().Group,
254+
Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind),
255+
Name: o.GetName(),
256+
ResourceRequest: true,
257+
})
258+
}
259+
260+
for _, resAttr := range resAttrs {
261+
if !rbacauthorizer.RulesAllow(resAttr, rules...) {
262+
errs = append(errs, fmt.Errorf("serviceaccount %s cannot get %s in namespace %s",
263+
ext.Spec.ServiceAccount.Name,
264+
resAttr.Resource,
265+
resAttr.Namespace))
266+
}
267+
}
268+
if len(errs) > 0 {
269+
errs = append([]error{fmt.Errorf("installer service account %s is missing required get permissions", ext.Spec.ServiceAccount.Name)}, errs...)
270+
}
271+
272+
return errors.Join(errs...)
273+
274+
// for _, o := range objects {
275+
// ssar := &authv1.SelfSubjectAccessReview{
276+
// Spec: authv1.SelfSubjectAccessReviewSpec{
277+
// ResourceAttributes: &authv1.ResourceAttributes{
278+
// Namespace: ext.Spec.Namespace,
279+
// Verb: "get",
280+
// Resource: o.GetObjectKind().GroupVersionKind().Kind,
281+
// Group: o.GetObjectKind().GroupVersionKind().Group,
282+
// },
283+
// },
284+
// }
285+
// ssar, err = authcl.SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
286+
// if err != nil {
287+
// return err
288+
// }
289+
// }
290+
}
291+
154292
func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) {
155293
currentRelease, err := cl.Get(ext.GetName())
294+
156295
if errors.Is(err, driver.ErrReleaseNotFound) {
157296
desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error {
158297
i.DryRun = true
@@ -186,6 +325,11 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
186325
return currentRelease, desiredRelease, relState, nil
187326
}
188327

328+
// RulesAllow() checks expects resource names to be lowercase and plural, there's probably a better way to do this
329+
func sanitizeResourceName(resourceName string) string {
330+
return strings.ToLower(resourceName) + "s"
331+
}
332+
189333
type postrenderer struct {
190334
labels map[string]string
191335
cascade postrender.PostRenderer

0 commit comments

Comments
 (0)