Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow selecting which SA should be used to install a Carvel pkg #3883

Merged
merged 27 commits into from
Dec 13, 2021
Merged

Allow selecting which SA should be used to install a Carvel pkg #3883

merged 27 commits into from
Dec 13, 2021

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Dec 1, 2021 •
edited

Description of the change

This PR (still a draft, tests remaining), aims at adding a new service account selector field when deploying a package (iff it is a carvel package, atm).
It requires:

  1. Adding a new backend endpoint for retrieving the SA; not the corev1.ServiceAccount object, but just their names (as we don't need the SA secret names at all).
    • I implemented it in Kubeops (as we did for the can-i endpoint); notwithstanding, this code ought to be moved to a separate plugin at some point.
    • I implemented it in the new Resources plugin :)
  2. Consuming this new API endpoint from the backend
  3. Creating a selector with its logic to i) fetch the whole list; ii) pass the selected SA to the createInstalledPkg op.

The list of changes is not that big, but I'm willing to split each of those ones out to separate PRs.

Benefits

Installing Carvel packages will work as the Carvel folks intended, I guess :P

image

image

Possible drawbacks

N/A

Applicable issues

Additional information

I'll work on the tests tomorrow
This PR depends on #3894 (yeah, I could have stacked it, but it would imply recreating the branch upstream and I don't foresee much work on the other, so once merged, it will get simplified (ie, not so many changes as it is atm).

@antgamdia
Use the cds select in the deployment/upgrade forms
a183e9b 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Use the pkg display name in the form header
9b187a3 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Add serviceaccount field when deploying wip
da9ce20 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Remove hardcoded SA
3c988e9 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Allow passing rec. opts. in UI
78ac927 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Allow displaying rec. opt. in installed view
51987df 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Implement listServiceAccounts operation in kubeops
4fbe6fc 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Add listServiceAccounts client in UI
20284c6 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Retrieve the SA list in the deplotyment view. Pass the SA through
378f29f 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia changed the title Use sa for deployment Allow selecting which SA should be used to install a Carvel pkg Dec 1, 2021
@absoludity
Copy link
Contributor

This PR (still a draft, tests remaining),

k, won't look at the code yet :)

aims at adding a new service account selector field when deploying a package (iff it is a carvel package, atm).

I think we want this for both carvel and flux plugins, no?

It requires:

1. Adding a new backend endpoint for retrieving the SA; not the `corev1.ServiceAccount`  object, but just their names (as we don't need the SA secret names at all).
   
   * I implemented it in Kubeops (as we did for the can-i endpoint); notwithstanding, this code ought to be moved to a separate plugin at some point.

Yeah, I'd be keen not to add more code to Kubeops right now. This is exactly the thing for the resources plugin, imo, since it's not specific to any packaging format. GetServiceAccountNames passing the context, would fit? (And it'd still be doing so only if the users RBAC allows).

The list of changes is not that big, but I'm willing to split each of those ones out to separate PRs.

at +246, -83 (the current modified/deleted lines) it's absolutely no issue. If it grows significantly then it may be worth splitting to help reviews, just see what you think.

@antgamdia
Merge branch 'master' into useSAForDeployment
7501b5c
@antgamdia
Fix test to get the dispaly name rather than id
28b3286 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Centralize conditions about plugin-specific capabilities
ea9e619 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Store reconciliation opts instead of just the SA
1c23afa 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Merge branch 'master'
3856ae1 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/src/components/DeploymentForm/DeploymentForm.tsx
@antgamdia
Add some tests
9b37afa 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Copy link
Contributor Author

Yeah, I'd be keen not to add more code to Kubeops right now. This is exactly the thing for the resources plugin, imo, since it's not specific to any packaging format. GetServiceAccountNames passing the context, would fit? (And it'd still be doing so only if the users RBAC allows).

Great; I wasn't sure if the purpose of these resources plugins was solely giving those resources generated by an installation or if it was aimed at covering a wide range of scenarios (like this one or even the can-i).
So, I'll do a prequel PR adding the new stuff for this operation (proto+autogenerated code)

@antgamdia
Add proto for GetServiceAccountNames
50778fd 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Add generated files
f911d73 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Remove stream
83f9520 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Run buf generate again
88d9760 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Remove implemented logic in kubeops
41369fc 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Fix failing tests after remove default SA
2ff2dbd 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Add impl + test of GetServiceAccountNames
6246893 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Run prettier
eaf843d 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Use kubeappsapis for retrieving SAs
6bca58f 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia
Remove leftover
23677d9 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia mentioned this pull request Dec 2, 2021
Merged
@antgamdia
Merge branch 'master'
8fcd4ea 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go
	dashboard/src/components/AppView/AppView.tsx
antgamdia
antgamdia commented Dec 3, 2021
View reviewed changes
dashboard/src/shared/utils.ts
Comment on lines +129 to +135
export function getPluginsRequiringSA(): string[] {
return [PluginNames.PACKAGES_FLUX, PluginNames.PACKAGES_KAPP];
}

export function getPluginsSupportingRollback(): string[] {
return [PluginNames.PACKAGES_HELM];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting the logic of which plugins are for certain capabilities, to DRY-up the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent - much better to have it all in the one place for when we want to replace it with the plugin-provided items themselves.

dashboard/src/components/DeploymentForm/DeploymentForm.tsx
Comment on lines +68 to +70
setReconciliationOptions({
...reconciliationOptions,
serviceAccountName: e.currentTarget.value,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, we may want to allow modifying other parameters, like the reconciliation interval, that's why I'm storing this obj instead of just the SA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sound good.

dashboard/src/components/DeploymentForm/DeploymentForm.tsx
if (getPluginsRequiringSA().includes(pluginObj.name)) {
// We assume the user has enough permissions to do that. Fallback to a simple input maybe?
Kube.getServiceAccountNames(targetCluster, targetNamespace).then(saList =>
setServiceAccountList(saList.serviceaccountNames),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm storing the SA list in the component's state solely; as it is not shared by other components.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and we need to fetch it whenever the target namespace changes (as you've indicated below with the dependencies), so yep, component state sounds perfect.

dashboard/src/components/DeploymentForm/DeploymentForm.tsx
Comment on lines +242 to +244
<CdsControlMessage error="valueMissing">
The Service Account name this application will be installed with.
</CdsControlMessage>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CDS components brings some form validation capabilities out-of-the-box (idem with inputs).

@antgamdia antgamdia marked this pull request as ready for review December 3, 2021 08:12
absoludity
absoludity approved these changes Dec 5, 2021
View reviewed changes
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I ignored the generated code and backend changes (which I assume are because you didn't get to rebase on master and remove the commits from #3894).

dashboard/src/components/DeploymentForm/DeploymentForm.tsx
Comment on lines +68 to +70
setReconciliationOptions({
...reconciliationOptions,
serviceAccountName: e.currentTarget.value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sound good.

dashboard/src/components/DeploymentForm/DeploymentForm.tsx
if (getPluginsRequiringSA().includes(pluginObj.name)) {
// We assume the user has enough permissions to do that. Fallback to a simple input maybe?
Kube.getServiceAccountNames(targetCluster, targetNamespace).then(saList =>
setServiceAccountList(saList.serviceaccountNames),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and we need to fetch it whenever the target namespace changes (as you've indicated below with the dependencies), so yep, component state sounds perfect.

dashboard/src/shared/utils.ts
Comment on lines +129 to +135
export function getPluginsRequiringSA(): string[] {
return [PluginNames.PACKAGES_FLUX, PluginNames.PACKAGES_KAPP];
}

export function getPluginsSupportingRollback(): string[] {
return [PluginNames.PACKAGES_HELM];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent - much better to have it all in the one place for when we want to replace it with the plugin-provided items themselves.

pkg/http-handler/http-handler.go
@@ -377,6 +377,7 @@ func SetupDefaultRoutes(r *mux.Router, namespaceHeaderName, namespaceHeaderPatte
if err != nil {
return err
}
//TODO(agamez): move these endpoints to a separate plugin when possible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though hopefully we won't just move them. For example, we don't want to provide generic endpoints that can return all app repositories. Let's see.

@antgamdia
Merge branch 'master'
dbe9eeb 
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/src/shared/Kube.ts
@antgamdia antgamdia merged commit 02aa525 into vmware-tanzu:master Dec 13, 2021
@antgamdia antgamdia deleted the useSAForDeployment branch December 13, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absoludity absoludity absoludity approved these changes

@castelblanque castelblanque castelblanque approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a mechanism to select a ServiceAccount with which the Carvel bundle will get installed
3 participants
@antgamdia @absoludity @castelblanque