✨ pass config between KB and external plugin #3526

Eileen-Yu · 2023-08-07T23:20:52Z

Description:

Todo:

Add new specs Config in both the PluginRequest and PluginResponse
Create a local type definition in the sampleexternalplugin path for easier testing
Inject the config in PluginRequest and pass it to the external plugin
Update the config on the sample external plugin side, send the updated config back via response
Enable KB to update the config based on the modified the config passed back from PluginResponse
Add e2e tests
Remove the replace once the PR gets merged (next PR)

Sample PROJECT updated by go sample external plugin:

Motivation:

Aims to fix the issue #3396

k8s-ci-robot · 2023-08-07T23:20:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/plugins/external/init.go

camilamacedo86 · 2023-08-08T07:07:55Z

pkg/plugins/external/init.go

+func (p *initSubcommand) InjectConfig(c config.Config) error {
+	p.config = c
+	return nil
+}


However, to ensure that works could we add some tests to this one?

In the e2e test that we have now, could we not validate the config is filled and with the data that is expected?
Also, could we try to pass a value unto the config and see if that will be persisted in the PROJECT file?

See here how we update the config with the data to have it in tracked in the PROJECT file: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/deploy-image/v1alpha1/api.go#L221-L247

Note that this code is reponsable for we track: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/PROJECT#L8-L25

docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/flags.go

docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/metadata.go

.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/external/types.go

everettraven · 2023-08-15T13:44:58Z

pkg/plugin/external/types.go

@@ -35,6 +35,9 @@ type PluginRequest struct {
 	// Universe represents the modified file contents that gets updated over a series of plugin runs
 	// across the plugin chain. Initially, it starts out as empty.
 	Universe map[string]string `json:"universe"`
+
+	// Config stores the project configuration file.
+	Config string `json:"config"`


I'm curious, why a string type? Do we have any other types we can use here to make it more explicit what the allowed content is?

@camilamacedo86 Fix me if I'm missing something or understanding wrong:

I'm wondering if a literal string of the config content can help make it more general when parsing the data structure from different programs.

I'm also wondering how we'd like to define a data structure for config?
Shall we directly import the definition from v3/config?
One question here is what exactly content/message a general external plugin will use from the config?
How much do we allow the external plugin to modify the config?

The biggest concern I have with a string literal of JSON is how do we perform validation on it? How do we ensure that the plugin isn't attempting to use configuration fields that don't exist or they have typos?

I'm wondering if we can have a number of clear criteria to help move on? More specifically, I'm curious if we can answer the following questions before implementation:

What exactly data structure do we want to transfer for config? (Does it have to be a go struct? If yes, then what is the relationship between such with private cfg in config v2/v3?)

Is it necessary to define new methods for Config interface so as to help generate the data that might be transferred with the ep?

How does it affect PluginRequest and PluginResponse?

What are the boundaries for an ep to touch with config? (Is the config read-only? OR what exactly fields an ep can and cannot touch with?)

Is it necessary to ALWAYS pass the config to an EP? (Do we need to introduce a mechanism to make it optional?)

@Eileen-Yu

Here, I think, should be config.Config

Kavinjsir

Suggestion: can we hold the implementation until we come up with clear definitions / criteria.

k8s-ci-robot · 2023-11-15T23:48:47Z

@Eileen-Yu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-kubebuilder-e2e-k8s-1-26-6
/test pull-kubebuilder-e2e-k8s-1-27-3
/test pull-kubebuilder-e2e-k8s-1-28-0
/test pull-kubebuilder-test

Use /test all to run all jobs.

In response to this:

/test pull-kubebuilder-e2e-k8s-1-26-1

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.

Eileen-Yu · 2023-11-15T23:49:36Z

/test pull-kubebuilder-e2e-k8s-1-26-6

Eileen-Yu · 2023-11-16T00:08:50Z

/test pull-kubebuilder-e2e-k8s-1-28-0

camilamacedo86

@Eileen-Yu

This one I think is very close in this one
It seems for me that we here :

a) Change:

// Config stores the project configuration file.
	Config string `json:"config"`

To use config.Config instead of string.

b) Ideally add a test.go for /pkg/plugin/external/types.go . See the other unit tests as example: https://github.com/kubernetes-sigs/kubebuilder/blob/924daa742ac59c9728572396658aaae3199b770f/pkg/config/store/errors_test.go

c) Here instead of re-create the implementation: https://github.com/kubernetes-sigs/kubebuilder/pull/3526/files#diff-e6ed63afd007e4f9eacc0376b867bef072ed4b39eb92cf3e9d683f6d87ffabcf we would use the struct that we have already so that we do not need to keep both places maintained. We can create the interface using the current struct

k8s-ci-robot · 2024-02-06T07:55:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Eileen-Yu, Kavinjsir
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Eileen-Yu · 2024-02-13T04:59:20Z

Tried to update it with the suggestions.. Is this what we're looking for?

Guess I meet with the similar blocker when I did this last time: the project version would somehow be 0 (but by default it should be 3, and it did show as 3 in the scaffolded PROJECT, but if print it in the middle it's actually 0... not quite sure what caused this), and PROECT content failed to update with new fields... but these issues wouldn't happen when req/res.Config is set to string.

Not sure if there's any unexpected issues in between 🤔

camilamacedo86 · 2024-02-13T08:29:02Z

pkg/plugins/external/init.go

@@ -40,16 +43,28 @@ func (p *initSubcommand) BindFlags(fs *pflag.FlagSet) {
 }

 func (p *initSubcommand) Scaffold(fs machinery.Filesystem) error {
+	cfg := p.config.(*v3cfg.Cfg)


@Eileen-Yu should you not get the Config from the Project file here?
It will want the config empty when we want the config that exists already, right?
I am betting that is the reason you see config 0 instead of the data populated.

We need to do something like:

if err := config.LoadFrom(fmt.Sprintf("%s/%s", opts.InputDir, yaml.DefaultPath)); err != nil { log.Fatalf("Failed to load PROJECT file %v", err)

As we do to re-scaffold:

kubebuilder/pkg/rescaffold/migrate.go

Lines 45 to 46 in 66c3572

if err := config.LoadFrom(fmt.Sprintf("%s/%s", opts.InputDir, yaml.DefaultPath)); err != nil {

log.Fatalf("Failed to load PROJECT file %v", err)

OR something like we do here:

kubebuilder/pkg/cli/cli.go

Lines 206 to 215 in 66c3572

// getInfoFromConfigFile obtains the project version and plugin keys from the project config file.

func (c *CLI) getInfoFromConfigFile() error {

// Read the project configuration file

cfg := yamlstore.New(c.fs)

if err := cfg.Load(); err != nil {

return err

}

return c.getInfoFromConfig(cfg.Config())

}

camilamacedo86 · 2024-02-13T08:39:44Z

Hi @Eileen-Yu

Thank you for coming back on this one. it would be very nice if we could achieve it.
Your help here is very appreciated.

Regards: #3526 (comment)

When you try to run the commands from the external plugin, you create the Config struct instead of loading it from the Project file. More info: #3526 (comment)

camilamacedo86 · 2024-02-14T09:47:44Z

pkg/plugins/external/init.go

+		if err != nil {
+			return err
+		}
+		defer file.Close()


@Eileen-Yu here we need to sort out the lint issue
We need to check the error

camilamacedo86

@Eileen-Yu

Can you please:
a) squash the commits for we have just 1 here
b) fix the lint issue
c) rebase with master and push so that we can check it with the final state

Please, let us know if you are facing any issue that you need help with

Eileen-Yu · 2024-02-15T04:29:55Z

Hi @camilamacedo86 , thx for the help!

a) squash the commits for we have just 1 here
b) fix the lint issue
c) rebase with master and push so that we can check it with the final state

I did all of the above lists.

And yes, I did see the unit tests still failed because of this version error:

 msg: "unable to create config for version \"0\": version 0 is not supported",
              err: <config.UnsupportedVersionError>{
                  Version: {Number: 0, Stage: 0},

I tried to fix this with this line: https://github.com/kubernetes-sigs/kubebuilder/pull/3526/files#diff-992e7a26b8776b12932c030601857ed56f364b1ecd7dc0fac9dcfd8c0cb903c6R66 but didn't get the luck 😔

camilamacedo86 · 2024-02-15T13:17:13Z

pkg/plugins/external/init.go

@@ -40,16 +46,51 @@ func (p *initSubcommand) BindFlags(fs *pflag.FlagSet) {
 }

 func (p *initSubcommand) Scaffold(fs machinery.Filesystem) error {
+	fileName := "PROJECT"
+
+	if _, err := os.Stat(fileName); os.IsNotExist(err) {


We should not create the file
We should fail in this case.
The CLI is who has the responsibility to create it.
the external plugin just read and update.

camilamacedo86 · 2024-02-15T13:19:02Z

Hi @Eileen-Yu

Regards: #3526 (comment)
thank you a lot
I do not have this weekend to check it out.
But up the following one, I will run it locally and see if I can fix the nit

k8s-ci-robot · 2024-04-03T03:13:08Z

PR needs rebase.

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.

k8s-ci-robot · 2024-05-16T13:05:24Z

@Eileen-Yu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubebuilder-e2e-k8s-1-28-6	`924daa7`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-6`
pull-kubebuilder-e2e-k8s-1-27-10	`924daa7`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-10`
pull-kubebuilder-e2e-k8s-1-29-0	`924daa7`	link	true	`/test pull-kubebuilder-e2e-k8s-1-29-0`
pull-kubebuilder-test	`65ae4a3`	link	true	`/test pull-kubebuilder-test`
pull-kubebuilder-e2e-k8s-1-27-3	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-3`
pull-kubebuilder-e2e-k8s-1-26-6	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-26-6`
pull-kubebuilder-e2e-k8s-1-28-0	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-0`
pull-kubebuilder-e2e-k8s-1-29-2	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-29-2`
pull-kubebuilder-e2e-k8s-1-27-11	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-11`
pull-kubebuilder-e2e-k8s-1-28-7	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-7`
pull-kubebuilder-e2e-k8s-1-30-0	`65ae4a3`	link	true	`/test pull-kubebuilder-e2e-k8s-1-30-0`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2023

k8s-ci-robot requested review from everettraven and Kavinjsir August 7, 2023 23:20

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 7, 2023

camilamacedo86 reviewed Aug 8, 2023

View reviewed changes

pkg/plugins/external/init.go Show resolved Hide resolved

camilamacedo86 reviewed Aug 8, 2023

View reviewed changes

pkg/plugins/external/init.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Aug 8, 2023

View reviewed changes

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from afbb077 to d8df5ac Compare August 10, 2023 20:23

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 Aug 10, 2023

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from d8df5ac to 18627f0 Compare August 10, 2023 20:39

Eileen-Yu changed the title ~~✨ pass config between KB and external plugin~~ ✨ [WIP] pass config between KB and external plugin Aug 11, 2023

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch 2 times, most recently from 8fac0bf to b5a1d16 Compare August 15, 2023 03:32

Eileen-Yu marked this pull request as ready for review August 15, 2023 03:35

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2023

k8s-ci-robot requested a review from camilamacedo86 August 15, 2023 03:35

Eileen-Yu changed the title ~~✨ [WIP] pass config between KB and external plugin~~ ✨ pass config between KB and external plugin Aug 15, 2023

camilamacedo86 reviewed Aug 15, 2023

View reviewed changes

docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/flags.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Aug 15, 2023

View reviewed changes

docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/cmd/metadata.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Aug 15, 2023

View reviewed changes

.../book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1/external/types.go Outdated Show resolved Hide resolved

everettraven reviewed Aug 15, 2023

View reviewed changes

Kavinjsir suggested changes Aug 21, 2023

View reviewed changes

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from e9fb09f to b8edf55 Compare August 22, 2023 17:08

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

camilamacedo86 mentioned this pull request Nov 14, 2023

Plugins API for external plugins should provide option to allow check what is the layout and plugins used in a chain #3396

Open

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from 9c2f795 to 924daa7 Compare November 15, 2023 23:04

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

camilamacedo86 closed this Feb 6, 2024

camilamacedo86 reopened this Feb 6, 2024

camilamacedo86 requested changes Feb 6, 2024

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 6, 2024

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from 924daa7 to 9ef48b9 Compare February 13, 2024 04:47

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

camilamacedo86 reviewed Feb 13, 2024

View reviewed changes

camilamacedo86 reviewed Feb 14, 2024

View reviewed changes

feat: pass config between KB and external plugin

65ae4a3

Eileen-Yu force-pushed the feat/ep-with-PROJECT branch from a0ba688 to 65ae4a3 Compare February 15, 2024 04:26

camilamacedo86 reviewed Feb 15, 2024

View reviewed changes

camilamacedo86 self-assigned this Feb 15, 2024

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

camilamacedo86 added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ pass config between KB and external plugin #3526

✨ pass config between KB and external plugin #3526

Eileen-Yu commented Aug 7, 2023 •

edited

k8s-ci-robot commented Aug 7, 2023

camilamacedo86 Aug 8, 2023

everettraven Aug 15, 2023

Eileen-Yu Aug 15, 2023

everettraven Aug 15, 2023

Kavinjsir Aug 21, 2023

camilamacedo86 Feb 6, 2024

Kavinjsir left a comment

k8s-ci-robot commented Nov 15, 2023

Eileen-Yu commented Nov 15, 2023

Eileen-Yu commented Nov 16, 2023

camilamacedo86 left a comment

k8s-ci-robot commented Feb 6, 2024

Eileen-Yu commented Feb 13, 2024 •

edited

camilamacedo86 Feb 13, 2024 •

edited

camilamacedo86 commented Feb 13, 2024 •

edited

camilamacedo86 Feb 14, 2024

camilamacedo86 left a comment

Eileen-Yu commented Feb 15, 2024

camilamacedo86 Feb 15, 2024 •

edited

camilamacedo86 commented Feb 15, 2024 •

edited

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented May 16, 2024

	if err := config.LoadFrom(fmt.Sprintf("%s/%s", opts.InputDir, yaml.DefaultPath)); err != nil {
	log.Fatalf("Failed to load PROJECT file %v", err)

	// getInfoFromConfigFile obtains the project version and plugin keys from the project config file.
	func (c *CLI) getInfoFromConfigFile() error {
	// Read the project configuration file
	cfg := yamlstore.New(c.fs)
	if err := cfg.Load(); err != nil {
	return err
	}

	return c.getInfoFromConfig(cfg.Config())
	}

✨ pass config between KB and external plugin #3526

Are you sure you want to change the base?

✨ pass config between KB and external plugin #3526

Conversation

Eileen-Yu commented Aug 7, 2023 • edited

Description:

Motivation:

k8s-ci-robot commented Aug 7, 2023

camilamacedo86 Aug 8, 2023

Choose a reason for hiding this comment

everettraven Aug 15, 2023

Choose a reason for hiding this comment

Eileen-Yu Aug 15, 2023

Choose a reason for hiding this comment

everettraven Aug 15, 2023

Choose a reason for hiding this comment

Kavinjsir Aug 21, 2023

Choose a reason for hiding this comment

camilamacedo86 Feb 6, 2024

Choose a reason for hiding this comment

Kavinjsir left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2023

Eileen-Yu commented Nov 15, 2023

Eileen-Yu commented Nov 16, 2023

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 6, 2024

Eileen-Yu commented Feb 13, 2024 • edited

camilamacedo86 Feb 13, 2024 • edited

Choose a reason for hiding this comment

camilamacedo86 commented Feb 13, 2024 • edited

camilamacedo86 Feb 14, 2024

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

Eileen-Yu commented Feb 15, 2024

camilamacedo86 Feb 15, 2024 • edited

Choose a reason for hiding this comment

camilamacedo86 commented Feb 15, 2024 • edited

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented May 16, 2024

Eileen-Yu commented Aug 7, 2023 •

edited

Eileen-Yu commented Feb 13, 2024 •

edited

camilamacedo86 Feb 13, 2024 •

edited

camilamacedo86 commented Feb 13, 2024 •

edited

camilamacedo86 Feb 15, 2024 •

edited

camilamacedo86 commented Feb 15, 2024 •

edited