Add --context flag #197

maruina · 2023-02-02T09:53:50Z

@vladimirvivien I know this is not what you suggested, but I spent some time looking at the cli-runtime and couldn't figure out how to wire it.
Since this is blocking our adoption and it is a pretty simple change, I'd like to ask you if we can still merge it and then I can try to take again a look at the cli-runtime fix

k8s-ci-robot · 2023-02-02T09:53:59Z

Welcome @maruina!

It looks like this is your first PR to kubernetes-sigs/e2e-framework 🎉. 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/e2e-framework 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 · 2023-02-02T09:53:59Z

Hi @maruina. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

vladimirvivien

@maruina Hi. Thanks for the contribution. This is a good start.
I left some comments about the context name. Also, I don't see where in the code you are passing the kubernetes context to the the client. I imagine that is what you want, correct ?

vladimirvivien · 2023-02-03T01:09:55Z

pkg/envconf/config.go

@@ -44,6 +44,7 @@ type Config struct {
 	dryRun                  bool
 	failFast                bool
 	disableGracefulTeardown bool
+	context                 string


To avoid confusion with Go context, I would suggest naming this k8sContext

We have a kubeconfig, what do you think about kubecontext? I am asking so we have only one way to prefix kubernetes things

vladimirvivien · 2023-02-03T01:11:07Z

pkg/flags/flags.go

@@ -175,6 +181,11 @@ func (f *EnvFlags) DisableGracefulTeardown() bool {
 	return f.disableGracefulTeardown
 }

+// Context returns an optional kubeconfig context to use
+func (f *EnvFlags) Context() string {


Same here, I would suggest naming the method KubernetesContext

maruina · 2023-02-03T09:01:32Z

Also, I don't see where in the code you are passing the kubernetes context to the the client. I imagine that is what you want, correct ?

No :P
You are right and having a better look at the code, correct me where I'm wrong:

we build the client with NewClient() where we pass the kubeconfig we got from NewFromFlags
Then we call NewWithKubeConfigFile which is calling conf.New(filePath)
If I look at the klient/conf/config.go I see we already have a NewWithContextName, but it seems we don't use it anywhere

What I could do is:

check ifc.kubecontext is empty or not in NewClient()
call something like NewWithKubeConfigFileAndContext() which then calls NewWithContextName

It sounds a little convoluted so I'm not 100% how would you like to do it or if you have a better suggestion

vladimirvivien · 2023-02-03T12:22:03Z

@maruina oh yes, you are right. There is already a method that can apply the Kubernetes cluster context at test time.

So, my next question to you, do you have to pass your cluster context name as a CLI argument ? You could use an environment variable (i.e. KUBE_CONTEXT) and read it from your test, then apply that method. That way you won't even have to wait for PR or a code change.

vladimirvivien · 2023-02-03T12:56:00Z

@maruina I went and look at the code again. If you definitely want to move with this, introduce a --context as a flag, there are some additional steps you will need for it to work. I think you are almost there, but here is the complete path of changes you need:

Add/define flag in pkg/flags - done
Update the envconf.Config with the context config (i.e. kubeContext field).
Next update envconf.NewFromFlags so that the environment can be created with the new flag
Next, add a pair of accessor methods Config.WithKubernetesContext and Config.KubernetesContext similar to how it is done for Namespace here
Update Config.NewClient() to create the klient.Client with the cluster name, when the kubeConfig is provided.

It sounds complicated, but we are here to assist you in getting this in.

cc @harshanarayana

maruina · 2023-02-03T13:52:26Z

So, my next question to you, do you have to pass your cluster context name as a CLI argument ? You could use an environment variable (i.e. KUBE_CONTEXT) and read it from your test, then apply that method. That way you won't even have to wait for PR or a code change.

Correct me if I'm wrong, but in the test we usually call cfg.NewClient() where cfg is a *envconf.Config. So I would still need to make some changes, or I'm not 100% sure what you mean with apply that method

Regarding the flag, thank you very much for your input, I will follow up with your suggestions

maruina · 2023-02-03T14:20:14Z

@vladimirvivien I think that I followed your suggestions up to this point

Update Config.NewClient() to create the klient.Client with the cluster name, when the kubeConfig is provided.

When I look at the code, we call client, err := klient.NewWithKubeConfigFile(c.kubeconfig) to create a client with a specific kubeconfig, so my understanding is that I need to provide a new method or change NewWithKubeConfigFile.

Do you want me to check if c.kubecontext is not an empty string inside NewClient()? The check we do for the kubeconfig is later in the code, when we call conf.New(filePath)

So I can call NewWithContextName(fileName, context string) but I probably need something like

client, err := klient.NewWithKubeConfigFileAndKubeContext(c.kubeconfig, c.kubecontext)

but again, it feels a little too convoluted. What am I missing?

vladimirvivien · 2023-02-03T15:46:12Z

So I can call NewWithContextName(fileName, context string) but I probably need something like
client, err := klient.NewWithKubeConfigFileAndKubeContext(c.kubeconfig, c.kubecontext)

@maruina I think you are getting closer. In Config.NewClient you could check to see if the context is non-empty then create the client as shown below. You can create a new constructor function klient.NewWithContextName(file, string) that uses klient/conf.NewWithContextName() to build the klient.Client

func (c *Config) NewClient() (klient.Client, error) {
	if c.client != nil {
		return c.client, nil
	}

        if c.kubecontext != "" {
            client, err := klient.NewWithContextName(c.kubeconfig, c.kubeContext)
	    if err != nil {
		return nil, fmt.Errorf("envconfig: client failed: %w", err)
	    } 
           c.client = client
        }else{
	   client, err := klient.NewWithKubeConfigFile(c.kubeconfig)
	   if err != nil {
  	       return nil, fmt.Errorf("envconfig: client failed: %w", err)
	   }
	   c.client = client
       }

	return c.client, nil
}

harshanarayana · 2023-02-04T15:53:17Z

@vladimirvivien could we not just offload all calls from NewClient to the klient.NewWithContextNam and handle the cases there where the context is set of not? Since that is really a klient related case than the Config is it not ?

harshanarayana · 2023-02-05T13:49:01Z

@maruina

Sorry in advance for the really lengthy comment.

If you check the args generated for the binaries of e2e-framework, it already has a -kube-context arg. May be we should consider reusing that instead of adding a new one?

❯ ./flags.test --help 2>&1 | grep -A2 kube-cont
  -kube-context string
    	Used to provide a custom KubeContext override for the file passed to the --kubeconfig
  -kubeconfig string

e2e-framework/klient/conf/config.go

Lines 112 to 122 in cd133e2

    
           func ResolveClusterContext() string { 
        
           	// If a flag --kube-context is specified use that 
        
           	if flag.Parsed() { 
        
           		f := flag.Lookup("kube-context") 
        
           		if f != nil { 
        
           			return f.Value.String() 
        
           		} 
        
           	} 
        
           	return "" 
        
           }

I think we should reuse this and modify the workflow as bit in terms of how the client is created.

func New(fileName string) (*rest.Config, error) {
	// if filename is not provided assume in-cluster-config
	if fileName == "" {
		return rest.InClusterConfig()
	}

	if kubeContext := ResolveClusterContext(); kubeContext != "" {
		return NewWithContextName(fileName, kubeContext)
	}
	
	// create the config object from k8s config path
	return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
		&clientcmd.ClientConfigLoadingRules{ExplicitPath: fileName}, &clientcmd.ConfigOverrides{}).ClientConfig()
}

This should keep the code change to really minimal ? cc @vladimirvivien

If we are to add a new one, I think here are a few suggestion.

func NewWithKubeConfigFileAndContext(kubeConfig string, kubeContext string) (Client, error) {
	if kubeContext == "" {
		return NewWithKubeConfigFile(kubeConfig)
	}
	cfg, err := conf.NewWithContextName(kubeConfig, kubeContext)
	if err != nil {
		return nil, err
	}
	return New(cfg)
}

I think we can add a function like so on the pkg/klient/client.go and call that from pkg/envconf/config.go like following

func (c *Config) NewClient() (klient.Client, error) {
	if c.client != nil {
		return c.client, nil
	}

	client, err := klient.NewWithKubeConfigFileAndContext(c.kubeconfig, c.kubeContext)
	if err != nil {
		return nil, fmt.Errorf("envconfig: client failed: %w", err)
	}
	c.client = client

	return c.client, nil
}

This will keep the isolation intact to a good extend.

Also a few other things to consider,

If the --kubeconfig arg is not provided but a --kube-context is provided, then we should probably throw an error ?
I think it is better to name the arg --kube-context instead of --kubecontext for readability.
Using proper camel cases in the code. Please consider using kubeContext instead of kubecontext
Since we are looking into it, I think the signature of func NewWithContextName(fileName, context string) (*rest.Config, error) might be better changed a bit on the variable name ? The second one reads context which is really conflicting with the default package. Might be better to rename this kubeContext ?

maruina · 2023-02-06T10:53:42Z

Hello @harshanarayana, thank you very much for your help.
I personally like your approach to keep the code change at the minimum and use ResolveClusterContext.
If @vladimirvivien is happy with that, I'm happy to change the PR and push that code.

Otherwise, I can follow your suggestion and go with the NewWithKubeConfigFileAndContext route

maruina · 2023-02-07T15:16:47Z

I still pushed the change so if we are happy with that, we can test it :)
I can always revert to the previous state

@harshanarayana from which branch did you try the flags? I'm trying with the latest main but it seems I can't see that kube-context flag

❯ pwd
~/go/src/github.com/kubernetes-sigs/e2e-framework/examples/flags
❯ git log -1
commit 11aa681198560751f4694b0ae613d580bbb50012 (HEAD -> main, origin/main, origin/HEAD)
Merge: 33c3d5e 35ab1e9
Author: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Date:   Fri Feb 3 04:08:30 2023 -0800

    Merge pull request #196 from embano1/issue-194

    fix: Parse multiple label values for same key
❯ go test -c -o flags.test .
❯ ./flags.test
PASS
❯ ./flags.test --help 2>&1 | grep -A2 kube
  -kubeconfig string
    	Path to a cluster kubeconfig file (optional)

harshanarayana · 2023-02-08T03:39:44Z

@harshanarayana from which branch did you try the flags? I'm trying with the latest main but it seems I can't see that kube-context flag

Oh no! I think I made a mistake. I'm terribly sorry about that.. I was on your PR branch and had modified the arg name.. I totally forgot about that when I ran the help command.. That's how the variable showed up there... You still have to add that arg yourself..

maruina · 2023-02-08T09:50:41Z

@harshanarayana no worries. I pushed again the flag, and I was able to target a specific cluster using

go test . -v -args --kubeconfig ~/.kube/config --labels category=mylabel --kube-context mycontext

vladimirvivien

Thanks @maruina for this awesome contribution.

maruina · 2023-02-17T13:11:22Z

/retest

k8s-ci-robot · 2023-02-17T13:11:36Z

@maruina: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

vladimirvivien · 2023-02-17T13:46:40Z

/ok-to-test

harshanarayana · 2023-02-17T14:22:15Z

@maruina

go: downloading github.com/josharian/intern v1.0.0
E0217 13:49:12.378771    9620 env.go:352] Recovering from panic and running finish actionsenvconfig: client failed: open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory
program not built with -cover
ok  	sigs.k8s.io/e2e-framework/examples/crds	41.446s
=== RUN   TestListPods
=== RUN   TestListPods/pod_list
=== RUN   TestListPods/pod_list/pods_from_kube-system
    k8s_test.go:33: envconfig: client failed: open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory
--- FAIL: TestListPods (0.00s)
    --- FAIL: TestListPods/pod_list (0.00s)
        --- FAIL: TestListPods/pod_list/pods_from_kube-system (0.00s)
FAIL
coverage: [no statements]
FAIL	sigs.k8s.io/e2e-framework/examples/custom_env_funcs	38.357s
E0217 13:50:14.534158   15810 env.go:352] Recovering from panic and running finish actionsenvconfig: client failed: open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory
program not built with -cover
ok  	sigs.k8s.io/e2e-framework/examples/decoder	20.[72](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_e2e-framework/197/pull-e2e-framework-test/1626578895165722624#1:build-log.txt%3A72)6s

Seems like new changes are breaking the tests

maruina · 2023-02-17T14:25:31Z

@vladimirvivien I think that I actually broke something, let me see where the problem is because I think I tested everything before adding the two new methods so I'm not sure how I broke things

klient/conf/config.go

harshanarayana · 2023-02-17T14:35:13Z

/hold

Add --kube-context flag Require --kubeconfig when passing --kube-context Add --kube-context documentation Update klient/conf/config.go Co-authored-by: Harsha Narayana <harsha2k4@gmail.com> Update klient/conf/config.go Co-authored-by: Harsha Narayana <harsha2k4@gmail.com> Fix indentation Use kubeContext instead of context to avoid clashing

vladimirvivien · 2023-02-17T15:28:08Z

@maruina ping me when you think you got it fixed.

maruina · 2023-02-17T15:39:57Z

@vladimirvivien all the tests are passing locally, can you please retest?

vladimirvivien · 2023-02-17T16:20:32Z

/retest

harshanarayana

/lgtm

k8s-ci-robot · 2023-02-17T16:47:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshanarayana, maruina, vladimirvivien

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~~ [harshanarayana,vladimirvivien]

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

harshanarayana · 2023-02-17T16:48:43Z

/hold cancel

vladimirvivien · 2023-02-17T17:05:19Z

/lgtm
/approved

vladimirvivien · 2023-02-17T18:33:30Z

Thanks @maruina !!

maruina · 2023-02-17T18:38:18Z

Thanks both of you for your patience and help

k8s-ci-robot requested a review from ShwethaKumbla February 2, 2023 09:53

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Feb 2, 2023

k8s-ci-robot requested a review from vladimirvivien February 2, 2023 09:53

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 2, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 2, 2023

vladimirvivien reviewed Feb 3, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2023

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 3, 2023

maruina force-pushed the cli-runtime branch from c48bd95 to 5947b23 Compare February 7, 2023 15:16

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 7, 2023

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 Feb 8, 2023

vladimirvivien approved these changes Feb 17, 2023

View reviewed changes

k8s-ci-robot assigned vladimirvivien Feb 17, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 17, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 17, 2023

harshanarayana requested changes Feb 17, 2023

View reviewed changes

klient/conf/config.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2023

maruina force-pushed the cli-runtime branch from f2d2b80 to 0855566 Compare February 17, 2023 14:56

harshanarayana approved these changes Feb 17, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2023

k8s-ci-robot merged commit 0b8b30c into kubernetes-sigs:main Feb 17, 2023

maruina deleted the cli-runtime branch February 17, 2023 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --context flag #197

Add --context flag #197

maruina commented Feb 2, 2023

k8s-ci-robot commented Feb 2, 2023

k8s-ci-robot commented Feb 2, 2023

vladimirvivien left a comment

vladimirvivien Feb 3, 2023

maruina Feb 3, 2023

vladimirvivien Feb 3, 2023

maruina commented Feb 3, 2023

vladimirvivien commented Feb 3, 2023

vladimirvivien commented Feb 3, 2023

maruina commented Feb 3, 2023

maruina commented Feb 3, 2023 •

edited

Loading

vladimirvivien commented Feb 3, 2023

harshanarayana commented Feb 4, 2023

harshanarayana commented Feb 5, 2023

maruina commented Feb 6, 2023

maruina commented Feb 7, 2023 •

edited

Loading

harshanarayana commented Feb 8, 2023

maruina commented Feb 8, 2023

vladimirvivien left a comment

maruina commented Feb 17, 2023

k8s-ci-robot commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

harshanarayana commented Feb 17, 2023 •

edited

Loading

maruina commented Feb 17, 2023

harshanarayana commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

maruina commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

harshanarayana left a comment •

edited

Loading

k8s-ci-robot commented Feb 17, 2023

harshanarayana commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

maruina commented Feb 17, 2023

Add --context flag #197

Add --context flag #197

Conversation

maruina commented Feb 2, 2023

k8s-ci-robot commented Feb 2, 2023

k8s-ci-robot commented Feb 2, 2023

vladimirvivien left a comment

Choose a reason for hiding this comment

vladimirvivien Feb 3, 2023

Choose a reason for hiding this comment

maruina Feb 3, 2023

Choose a reason for hiding this comment

vladimirvivien Feb 3, 2023

Choose a reason for hiding this comment

maruina commented Feb 3, 2023

vladimirvivien commented Feb 3, 2023

vladimirvivien commented Feb 3, 2023

maruina commented Feb 3, 2023

maruina commented Feb 3, 2023 • edited Loading

vladimirvivien commented Feb 3, 2023

harshanarayana commented Feb 4, 2023

harshanarayana commented Feb 5, 2023

maruina commented Feb 6, 2023

maruina commented Feb 7, 2023 • edited Loading

harshanarayana commented Feb 8, 2023

maruina commented Feb 8, 2023

vladimirvivien left a comment

Choose a reason for hiding this comment

maruina commented Feb 17, 2023

k8s-ci-robot commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

harshanarayana commented Feb 17, 2023 • edited Loading

maruina commented Feb 17, 2023

harshanarayana commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

maruina commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

harshanarayana left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2023

harshanarayana commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

vladimirvivien commented Feb 17, 2023

maruina commented Feb 17, 2023

maruina commented Feb 3, 2023 •

edited

Loading

maruina commented Feb 7, 2023 •

edited

Loading

harshanarayana commented Feb 17, 2023 •

edited

Loading

harshanarayana left a comment •

edited

Loading