[clusterctl] Fix ensure namespace function in clusterclient #1209

eromanova · 2019-07-30T09:01:47Z

Is needed to avoid possible tracebacks due to auth permission (if user does not have permission to create or get namespaces)

Fixes #1208

k8s-ci-robot · 2019-07-30T09:01:48Z

Welcome @eromanova!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-07-30T09:02:02Z

Hi @eromanova. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

eromanova · 2019-07-30T09:18:18Z

/assign @justinsb

ncdc

@eromanova thank you for your PR! I added a couple of comments.

ncdc · 2019-07-30T13:31:01Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

 		return errors.Wrap(err, "error creating core clientset")
 	}

+	namespaces, err := clientset.CoreV1().Namespaces().List(metav1.ListOptions{})


Please use Get instead of List to check for a single namespace. You can use apierrors.IsNotFound(err) to distinguish between a 404 not found and other errors.

ncdc · 2019-07-30T13:31:53Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

 		},
 	}
 	_, err = clientset.CoreV1().Namespaces().Create(&namespace)
-	if err != nil && !apierrors.IsAlreadyExists(err) {


I would leave in the check for existence. There is a slim chance that the Get call could return a 404 not found, then something else creates the namespace, and then this line tries to create it.

detiber

@eromanova Thank you for the contribution, a couple of comments related to the new check.

detiber · 2019-07-30T13:37:21Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

 		return errors.Wrap(err, "error creating core clientset")
 	}

+	namespaces, err := clientset.CoreV1().Namespaces().List(metav1.ListOptions{})


Any particular reason to use List here instead of Get?

This is also missing a if err != nil check. It might be good to test the error against apierrors.IsForbidden to see if the user is also missing permissions to List or Get Namespaces, in which case we can likely emit a warning to the user.

eromanova · 2019-07-30T15:21:37Z

@detiber @ncdc Thank you for your comments, guys. The reason I used List instead of Get is possible missing Get permissions for user. But such case when user does not have permissions to List, but can Get namespaces should also be handled. I've made changes to this PR according to your comments, please take a look one more time. What do you think about this approach?

ncdc · 2019-07-30T15:22:20Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go


+	_, err = clientset.CoreV1().Namespaces().Get(namespaceName, metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsAlreadyExists(err) {


You won't get this error from a Get call

ncdc · 2019-07-30T15:24:16Z

@eromanova I'm curious to know when/where you ran into a situation where you couldn't list or get namespaces? Is that common?

eromanova · 2019-07-31T08:42:01Z

@ncdc I'm not so sure that it is a common case. But I had to deal with that in my environment (strong restrictions for user, only list namespaces permissions). I guess, the absence of one of these permissions is not the reason to fail ensureNamespace.

ncdc · 2019-08-05T17:10:58Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

-	_, err = clientset.CoreV1().Namespaces().Create(&namespace)
-	if err != nil && !apierrors.IsAlreadyExists(err) {
-		return err
+	_, err = clientset.CoreV1().Namespaces().Get(namespaceName, metav1.GetOptions{})


How about restructuring it like this?

// Happy path - return quickly if err == nil { return nil } if apierrors.IsForbidden(err) { // get was forbidden. see if list works namespaces, err := clientset.CoreV1().Namespaces().List(metav1.ListOptions{}) if apierrors.IsForbidden(err) { // either return err, which means we halt work // or return nil, and either things will be ok (user has permissions to create CAPI resources but they can't check for namespace existence), or things won't (they can't create CAPI resources, or the namespace doesn't exist) } if err != nil { return err } // check the list } if !apierrors.IsNotFound(err) { // err is not Forbidden or NotFound - return it return err } // try to create // ...

@ncdc I'm sorry for long delay. Thank you so much for the proposal. Could you please take a look again.

vincepri · 2019-08-13T14:59:52Z

@eromanova Did you have a chance to look at @ncdc's feedback?

Is needed to avoid possible tracebacks due to auth permission Change-Id: Id9e197be1fee6bb300cb2681a81b7f65bdd27d74

eromanova · 2019-08-13T16:39:42Z

@vincepri I'm sorry it's been so long. I updated this PR, could you please take a look. Thanks in advance.

detiber · 2019-08-13T16:43:33Z

/ok-to-test
/lgtm
/assign @vincepri

vincepri

/approve

k8s-ci-robot · 2019-08-13T18:34:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eromanova, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2019-08-13T18:35:04Z

/test pull-cluster-api-test

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 30, 2019

k8s-ci-robot requested review from detiber and ncdc July 30, 2019 09:02

k8s-ci-robot assigned justinsb Jul 30, 2019

ncdc suggested changes Jul 30, 2019

View reviewed changes

detiber reviewed Jul 30, 2019

View reviewed changes

eromanova force-pushed the ensure-namespace branch from 1bfbd78 to d6cc8dc Compare July 30, 2019 15:07

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 30, 2019

eromanova force-pushed the ensure-namespace branch from d6cc8dc to 6d874f4 Compare July 30, 2019 15:10

ncdc reviewed Jul 30, 2019

View reviewed changes

eromanova force-pushed the ensure-namespace branch from 6d874f4 to 2356b57 Compare July 31, 2019 08:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 31, 2019

ncdc reviewed Aug 5, 2019

View reviewed changes

Fix ensure namespace function in clusterclient

8f67f5d

Is needed to avoid possible tracebacks due to auth permission Change-Id: Id9e197be1fee6bb300cb2681a81b7f65bdd27d74

eromanova force-pushed the ensure-namespace branch from 2356b57 to 8f67f5d Compare August 13, 2019 16:31

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 13, 2019

k8s-ci-robot assigned vincepri and detiber Aug 13, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2019

vincepri approved these changes Aug 13, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2019

k8s-ci-robot merged commit b7be135 into kubernetes-sigs:master Aug 13, 2019

eromanova deleted the ensure-namespace branch August 14, 2019 07:14

[clusterctl] Fix ensure namespace function in clusterclient #1209

[clusterctl] Fix ensure namespace function in clusterclient #1209

Uh oh!

Conversation

eromanova commented Jul 30, 2019

Uh oh!

k8s-ci-robot commented Jul 30, 2019

Uh oh!

k8s-ci-robot commented Jul 30, 2019

Uh oh!

eromanova commented Jul 30, 2019

Uh oh!

ncdc left a comment

Choose a reason for hiding this comment

Uh oh!

ncdc Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

ncdc Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

detiber left a comment

Choose a reason for hiding this comment

Uh oh!

detiber Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

eromanova commented Jul 30, 2019

Uh oh!

ncdc Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

ncdc commented Jul 30, 2019

Uh oh!

eromanova commented Jul 31, 2019

Uh oh!

ncdc Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

eromanova Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

vincepri commented Aug 13, 2019

Uh oh!

eromanova commented Aug 13, 2019

Uh oh!

detiber commented Aug 13, 2019

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 13, 2019

Uh oh!

vincepri commented Aug 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants