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.

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

Add 'CreateInstalledPackage' impl #3789

Merged
merged 27 commits into from Nov 24, 2021
Merged

Add 'CreateInstalledPackage' impl #3789

merged 27 commits into from Nov 24, 2021

Conversation

antgamdia
Copy link
Contributor

Description of the change

Initial implementation of the CreateInstalledPackage operation

Benefits

An initial and very limited Carvel support in Kubeapps

Possible drawbacks

N/A

Applicable issues

Additional information

N/A

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
pkgVersion := request.GetPkgVersionReference().GetVersion()
installedPackageName := request.GetName()
values := request.GetValues()
targetNamespace := request.GetTargetContext().GetNamespace()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not pull out the target cluster as well? Currently it looks like it's just assuming the available package cluster. AFAICT, we don't lose anything by assuming multi-cluster support here, rather than explicitly not supporting it?

}

// create the Secret in the cluster
createdSecret, err := typedClient.CoreV1().Secrets(targetNamespace).Create(ctx, secret, metav1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder if we should do this later in the process before s.createPkgInstall (or even after it, depending on how carvel handles that situation - I assume it'll see the secret as soon as created but we'd have to check). Main thing being to minimize the chance that we create the secret and leave it dangling if the other parts fail.

Actually, it could be simpler that we also clean up the secret if any other error occurs. I'd think we'd need to do this anyway otherwise the next creation would fail (unless you're avoiding collisions on the secret name). Even if not, we still should clean up imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I'm moving the secret creation right before the s.createPkgInstall + adding a TODO to check if we really need to have the secret in advance (I'd say it is possible, but perhaps we will have to wait for an additional reconciliation cycle, but not sure).

APIVersion: k8scorev1.SchemeGroupVersion.WithResource(k8scorev1.ResourceSecrets.String()).String(),
},
ObjectMeta: metav1.ObjectMeta{
// TODO(agamez): think about name collisions
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha - there you go :P. Yes, I guess either way (ie. even if we clean up secrets on failed installs) we'll still need to handle creating a unique name for the secret given we can't avoid users creating secrets with specific names.

Not related to this PR, but we may eventually want to support people using an existing secret for values rather than entering values.

// Cluster: &kappctrlv1alpha1.AppCluster{
// Namespace: targetNamespace,
// KubeconfigSecretRef: &kappctrlv1alpha1.AppClusterKubeconfigSecretRef{},
// },
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's Carvel's way of supporting deployments across clusters (without having kapp-controller on those other clusters). I've chatted with the carvel team about this. My concern is that with this you're relying on secrets on the cluster that give very permissive RBAC to another cluster. Their response was that this is how others (kubernetes dashboard) do things, and that the secret is no worse than a service account for this cluster (which yes, it may be in some cases limited to a specific namespace etc., but it's a level up from RBAC imo).

As it is, with our UX we'd be assuming kapp-controller on the other cluster and talking to it with the users creds as normal. I don't think we have to support this case of deploying to another cluster without kapp-controller being installed there, at least yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the context here, really useful!! When I saw it for the very first time I thought -oh, out-of-the-box multicluster! - but then I noticed it wasn't what I expected, haha.
I've added a note in the code pointing to this comment.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
but throw an error if not matching with pkg one

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Base automatically changed from carvel-10 to carvel November 24, 2021 21:20
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_ctrl_packages.go
	cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_data_adapters.go
	cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_data_resources.go
	cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia merged commit 4b68e5f into carvel Nov 24, 2021
@antgamdia antgamdia deleted the carvel-11 branch November 24, 2021 23:43
antgamdia added a commit that referenced this pull request Nov 25, 2021
…3816)

* Add required files for the Carvel plugin impl (#3783)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl (#3784)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl (#3785)

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl (#3786)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl (#3785)

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl (#3787)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl (#3785)

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageDetail' impl (#3788)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageDetail

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong secret name in test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl (#3785)

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'CreateInstalledPackage' impl (#3789)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'CreateInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageDetail

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong secret name in test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add targetCluster
but throw an error if not matching with pkg one

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestCreateInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove populated secrets in the create test. Minor renames

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'UpdateInstalledPackage' impl (#3790)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'CreateInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'UpdateInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageDetail

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong secret name in test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add targetCluster
but throw an error if not matching with pkg one

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestCreateInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add basic TestUpdateInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong pkg cluster

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove populated secrets in the create test. Minor renames

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'DeleteInstalledPackage' impl (#3791)

* Add required files for the Carvel plugin impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageVersions' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetAvailablePackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageSummaries' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetInstalledPackageDetail' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'CreateInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'UpdateInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'DeleteInstalledPackage' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove old tests. Fix TestGetClient test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetAvailablePackageVersions

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageSummaries

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetInstalledPackageDetail

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong secret name in test

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add targetCluster
but throw an error if not matching with pkg one

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestCreateInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add basic TestUpdateInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix wrong pkg cluster

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Remove populated secrets in the create test. Minor renames

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestDeleteInstalledPackage

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add 'GetPackageRepositories' impl (#3792)

* Add 'GetPackageRepositories' impl

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix nil pointer

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add TestGetPackageRepositories

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Merge manually some changes

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Changes after code review

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Fix example plugin name in values.yaml

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Add "try again" button in the installed pkgs view

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

* Initial WIP GetInstalledPackageResourceRefs

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants