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.

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

Enable webhook conversion patch in kustomization #143

Closed
wants to merge 4 commits into from

Conversation

RafalKorepta
Copy link
Contributor

@RafalKorepta RafalKorepta commented May 17, 2024

b4809a1

Enable webhook conversion patch in kustomization

Kustomization is not recommented in our official documentation, but those
kustomization patch are neccessery for Redpanda conversion

605ec3a

Fix ApiVersion overwrite during conversion

During conversion webhook handler controller-runtime sets desire meta api version of the
destination resource from the Kubernetes API server request. Round trip marshall and unmarshall
is overwriting metadata.

Reference

https://github.com/kubernetes-sigs/controller-runtime/blob/f4ca78ebc00a4717ecd1c2daea1874d69ddd0137/pkg/webhook/conversion/conversion.go#L101

Kustomization is not recommented in our official documentation, but those
kustomization patch are neccessery for Redpanda conversion.
@RafalKorepta RafalKorepta force-pushed the rk/fix-webhook-setup branch 2 times, most recently from 20ffaec to 605ec3a Compare May 17, 2024 18:28
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

LGTM!

During conversion webhook handler controller-runtime sets desire meta api version of the
destination resource from the Kubernetes API server request. Round trip marshall and unmarshall
is overwriting metadata.

Reference

https://github.com/kubernetes-sigs/controller-runtime/blob/f4ca78ebc00a4717ecd1c2daea1874d69ddd0137/pkg/webhook/conversion/conversion.go#L101
@chrisseto
Copy link
Contributor

I think we might need to make some changes to the rbac kustomization.yaml as well?

@chrisseto chrisseto disabled auto-merge May 17, 2024 19:32
@chrisseto
Copy link
Contributor

Oh wait, I think I missed something.

So we don't need to register a Mutating or Validating webhook? We just need there to exist a webhook service?

@chrisseto
Copy link
Contributor

Dang and we'll need to rush out an update to the docs as installing the CRDs through kustomize isn't going to work any more .

The helm chart will need to override the webhook settings to ensure everything is configured appropriately. What a headache.

@RafalKorepta
Copy link
Contributor Author

Yes, but webhook would register handlers for validating and mutating requests. Currently there is no logic around that in Tedpanda custom resource.

@RafalKorepta
Copy link
Contributor Author

RafalKorepta commented May 17, 2024

I don't understand why CRD installation will not work anymore. I can only imagine that there is dependency on certificate. Our operator helm chart could create the self sign cert and eventually cert manager injector would include ca bundle.

@chrisseto
Copy link
Contributor

The service name and namespace need to be configured in order for the CRD's to work.

For example, for the helm chart the conversion strategy gets set to:

  conversion:
    strategy: Webhook
    webhook:
      clientConfig:
        service:
          name: '{{ include "redpanda-operator.fullname" $ }}-webhook-service'
          namespace: '{{ $.Release.Namespace }}'
          path: /convert
      conversionReviewVersions:
      - v1

@RafalKorepta
Copy link
Contributor Author

You are right. To not break users we should move to operator helm chart for consistency. I wouldn't recommend kustomization (addidiontal layer) that would take inputs to correctly render CRD.

@chrisseto
Copy link
Contributor

Want to jump on a slack huddle so we can try to tie this up and let you get to sleep?

I think kustomize is actually fine but we'd have to support kustomize for installing the entire operator which isn't happening before the weekend.

@RafalKorepta RafalKorepta marked this pull request as draft May 20, 2024 16:07
@RafalKorepta RafalKorepta marked this pull request as draft May 20, 2024 16:07
@RafalKorepta
Copy link
Contributor Author

I moved PR to draft as #144 should fix our current broken deployment.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@RafalKorepta RafalKorepta deleted the rk/fix-webhook-setup branch August 6, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants