Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager #4123

Merged
merged 1 commit into from
Oct 11, 2024
Merged

🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager #4123

merged 1 commit into from
Oct 11, 2024

Conversation

camilamacedo86
Copy link
Member

  • Removed config/default/webhookcainjection_patch.yaml to streamline the scaffold.
  • Clarified replacements blocks in kustomization.yaml for easier understanding. Each block is now labeled with instructions for uncommenting based on specific webhook scenarios (ValidatingWebhook, DefaultingWebhook, ConvertingWebhook).

Closes: #3538
Closes: #4119

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ Simplify scaffold by removing webhookcainjection_patch and clarifying replacements ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold. Removes webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold. Removes webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 3 times, most recently from 8d61421 to 146865f Compare August 31, 2024 21:41
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager 🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 1, 2024
Closed
varshaprasad96
varshaprasad96 reviewed Sep 3, 2024
View reviewed changes
testdata/project-v4-multigroup-with-deploy-image/config/default/kustomization.yaml Outdated
# - select:
# kind: ValidatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camilamacedo86 one question in this - replacements are executed sequentially. If we enable all defaulting, mutating and conversion - wouldn't the annotation be overwritten given the field path is the same?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our e2e tests, we have enabled all of them. You can see the changes here: Link to GitHub PR (we uncommented all tests, and they work as expected).

Additionally, the paths are different for each configuration. One will replace the ValidatingWebhookConfiguration, another will replace the MutatingWebhookConfiguration, and so on.

Moreover, in the e2e tests for the deploy image sample, we are specifically checking the scenario where only validating webhooks are scaffolded. You can review that here: Link to GitHub Workflow.

However, If you identify any scenarios where this could potentially cause issues, please let us know. We can then add e2e tests to ensure everything is functioning correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

We are working to improve the coverage /tests so that we can ensure this one better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel

Now, see that we are testing with e2e all samples under testdata and the tutorial ones
Also, for go/v4 we added a test to validate webhooks when they are applied in a ns that is not the same where the operator and its server is running

So, it is very well covered too.

@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from a7347f4 to f4df3c6 Compare September 3, 2024 07:15
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 9, 2024
Closed
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from 0ed05fd to bd3991f Compare September 10, 2024 18:57
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2024
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from 495d670 to d0fd88b Compare September 12, 2024 14:04
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 13, 2024
Merged
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 3 times, most recently from b252b3b to 2ae2852 Compare September 14, 2024 16:41
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 14, 2024
Merged
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2024
@camilamacedo86
Copy link
Member Author

c/c @lentzi90

@camilamacedo86
Copy link
Member Author

Hi @erikgb

You might be a good person to help us with the review of this one.
WDYT?

@camilamacedo86 camilamacedo86 mentioned this pull request Oct 2, 2024
Merged
varshaprasad96
varshaprasad96 approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Thanks @camilamacedo86 !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2024
Kavinjsir
Kavinjsir approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@Kavinjsir Kavinjsir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Nit:
Could we probably remove the code also at:

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, 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:
  • OWNERS [Kavinjsir,camilamacedo86,varshaprasad96]

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

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2024
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@camilamacedo86
fix issues by simplifying the scaffold. Removes webhookcainjection_pa…
21e03d5 
…tch and clarify replacements.

- Removed config/default/webhookcainjection_patch.yaml to streamline the scaffold.
- Clarified replacements blocks in kustomization.yaml for easier understanding. Each block is now labeled with instructions for uncommenting based on specific webhook scenarios (ValidatingWebhook, DefaultingWebhook, ConvertingWebhook).
@camilamacedo86 camilamacedo86 merged commit 7527b0d into kubernetes-sigs:master Oct 11, 2024
23 of 25 checks passed
@camilamacedo86 camilamacedo86 deleted the simplify-replaces branch October 11, 2024 12:37
@camilamacedo86 camilamacedo86 mentioned this pull request Oct 11, 2024
Closed
camilamacedo86 added a commit to camilamacedo86/kubebuilder that referenced this pull request Oct 31, 2024
@camilamacedo86
Refine CA injection setup for ConversionWebhook in Kubebuilder scaffold
4b4124d 
In PR kubernetes-sigs#4123, cert-manager CA injection annotations were added directly in the centralized config for ConversionWebhook:

  - Fields such as `fieldPath: .metadata.namespace` and `fieldPath: .metadata.name` were added to inject the CA into each CRD’s ConversionWebhook.

However, this setup is redundant for ConversionWebhooks. Each CRD with a ConversionWebhook already receives the necessary CA injection through a patch managed by cert-manager, making a centralized configuration unnecessary.

This commit removes the centralized CA injection configuration for ConversionWebhook to simplify the scaffold and prevent potential duplicate injection issues.

Fixes:
- Ensures CA injection is handled on a per-CRD basis by cert-manager patches, providing clear, efficient CA management for ConversionWebhooks.
@camilamacedo86 camilamacedo86 mentioned this pull request Oct 31, 2024
Closed
@s-urbaniak s-urbaniak mentioned this pull request Feb 21, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kavinjsir Kavinjsir Kavinjsir approved these changes

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

Assignees

@varshaprasad96 varshaprasad96

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
4 participants
@camilamacedo86 @k8s-ci-robot @Kavinjsir @varshaprasad96