Migrate namespaces retrieval from Kubeops to Kubeapps APIs #5239
Conversation
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
✅ Deploy Preview for kubeapps-dev canceled.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 1571 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 748 | 2 | 821 | 0 |
Click to see the invalid file list
- cmd/kubeapps-apis/plugins/resources/v1alpha1/common/plugin_test.go
- cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
| @@ -312,7 +307,6 @@ type handler interface { | |||
| UpdateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*v1alpha1.AppRepository, error) | |||
| RefreshAppRepository(repoName string, requestNamespace string) (*v1alpha1.AppRepository, error) | |||
| DeleteAppRepository(name, namespace string) error | |||
There was a problem hiding this comment.
just curious: do you know if we can remove more logic from this file? If so, do you plan to do so as part of the kubeops deprecation? Or should we file another issue for it?
There was a problem hiding this comment.
Exactly, from the pkg/kube we can remove a lot of stuff. That will be done after removing Kubeops in a subsequent step.
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
| } | ||
| var config resourcesConfig | ||
|
|
||
| // #nosec G304 |
There was a problem hiding this comment.
Hey @castelblanque, just for the sake of learning, I've never seen this kind of comment/annotation before, and I guess it should be processed by some kind of security linter/checker. Could you tell me what's the tool in charge of this and where is its configuration?
There was a problem hiding this comment.
This is a gosec annotation we use for ignoring false positives and wontfixes. See https://github.com/securego/gosec#annotating-code
| if err != nil { | ||
| log.Fatalf("%s", err) | ||
| } |
There was a problem hiding this comment.
Haven't you considered the use of github.com/stretchr/testify library (which I've checked is already included in the project) to ease the writing and reading of tests? This way you could replace these three lines with just a oneliner like require.NotNil(t, err)
There was a problem hiding this comment.
Definitely, I'll try to remember next time. In many cases, this block of code is just copy-pasted.
Thanks!
| } | ||
|
|
||
| // Try to list namespaces with the user token, for backward compatibility | ||
| namespaces, err := typedClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}) |
There was a problem hiding this comment.
I don't understand why context.TODO() is used here. Shouldn't we pass the parent function's context ctx or context.Background() in case we want a totally new context?
There was a problem hiding this comment.
The auth token of the request is actually handled in the s.clientGetter func of line 44. It is, in turn, handled here based on the request's context.
Commenting here your post below: parts of the code are legacy from Kubeops so we inherit some stuff, but agree with you that we should try to harmonize the usage of context.Background() and context.TODO(). The two are mainly the same (see the contexts definition here), it is regarding semantics.
Go docs say that Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter).
There was a problem hiding this comment.
I see, thanks for the explanation 🙂
| namespaces, err := typedClient.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}) | ||
| if err != nil { | ||
| if k8sErrors.IsForbidden(err) { | ||
| backgroundCtx := context.Background() |
There was a problem hiding this comment.
Also, here we're not being consistent with previous uses and are using context.Background(), and below we resort to context.TODO() again. Also, I guess you could get rid of backgroundCtx variable as it's only used in one single place, so it could be inlined.
Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com
Description of the change
Migration of the namespaces list retrieval from Kubeops to Kubeapps APIs. It resides now in the resources plugin, under the endpoint
GetNamespaceNames. Frontend has been changed to use the new endpoint too.One affected functionality is the one for trusted namespaces (#2671/#2506). Chart values for specifying header name and pattern have been moved into the resources plugin configuration section.
Benefits
We are one step closer to deprecating Kubeops.
Possible drawbacks
Backwards compatibilty issue for adopters using the trusted namespaces functionality from headers.
Applicable issues