Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

templates: MutatingWebhookConfig filter to CREATE POD events only #1904

Merged

Conversation

draychev
Copy link
Contributor

@draychev draychev commented Oct 22, 2020

This PR:

  • adds rules to the MutatingWebhookConfiguration YAML template - observe only pods/create events
  • removes Rules from the MutatingWebhookConfiguration struct created by patchMutatingWebhookConfiguration in webhook.go
  • refactors and documents what and why the update/patch code does

Context

The template for the MutatingWebhookConfiguration is a bit misleading since it seems like it observes all events. In reality the final version of the MutatingWebhookConfiguration is augmented by the OSM Controller with a) rules and b) CA Bundle and looks like this:

$ kubectl get MutatingWebhookConfiguration osm-webhook-osm -o yaml

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
...
webhooks:
- admissionReviewVersions:
  - v1beta1
  clientConfig:
    caBundle: ABCDEF==
    service:
      name: osm-controller
      namespace: osm-system
      path: /mutate
      port: 443
...
  rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    operations:
    - CREATE
    resources:
    - pods
    scope: '*'
...

The addition of rules and caBundle is done in webhook.go via the patchMutatingWebhookConfiguration() function: https://github.com/openservicemesh/osm/blob/release-v0.4/pkg/injector/webhook.go#L332-L336

Even though adding this section to the YAML file is technically a noop - it would lead to fewer confusions when folks are browsing the source code to understand how this mutating webhook works.

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [X]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

@draychev draychev requested a review from a team as a code owner October 22, 2020 22:31
@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #1904 into main will decrease coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1904      +/-   ##
==========================================
- Coverage   58.57%   58.40%   -0.17%     
==========================================
  Files         130      130              
  Lines        5265     5256       -9     
==========================================
- Hits         3084     3070      -14     
- Misses       2178     2183       +5     
  Partials        3        3              
Impacted Files Coverage Δ
pkg/injector/webhook.go 71.11% <100.00%> (-1.38%) ⬇️
...ertificate/providers/tresor/certificate_manager.go 69.66% <0.00%> (-6.75%) ⬇️
pkg/envoy/route/config.go 95.83% <0.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3ade1e...425146a. Read the comment docs.

@draychev draychev requested review from shashankram and a team October 23, 2020 16:40
shashankram
shashankram previously approved these changes Oct 23, 2020
pkg/injector/webhook.go Outdated Show resolved Hide resolved
shashankram
shashankram previously approved these changes Oct 23, 2020
michelleN
michelleN previously approved these changes Oct 23, 2020
snehachhabria
snehachhabria previously approved these changes Oct 23, 2020

// updateMutatingWebhookCABundle updates the existing MutatingWebhookConfiguration with the CA this OSM instance runs with.
// It is necessary to perform this patch because the original MutatingWebhookConfig YAML does not contain the root certificate.
func updateMutatingWebhookCABundle(cert certificate.Certificater, webhookName string, clientSet kubernetes.Interface) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I missed this in the code somewhere but do we watch for changes in this mwhc resource to reconcile/ensure the CA cert in this mwhc is using the most recent CA cert from the secret? I can open an issue if that helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ritazh very good point - this does not exist yet!
An issue would be great!

ksubrmnn
ksubrmnn previously approved these changes Oct 23, 2020
Copy link
Contributor

@eduser25 eduser25 left a comment

Choose a reason for hiding this comment

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

lgtm

@draychev draychev merged commit 0be40ed into openservicemesh:main Oct 23, 2020
@draychev draychev deleted the mutatingwebhookconfig-create-pods-only branch October 23, 2020 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants