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

Implement conversion webhook #355

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ruromero
Copy link
Contributor

Implement conversion webhook between ActiveMQArtemises v2alpha5 and v1beta1

@gtully gtully changed the title Redelivery rollback Implement conversion webhook Oct 28, 2022
@gtully
Copy link
Contributor

gtully commented Oct 28, 2022

This needs a re base from main, the conversion got a little easier after dropping the two problematic float variables. It will allow us to cleanly move to a v1 without interruption in the future so it is a great feature. nice!

@gtully
Copy link
Contributor

gtully commented Oct 28, 2022

do we need to create a self signed cert as part of the build? Having something hard coded that can expire will be problematic in the future I guess.

@ruromero ruromero force-pushed the redelivery_rollback branch 2 times, most recently from 08723b5 to 0f3c949 Compare October 28, 2022 13:46
Signed-off-by: ruromero <rromerom@redhat.com>
@gaohoward
Copy link
Collaborator

@ruromero We may need add conversion for security and address crds, may be do that separately.
To avoid static caBundle values, we may try use cert manager to auto inject the certs.
Also did you test/figure out how it works within OLM framework, we may need some doc here.

@gaohoward
Copy link
Collaborator

another problem with "static certs" is that it tied to a specific namespace. That's because the cert's CN name must match the webhook service name (so that the service's hostname ..svc can match the CN so it can be trusted).

@gaohoward
Copy link
Collaborator

on other thing, If I set ENABLE_WEBHOOKS to false and I can't deploy a v1beta1 CR.

@ruromero
Copy link
Contributor Author

ruromero commented Nov 2, 2022

@gaohoward I did not test with OLM only on OpenShift with basic deployment, as you noticed, static certs won't be trusted if it is deployed in a different namespace. I just used the existing cert secret generated with the operator-sdk.

on other thing, If I set ENABLE_WEBHOOKS to false and I can't deploy a v1beta1 CR.

That is correct, you have to deploy it with the envVar set to true, I can make it the default as this PR requires webhooks to be deployed.

@ruromero
Copy link
Contributor Author

ruromero commented Nov 2, 2022

I have updated the kustomize patch during the make generate-deploy target to set the ENABLE_WEBHOOKS to true

Signed-off-by: ruromero <rromerom@redhat.com>
Signed-off-by: ruromero <rromerom@redhat.com>
@gaohoward
Copy link
Collaborator

gaohoward commented Nov 3, 2022

@ruromero Hi, I got some error using the cert manager option. Below is what I did

$ export USE_CERTMANAGER=true
$ make deploy
$ kubectl create -f v2alpha5.yaml 
Error from server (InternalError): error when creating "v2alpha5.yaml": Internal error occurred: conversion webhook for broker.amq.io/v2alpha5, Kind=ActiveMQArtemis returned invalid object: must have the same name: ex-aao != 

$ cat v2alpha5.yaml
apiVersion: broker.amq.io/v2alpha5
kind: ActiveMQArtemis
metadata:
  name: ex-aao
spec:
  deploymentPlan:
    size: 1
    image: placeholder

Any idea?

@ruromero
Copy link
Contributor Author

ruromero commented Nov 3, 2022

Make sure you're creating the resource in the same namespace where the operator is deployed.
EDIT: I have tried and I can't reproduce this error from a fresh install.

Steps.

  1. Create minikube
  2. Install cert-manager kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.10.0/cert-manager.yaml
  3. Deploy activemq-artemis-operator make deploy OPERATOR_IMAGE_REPO=quay.io/ruben/activemq-artemis-operator USE_CERTMANAGER=false
  4. Create the v2alpha5 resource

@@ -8,9 +8,10 @@ VERSION ?= 1.0.0
KUBE_CLI=kubectl
OPERATOR_VERSION := 1.0.6
OPERATOR_ACCOUNT_NAME := activemq-artemis-operator
OPERATOR_CLUSTER_ROLE_NAME := operator-role
OPERATOR_CLUSTER_ROLE_NAME := operator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtully why was this working before?

@ruromero
Copy link
Contributor Author

ruromero commented Nov 4, 2022

@gaohoward @gtully I have tested the deployment on minikube and OCP with cert-manager and also on OCP with OLM (see the new target for deploying the catalog source) but another pair of eyes would help in case I missed something.

Thanks

@gaohoward
Copy link
Collaborator

as discussed with @ruromero I'll be closing this PR and send a new one for review.

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