🐛 Fail when creating api webhook for a core type #2152

kopiczko · 2021-04-23T08:11:28Z

This change makes:

create api fail when --resource is set and the API is not already scaffolded for a core type
create webhook fail for a core type

k8s-ci-robot · 2021-04-23T08:11:36Z

Welcome @kopiczko!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. 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/kubebuilder 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 · 2021-04-23T08:11:36Z

Hi @kopiczko. 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.

kopiczko · 2021-04-23T08:14:50Z

/cc @estroz

pkg/plugins/golang/options.go

kopiczko · 2021-04-23T08:18:28Z

pkg/model/resource/resource.go

@@ -40,6 +40,9 @@ type Resource struct {
 	// Controller specifies if a controller has been scaffolded.
 	Controller bool `json:"controller,omitempty"`

+	// Core indicates if this is a core resource.
+	Core bool `json:"core,omitempty"`


I'm not sure why there are those json tags. Is it marshalled somewhere?

Yep, in the config. I think we can get the same functionality as setting a bool if we check strings.HasPrefix(Resource.Path, modulePath) where modulePath is from go.mod. This might break for submodules though.

If we do decide to go with an extra field, I vote for

Suggested change

Core bool `json:"core,omitempty"`

External bool `json:"external,omitempty"`

Since it covers both cases. If the path starts with k8s.io/ then we can be pretty sure it is core.

Thanks, thinking of that there is no point of introducing .Core because we can basically check strings.HasPrefix(res.Path, "k8s.io").

Checking against module is also not hard. There is golang.findGoModulePath function already defined. It could be exported.

@estroz I went with simple prefix check. That can be changed if for any reason .External has to be introduced, but I'd prefer to avoid changing PROJECT because something may be needed.

We cannot change core to external.

it is a breaking change

Core type means types that are defined in the K8s API and External means we scaffold an API that is defined in another project. For example, I have an Operator A which defines APIs that will also be used by my Operator B.

pkg/plugins/golang/v2/webhook.go

pkg/plugins/golang/v3/webhook.go

pkg/plugins/golang/v2/webhook.go

kopiczko · 2021-04-28T08:19:51Z

pkg/plugins/golang/v3/webhook.go

 	// check if resource exist to create webhook
-	if r, err := p.config.GetResource(p.resource.GVK); err != nil {
+	loadedResource, err := p.config.GetResource(p.resource.GVK)
+	if err != nil || !loadedResource.HasAPI() {
 		return fmt.Errorf("%s create webhook requires a previously created API ", p.commandName)
-	} else if r.Webhooks != nil && !r.Webhooks.IsEmpty() && !p.force {
+	}
+	if loadedResource.Webhooks != nil && !loadedResource.Webhooks.IsEmpty() && !p.force {
 		return fmt.Errorf("webhook resource already exists")
 	}


After rereading it again I found that the check was already there but it had a bug: if the err != nil that doesn't guarantee the API is created. We need to check if the loaded resource HasAPI() as well.

So it now all works like this:

Path is always set for core resources. create api fails if the Path starts with k8s.io

create webhook fails if there is no scaffolded API

@Adirio @estroz PTALA

camilamacedo86 · 2021-04-28T12:49:51Z

pkg/plugins/golang/v3/api.go

+		// Generating the API for a core resource is forbidden.
+		if strings.HasPrefix(p.resource.Path, "k8s.io") {
+			return errors.New("scaffolding API for core resources is not supported")
+		}


WDYT about if is core-type then we allow to do the default scaffold and just update the PROJECT file?
What would be the bad side impacts one that?

c/c @estroz @Adirio

Do I understand correctly you'd like to ignore the --resource flag completely for the core types and silently set it to false?

@camilamacedo86 I believe as a first iteration create api may fail when for a core resource. If the handling of the --resource flag changes in the future it will be a non-breaking change anyway if it fails now.

We can create api for a core type however that will be added to the project domain and is required to change manually. So, the correct approach IMO is:

create API: check if is a core-type and do not scaffold the resource and populate accordingly the PROJECT file

webhook API: check if we are trying to create a webhook for a core type, if yes then call create API for that to do the same above and allow the default scaffold occurs.

I think the PROJECT file is updated correctly for the core resource. See below:

$ kubebuilder create api --group "core" --version "v1" --kind "Pod" --resource=false --controller=true $ cat PROJECT domain: my-domain.com layout: - go.kubebuilder.io/v3 multigroup: true projectName: kubebuilder-test repo: github.com/kopiczko/kubebuilder-test resources: - controller: true group: core kind: Pod path: k8s.io/api/core/v1 version: v1 version: "3"

The problem you mention (if I understand it correctly) is present when you set --group "" instead of --group "core". This is kind of confusing. I opened a separate issue for this #2162.

See; https://github.com/kubernetes-sigs/kubebuilder/pull/2152/files#r720661651

k8s-triage-robot · 2021-07-29T13:55:47Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

k8s-triage-robot · 2021-08-28T14:44:34Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

estroz · 2021-08-30T15:45:50Z

/remove-lifecycle rotten
/ok-to-test

@camilamacedo86 follow-up thoughts on this?

camilamacedo86 · 2021-10-02T10:32:02Z

pkg/plugins/golang/v2/api.go

@@ -133,6 +134,11 @@ func (p *createAPISubcommand) InjectResource(res *resource.Resource) error {

 	// In case we want to scaffold a resource API we need to do some checks
 	if p.options.DoAPI {
+		// Generating the API for a core resource is forbidden.
+		if strings.HasPrefix(p.resource.Path, "k8s.io") {
+			return errors.New("scaffolding API for core resources is not supported")


Why that is forbidden? If it is a core type we just need to ignore the creation of the resources into the api/ dir and ensure that we will create the imports accordingly. This part also seems missing here. We would need to change the code for the fragments. Is that working now? Could we ensure that we have this scenario scaffold in the test data samples to check it?

camilamacedo86 · 2021-10-02T10:38:14Z

pkg/plugins/golang/v2/webhook.go

@@ -101,7 +101,7 @@ func (p *createWebhookSubcommand) InjectResource(res *resource.Resource) error {
 			return fmt.Errorf("%s create webhook requires a previously created API ", p.commandName)
 		}
 	} else {
-		if r, err := p.config.GetResource(p.resource.GVK); err != nil {
+		if r, err := p.config.GetResource(p.resource.GVK); err != nil || !r.HasAPI() {


IHMO we need here to improve this check.

If the error be faced here because the resource was not found we need to check if the resource is a core type and it is scaffold in the project file. If yes, then fine we should not return an error and allow scaffold the webhooks.

camilamacedo86

Thank you for your contribution 🥇
And sorry for the delay in the response.

This PR requires some small changes. Could you please check my comments?

k8s-ci-robot · 2021-10-02T10:39:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kopiczko
To complete the pull request process, please assign estroz after the PR has been reviewed.
You can assign the PR to them by writing /assign @estroz in a comment when ready.

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

k8s-triage-robot · 2021-12-31T13:55:48Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-01-30T14:28:30Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-03-01T15:23:34Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-03-01T15:23:46Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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 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. labels Apr 23, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 23, 2021

k8s-ci-robot requested review from camilamacedo86 and pwittrock April 23, 2021 08:11

k8s-ci-robot requested a review from estroz April 23, 2021 08:14

kopiczko commented Apr 23, 2021

View reviewed changes

kopiczko force-pushed the error-core-webhook branch from 9eefee0 to 121cc3b Compare April 25, 2021 11:07

Adirio suggested changes Apr 26, 2021

View reviewed changes

pkg/plugins/golang/v2/webhook.go Outdated Show resolved Hide resolved

pkg/plugins/golang/v3/webhook.go Outdated Show resolved Hide resolved

kopiczko force-pushed the error-core-webhook branch 3 times, most recently from 068ba61 to a38e802 Compare April 28, 2021 06:39

Adirio reviewed Apr 28, 2021

View reviewed changes

pkg/plugins/golang/v2/webhook.go Outdated Show resolved Hide resolved

kopiczko commented Apr 28, 2021

View reviewed changes

🐛 Fail when creating api webhook for a core type

9252e90

kopiczko force-pushed the error-core-webhook branch from a38e802 to 9252e90 Compare April 28, 2021 10:34

camilamacedo86 reviewed Apr 28, 2021

View reviewed changes

kopiczko requested review from Adirio and camilamacedo86 April 30, 2021 10:45

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 28, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Aug 30, 2021

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 30, 2021

camilamacedo86 reviewed Oct 2, 2021

View reviewed changes

camilamacedo86 requested changes Oct 2, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 31, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 30, 2022

k8s-ci-robot closed this Mar 1, 2022

kopiczko deleted the error-core-webhook branch April 29, 2022 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fail when creating api webhook for a core type #2152

🐛 Fail when creating api webhook for a core type #2152

kopiczko commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

kopiczko commented Apr 23, 2021

kopiczko Apr 23, 2021

estroz Apr 23, 2021

kopiczko Apr 24, 2021

kopiczko Apr 25, 2021

camilamacedo86 Oct 2, 2021

kopiczko Apr 28, 2021

camilamacedo86 Apr 28, 2021 •

edited

Loading

kopiczko Apr 28, 2021

kopiczko Apr 30, 2021

camilamacedo86 Apr 30, 2021

kopiczko Apr 30, 2021

camilamacedo86 Oct 2, 2021 •

edited

Loading

k8s-triage-robot commented Jul 29, 2021

k8s-triage-robot commented Aug 28, 2021

estroz commented Aug 30, 2021

camilamacedo86 Oct 2, 2021 •

edited

Loading

camilamacedo86 Oct 2, 2021

camilamacedo86 left a comment

k8s-ci-robot commented Oct 2, 2021

k8s-triage-robot commented Dec 31, 2021

k8s-triage-robot commented Jan 30, 2022

k8s-triage-robot commented Mar 1, 2022

k8s-ci-robot commented Mar 1, 2022

	Core bool `json:"core,omitempty"`
	External bool `json:"external,omitempty"`

🐛 Fail when creating api webhook for a core type #2152

🐛 Fail when creating api webhook for a core type #2152

Conversation

kopiczko commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

kopiczko commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

k8s-triage-robot commented Jul 29, 2021

k8s-triage-robot commented Aug 28, 2021

estroz commented Aug 30, 2021

camilamacedo86 Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 2, 2021

k8s-triage-robot commented Dec 31, 2021

k8s-triage-robot commented Jan 30, 2022

k8s-triage-robot commented Mar 1, 2022

k8s-ci-robot commented Mar 1, 2022

camilamacedo86 Apr 28, 2021 •

edited

Loading

camilamacedo86 Oct 2, 2021 •

edited

Loading

camilamacedo86 Oct 2, 2021 •

edited

Loading