carvel-dev / carvel-dev/kapp-controller

App CR should require one of serviceAccountName or cluster.

Open
#598 4 comments 2 reactions 0 assignees Claimed by @varshaprasad96 View on GitHub
carvel-accepted enhancement good first issue priority/important-longterm
Dominant language
Go
Stars
323
Forks
127
Avg merge
2d 10h
Merged PRs (30d)
6

Description

**Describe the problem/challenge you have**

Both `serviceAccountName` and `cluster` properties of the `App` CR are marked as optional. Anyone not understanding that one or the other is required may not even add either. When neither is added, then the `App` CR is still accepted and you end up with an error which you have to look in the `App` CR status of:

```
status:
conditions:
- message: 'Preparing kapp: Expected service account or cluster specified'
status: "True"
type: ReconcileFailed
```

Further, when trying to delete the `App` in this situation it fails with it eventually exiting with the error:

```
kapp: Error: waiting on reconcile app/educates-training-platform (kappctrl.k14s.io/v1alpha1) namespace: default:
Finished unsuccessfully (Reconcile failed: (message: Preparing kapp: Expected service account or cluster specified))
```

resulting in you needing to manually delete the finalizer from the `App` resource.

The latter problem relates to issue https://github.com/vmware-tanzu/carvel-kapp-controller/issues/416.

**Describe the solution you'd like**

The CR definition should enforce that one or the other of `serviceAccountName` and `cluster` is required so that attempting to create an `App` CR with neither is rejected immediately.

My understanding is that this can be achieved in the CR schema by using:

```
anyOf:
- required:
- serviceAccountName
- required:
- cluster
```

**Anything else you would like to add:**

Having to look into the status of the `App` CR is non obvious so anything that avoids getting into that situation in the first place is a bonus. So failing early at the point of creating the `App` is better.

---
Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

Contributor guide

Open the contributing guide

Research direction

Start with the App CR definition and its schema validation, focusing on serviceAccountName and cluster. Check the existing validation tests or add a focused test showing that an App with neither field is rejected, while either field alone is accepted.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
devops, infrastructure
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.