-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 stop to generate crd webhooks patches and cainjetions for any CRD/API and projects without webhooks #3647
🐛 stop to generate crd webhooks patches and cainjetions for any CRD/API and projects without webhooks #3647
Conversation
8e788f7
to
96508c3
Compare
96508c3
to
2407612
Compare
2407612
to
7841f64
Compare
97bf0df
to
fb61d82
Compare
fb61d82
to
fdfab55
Compare
@@ -49,30 +49,19 @@ func (f *EnableWebhookPatch) SetTemplateDefaults() error { | |||
} | |||
|
|||
const enableWebhookPatchTemplate = `# The following patch enables a conversion webhook for the CRD | |||
{{- if ne .Resource.API.CRDVersion "v1" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRD v1beat1 is no longer supported
We are removing because the subCommend webhook has not the implementation to return it. Due the fact of that the code under the condition cannot be used since k8s 1.22 make no sense keep it and change the subCommand to get the value from the resources tracked in the PROJECT config.
|
||
namespace: | ||
- kind: CustomResourceDefinition | ||
version: {{ .Resource.API.CRDVersion }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRD v1beta1 is now unsupported. It's being removed as the subCommand webhook lacks the necessary implementation to retrieve it. Given that the related code has been unusable since k8s 1.22, there's no rationale to maintain it and modify the subCommand to extract values from resources in the PROJECT config.
fdfab55
to
6d5ae4e
Compare
6d5ae4e
to
c8e81ac
Compare
@@ -76,8 +75,6 @@ func (s *apiScaffolder) Scaffold() error { | |||
&samples.CRDSample{Force: s.force}, | |||
&rbac.CRDEditorRole{}, | |||
&rbac.CRDViewerRole{}, | |||
&patches.EnableWebhookPatch{}, | |||
&patches.EnableCAInjectionPatch{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should only be generated with the scaffolding of a webhook. Hence, it's been moved to the webhook subcommand.
See that I added your PR here and address a few nits on top of that to make work. |
&patches.EnableCAInjectionPatch{}, | ||
); err != nil { | ||
return fmt.Errorf("error scaffolding kustomize webhook manifests: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just putting the execution here to occur before we start uncomment the pacthes and etc. We have not the scaffold to uncomment before the execution.
However the only chnage here is to add the following generations that should be made ONLY when we scaffold a webhook for the API and not for each API:
&patches.EnableWebhookPatch{},
&patches.EnableCAInjectionPatch{},
namespace: system | ||
name: webhook-service | ||
path: /convert | ||
{{- else }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !hasWebHookUncommented || err != nil { | ||
log.Errorf("Unable to find the target(s) #configurations:\n#- kustomizeconfig.yaml to uncomment in the file "+ | ||
"%s.", crdKustomizationsFilePath) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will uncomment the
configurations:
- kustomizeconfig
When webhooks are scaffold
The kustomizeconfig has the patches under config/crd/patches which are relevant only when we scaffold webhooks for the CRD
func GenerateV4(kbc *utils.TestContext) { | ||
var err error | ||
initingTheProject(kbc) | ||
creatingAPI(kbc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the e2e tests, we've made the following modifications:
- Refactored the code for initializing and creating an API into a function, enabling reuse in both the existing and new tests.
- Moved the code segment for uncommenting when webhooks/certmanager is disabled into a constant.
- Introduced a new test that replicates all the standard checks (excluding webhook-specific ones) to validate that a project without enabled webhooks deploys and operates as intended.
… and projects without webhooks At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks. Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585. To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated. Signed-off-by: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Co-authored-by: lowang-bh <lhui_wang@163.com>
c8e81ac
to
a4ad443
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me. Thanks @camilamacedo86 and @lowang-bh for finding and fixing this.
/lgtm
/approve
I haven't tested this with other plugin versions, but a small question - if this is happening with go/v4, does this happen with v3 also? I know we have deprecated it, but just wanted to verify. I am believing apart from the project structure the scaffolding logic is the same for both the versions.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, varshaprasad96 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:
Approvers can indicate their approval by writing |
Hi @varshaprasad96, Thank you for your support and thorough review. I'd like to address your question:
We deprecated go/v3 some time ago. As a result, we don't currently run tests against this version. In my opinion, we should avoid making changes to the deprecated version unless we encounter a very critical issue that necessitates a fix specifically for that version. (Honest, I am not able to think of a scenario where would be mandatory for us to backport the fix) Those fix does not change go/v3, it utilizes kustomize/v1 (reference: here). All ongoing changes made are on kustomize/v2 (used by go/v4 only). |
Thanks for the info @camilamacedo86 |
Description
What Problem This PR Solves
Projects without webhooks currently malfunction because we are scaffolding configurations during API creation that are exclusively valid for webhooks.
Details
Currently, our process automatically scaffolds
config/crd/patches
and CA injections for every CRD, without taking into consideration if webhooks are enabled in the project. These configurations are, in fact, only applicable when the project has webhooks enabled.This one-size-fits-all approach causes failures in projects without webhooks, as observed by @lowang-bh in their attempt to rectify the situation with kubebuilder pull request #3585.
While the intention behind PR #3585 was sound, the implementation was not thorough. The proposed solution merely added an extra space and missed uncommenting the option when generating webhooks. This oversight meant that the necessary patches were not scaffolded for each API (CRD) created. These patches are only relevant for APIs (CRDs) that have webhooks:
In addition to the primary fix, this PR introduces a new end-to-end test to validate our changes and prevent such issues from reoccurring. This test generates a project without webhooks and verifies that everything functions as expected:
Given the comprehensive nature of this fix, the following issues and pull requests can now be closed: