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 explicit token for Kubeapps cluster reference #5566

Merged
merged 4 commits into from Oct 26, 2022

Conversation

castelblanque
Copy link
Collaborator

@castelblanque castelblanque commented Oct 25, 2022

Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com

Description of the change

Adapted version of the outdated PR #4591. After removing kubeops, there has been a refactor of places changes by the old PR.
It fixes the scenario in which the cluster where Kubeapps is installed is not part of the configured list of clusters.
For example:

clusters:
  - name: second-cluster
    domain: ...
    apiServiceURL: ....
    certificateAuthorityData: ...
    serviceToken: eyJhbGciOiJS...

For doing so, it uses a token to reference the Kubeapps cluster: -.
Also empty string '' is accepted as cluster reference, for allowing payloads in which there is no cluster specified in context.

Therefore, if a cluster string is empty or -, backend logic assumes that the Kubeapps cluster will be used.

Benefits

User can work in a multicluster setups in which the Kubeapps cluster is not part of the allowed clusters.

Possible drawbacks

N/A

Applicable issues

Additional information

Manual tests have been done with Helm plugin (only one working in multicluster so far).
CI E2E test will be done in #4563

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@netlify
Copy link

netlify bot commented Oct 25, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 68bf9dc
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/6358e72483d7e9000948de49

Rafa Castelblanque added 2 commits October 25, 2022 16:52
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great, thanks for the change. I guess this only is addressing the backend changes, but Anthony's PR (#4613) would still be required, wouldn't it?

Besides, don't we also need to apply these changes in other packages?
For instance: if cluster != "" (see below)

Anyway, +1ing in case you want to tackle the rest in another PR, if needed,

https://github.com/vmware-tanzu/kubeapps/blob/1-3535-customSchema/cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go#L311-L317

	cluster := request.GetContext().GetCluster()
	if cluster != "" && cluster != s.kubeappsCluster {
		return nil, status.Errorf(
			codes.Unimplemented,
			"not supported yet: request.Context.Cluster: [%v]",
			cluster)
	}

@castelblanque
Copy link
Collaborator Author

castelblanque commented Oct 26, 2022

but Anthony's PR (#4613) would still be required, wouldn't it?

Not really. It is enough with the backend handling the Kubeapps cluster name correctly, even when the cluster is not available in the list. I have tested it and are preparing a CI E2E test to avoid regression on this again.

don't we also need to apply these changes in other packages?

Not needed neither in this case:

if cluster != "" && cluster != s.kubeappsCluster {

because:

  • If cluster is empty -> OK. It will fall back to the kubeapps cluster later, inside the client getter
  • If cluster is not empty and equals the configured kubeapps cluster name (or - if none)` -> OK
  • Otherwise it will fail, which is the desired behavior

@castelblanque castelblanque merged commit bb6b78b into main Oct 26, 2022
@castelblanque castelblanque deleted the 4564-explicit-token-cluster branch October 26, 2022 14:09
castelblanque added a commit that referenced this pull request Nov 3, 2022
…ist (#5573)

### Description of the change

This PR adds an E2E test in CI to avoid regression on #5566 and hence to
avoid that #4564 happens again.

### Benefits

Kubeapps is usable even if Kubeapps cluster is not among the clusters
configured.

### Possible drawbacks

N/A

### Applicable issues

- fixes #4563

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@mecampbellsoup
Copy link
Contributor

Possible to get this fix backported? As of https://github.com/vmware-tanzu/kubeapps/releases/tag/v2.4.6 it sounds like you need k8s version > 1.21, but we're on 1.20.X and can't easily upgrade for a while.

cc @aanthonyrizzo

@absoludity
Copy link
Contributor

Possible to get this fix backported? As of https://github.com/vmware-tanzu/kubeapps/releases/tag/v2.4.6 it sounds like you need k8s version > 1.21, but we're on 1.20.X and can't easily upgrade for a while.

cc @aanthonyrizzo

We don't currently do branched releases (where we support previous releases with bug fixes etc.), so your best bet, if you were unable to upgrade your cluster (and have verified that the latest Kubeapps does not work on the older cluster - often we specify that based on the internal k8s client that we import, not because it's not compatible), would be to see if you can backport the fix to your own kubeapps-apis image to run. It's a pretty small change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] Allow package details to fetch from config.kubeappsCluster
5 participants