[repository] fixes and enhancements for repository update support #5763

dlaloue-vmware · 2022-11-30T21:39:50Z

Description of the change

This PR contains changes for two purposes:

improve feature parity between plugins
enable update support in UI

Feature parity:

flux: description support
flux: support changing to/from auth provider
carvel: description support
carvel: support changing auth type

Bug fixes to support update support in UI:

auth updates were not possible
fixed conditions for fields enablement
form fields data were sent incorrectly as a group, instead of only sending what applied
errors handling 'undefined' values
fixed which auth types are valid per plugin+type combination
fixed issue with helm plugins image pull secrets

helm/flux bug fixes:

fixed handling of redacted fields leading to secrets not being updated as expected or secret data being overwritten with 'redacted' as value.

(note: the PR has many small changes, but i was fixing issues as i was testing, hence the list)

Benefits

With the changes, users should see more commonality between plugin features, as well as a more robust UI experience regarding repository creation/updates.

Applicable issues

fixes issues #5745, #5746 and #5747

Additional information

The issue 1 in # 5746 could not be reproduce with the latest bits, so no change were made for it.

netlify · 2022-11-30T21:39:55Z

✅ Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`459dfd3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/6390dbb0a5d4290009fa38cc

gfichtenholt · 2022-12-01T00:46:34Z

just curious, is there a similar issue in flux plugin?

dlaloue-vmware · 2022-12-01T00:48:21Z

just curious, is there a similar issue in flux plugin?
yes, issue #5747

gfichtenholt · 2022-12-01T00:49:46Z

just curious, is there a similar issue in flux plugin?
yes, issue #5747

would it be ok to ask you to fix both as long as you're at it? Code should be similar

dlaloue-vmware · 2022-12-01T00:50:54Z

would it be ok to ask you to fix both as long as you're at it? Code should be similar

I don't mind.
but can it be 2 separate PRs, or you prefer me to do it in the same PR?

gfichtenholt · 2022-12-01T00:52:36Z

would it be ok to ask you to fix both as long as you're at it? Code should be similar

I don't mind. but can it be 2 separate PRs, or you prefer me to do it in the same PR?

just do it here, its like 2 more files for a grand total of 5 or 6

gfichtenholt

I am ok with this. I think there are just some linter error(s) to clean up, then you're set

dlaloue-vmware · 2022-12-02T06:55:46Z

moved to draft, as additional changes will be added

…in carvel

absoludity

Hi Dimitri

(note: the PR has many small changes, but i was fixing issues as i was testing, hence the list)

Yeah - I don't think I did a good job of reviewing this past the first 400 lines or so. I think it's pretty well established that code reviews degrade as the diff gets bigger beyond a certain point... certainly true for me. If possible in the future, try to push a PR whenever you have a small unit that can be reviewed (ie. 200-400 lines).

Anyway, I've left a few thoughts, but none of it blocking.

Please don't land this until we've cut the release - it's the kind of larger change that could have unseen impact that I'd prefer to have us dogfood for a while in dev before releasing.

Thanks!

absoludity · 2022-12-06T20:18:43Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go

@@ -40,6 +40,9 @@ const (
 	// see docs at https://fluxcd.io/docs/components/source/ and
 	// https://fluxcd.io/docs/components/helm/api/
 	fluxHelmRepositories = "helmrepositories"
+
+	// description support
+	Annotation_Description_Key = "kubeapps.dev/description"


Not important, but any reason to use snake-case here? The Go convention is to always use MixedCaps (public) or mixedCaps (private) for all identifiers (with a few exceptions).

no particular reason.
this was copied from the carvel plugin, where i introduced them that way - i think the reason is that the code already had constants with underscore though all caps.
i can easily fix that.

absoludity · 2022-12-06T21:56:11Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go

+func getDescription(fluxRepo *sourcev1.HelmRepository) string {
+	return fluxRepo.Annotations[Annotation_Description_Key]
+}


Did you mean to remove these and use the same function defined in utils? (or vice-versa?)

i don't think we have those functions in a utils file in flux plugin or in a shared utils

It's in the kapp_controller/server_utils.go that you've added below (I hadn't noticed earlier that that was specific to kapp_controller). Just wondering if it should be DRY'd up and used just once... perhaps in plugins/pkg/k8sutils/k8sutils.go? Fine either way.

I will make the code common, that is the right thing to do.

absoludity · 2022-12-06T22:06:32Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go

+		if provider != "" && provider != "generic" {
+			if request.Auth != nil && request.Auth.Type != corev1.PackageRepositoryAuth_PACKAGE_REPOSITORY_AUTH_TYPE_UNSPECIFIED {
+				return nil, status.Errorf(codes.InvalidArgument, "Auth provider cannot be configured in combination with another auth method")
+			}
+			if repo.Spec.Provider != "" && repo.Spec.Provider != "generic" && repo.Spec.Provider != provider {
+				return nil, status.Errorf(codes.InvalidArgument, "Auth provider cannot be changed.")
+			}
+			repo.Spec.Provider = provider
+		} else {
+			repo.Spec.Provider = ""
+		}


I'm just trying to understand some of the logic here: on lines 487 we explicitly disallow the provider from being changed if it has been set, but on line 491 we allow it to be set to empty. So, unless I'm missing something, users can change the auth provider, they just need to first set it to empty then set it?

The idea was that changing the provider did not really make sense (the original code was completely ignoring the new value) so we are explicitly rejecting such change.
The main reason to allow to unset the provider is that the user may have tried with the provider and it did not work and as such is switching back to using basic auth for example.
But yes, a side effect would be that you could unset the provider and switch to another one in the next update.

absoludity · 2022-12-06T22:13:09Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_auth.go

+			if existingSecret != nil {
+				if err = secretInterface.Delete(ctx, existingSecret.Name, metav1.DeleteOptions{}); err != nil {
+					log.Errorf("Error deleting existing secret: [%s] due to %v", err)
+				}
+			}


I don't remember how the discussions ended, but I thought in the past we'd talked about the fact that we should not delete a secret unless it was created by kubeapps? That is, if I create a repo in Kubeapps (perhaps via the CLI, not relevant) and associate an existing secret with that repo, what should happen if I then update the repo credentials via the Kubeapps UI.

Also wondering, in what case should the error when deleting the secret here not be raised? Why is it not raised here?

It is only deleting it because this is a secret created by kubeapps. we are not deleting secrets that kubeapps has not created.
As per the spec, there is a validation that ensures that we are not switching 'mode' between user managed and kubeapps managed secrets. So in this code path, we know this is a kubeapps managed secret.

absoludity · 2022-12-06T22:23:24Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_auth.go

+			if (ck.Cert == redactedString && !hadSecretTlsCert) || (ck.Key == redactedString && !hadSecretTlsKey) {
+				return nil, false, status.Errorf(codes.InvalidArgument, "Invalid configuration, unexpected REDACTED content")
+			}


Thanks for all these checks - ensuring that we'll know if the UX sends the incorrect redacted content (I assume?).

absoludity · 2022-12-06T22:34:06Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_auth.go

+	for server, entry := range config.Auths {
+		docker := &corev1.DockerCredentials{
+			Server:   server,
+			Username: entry.Username,
+			Password: entry.Password,
+			Email:    entry.Email,
+		}
+		return docker, nil
+	}


I don't understand why you're using a for loop over the range of config.Auths here? It reads as though you're just returning after using the first of config.Auths always? But even then, there is no concept of "first" for a map in go (config.Auths is a map[string]DockerConfigEntry I think?) - the order when you iterate a map in go is explicitly random.

I reused existing code. i.e. that is how Kubeapps has been handling AppRepository image pull secrets and docker config, assuming there was only one even though the struct is a collection.
is there a better way to take one item from a map than this kind of loop?

The normal way to get ordered output from a map would be to get the keys and order those, but yeah, not much point if the assumption here is that there's only a single credential. It'd be worth a comment so that it's clear what the assumption is (here and in the other code where you found it, if it's not already commented there, it should be).

i will add a comment.
I will try to find the other code assuming a single credentials, but i don't want to bloat the PR even more :-)

absoludity · 2022-12-06T22:45:41Z

dashboard/src/components/Config/PkgRepoList/PkgRepoForm.tsx

-        finalHeader = `Bearer ${bearerToken}`;
+        finalHeader = bearerToken;


This change is surprising? Was it a bug currently, or why is the Bearer not included now?

i think this was a left-over bug from when the UI was switched to the new API.
The UI code was not changed to comply with what the spec says. The UI was sending the token prefixed with Bearer, and the backend would strip the prefix if present, only (for helm) to add it back before putting it in the secret.
On a read, the backend would always send the token only, without the Bearer prefix (as per the spec). So the UI was inconsistent between its read and write operations.
this is double defensive and inconsistent code that makes things so hard to read and understand, so i cleaned it up.

dlaloue-vmware · 2022-12-07T02:24:08Z

to avoid making even more changes to this PR, is it ok to push the cleanups to another PR?

absoludity · 2022-12-07T02:50:15Z

to avoid making even more changes to this PR, is it ok to push the cleanups to another PR?

Doesn't matter to me either way (it's already huge - but whatever works for you). It'd just be helpful not to land anything until we're confident the current release is good. So if it's not landing, it might be simpler just to update this one with any small cleanups (and anyway, the DRYing up of your two functions will reduce the diff :) )

dlaloue-vmware added 2 commits November 30, 2022 13:09

fixes

75a419a

Merge remote-tracking branch 'origin' into issue5746

c74c978

vmwclabot added the cla-not-required label Nov 30, 2022

dlaloue-vmware requested a review from gfichtenholt November 30, 2022 21:40

fixes for flux

d3f3c63

gfichtenholt approved these changes Dec 2, 2022

View reviewed changes

dlaloue-vmware marked this pull request as draft December 2, 2022 06:54

dlaloue-vmware added 5 commits December 2, 2022 10:05

carvel repos now allow changing auth type

7332d4f

added description support for carvel repositories

5faa8bf

ui fixes related to flux support, plus flux support enhancements

e35dd3d

fixes for ui and backend to support auth type change and description …

37d7231

…in carvel

fixed ui tests

26552f4

dlaloue-vmware changed the title ~~Issue5746 - secrets not properly updated if auth is partially redacted~~ [repository] fixes and enhancements for repository update support Dec 6, 2022

dlaloue-vmware marked this pull request as ready for review December 6, 2022 06:37

dlaloue-vmware requested a review from absoludity December 6, 2022 06:38

absoludity reviewed Dec 6, 2022

View reviewed changes

implemented code review suggestions

459dfd3

dlaloue-vmware merged commit ed020ef into vmware-tanzu:main Dec 15, 2022

dlaloue-vmware deleted the issue5746 branch December 15, 2022 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[repository] fixes and enhancements for repository update support #5763

[repository] fixes and enhancements for repository update support #5763

dlaloue-vmware commented Nov 30, 2022 •

edited

netlify bot commented Nov 30, 2022 •

edited

gfichtenholt commented Dec 1, 2022

dlaloue-vmware commented Dec 1, 2022

gfichtenholt commented Dec 1, 2022

dlaloue-vmware commented Dec 1, 2022 •

edited

gfichtenholt commented Dec 1, 2022

gfichtenholt left a comment

dlaloue-vmware commented Dec 2, 2022

absoludity left a comment

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022 •

edited

absoludity Dec 7, 2022

dlaloue-vmware Dec 7, 2022

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022

absoludity Dec 6, 2022

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022

absoludity Dec 7, 2022

dlaloue-vmware Dec 7, 2022

absoludity Dec 6, 2022

dlaloue-vmware Dec 6, 2022

dlaloue-vmware commented Dec 7, 2022

absoludity commented Dec 7, 2022

		finalHeader = `Bearer ${bearerToken}`;
		finalHeader = bearerToken;

[repository] fixes and enhancements for repository update support #5763

[repository] fixes and enhancements for repository update support #5763

Conversation

dlaloue-vmware commented Nov 30, 2022 • edited

Description of the change

Benefits

Applicable issues

Additional information

netlify bot commented Nov 30, 2022 • edited

✅ Deploy Preview for kubeapps-dev canceled.

gfichtenholt commented Dec 1, 2022

dlaloue-vmware commented Dec 1, 2022

gfichtenholt commented Dec 1, 2022

dlaloue-vmware commented Dec 1, 2022 • edited

gfichtenholt commented Dec 1, 2022

gfichtenholt left a comment

Choose a reason for hiding this comment

dlaloue-vmware commented Dec 2, 2022

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlaloue-vmware Dec 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlaloue-vmware commented Dec 7, 2022

absoludity commented Dec 7, 2022

dlaloue-vmware commented Nov 30, 2022 •

edited

netlify bot commented Nov 30, 2022 •

edited

dlaloue-vmware commented Dec 1, 2022 •

edited

dlaloue-vmware Dec 6, 2022 •

edited