fix : operatorhub-bundle-validation we cannot check webhooks

camilamacedo86 commented 3 years ago

Description

We cannot check webhooks via webhooks[*].admissionReviewVersions. It will be removed eventually. Seems that the version of AdmissionReview supported is based on the controller-runtime version in use. If someone is on a c-r new enough to support v1 and seems that has not a real reason for the v1beta1 format still. So, (presumably) it is part of a transitionary period. However, the admissionReviewVersions can still be accepting the value v1beta1. See: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#webhook-resources-v122

webhooks[*].admissionReviewVersions default value is removed and the field made required for v1 (supported versions for AdmissionReview are v1 and v1beta1)

Also, see that it was not removed from the validation yet: https://github.com/kubernetes/kubernetes/blob/ea0764452222146c47ec826977f49d7001b0ea8c/pkg/apis/admissionregistration/validation/validation.go#L156-L160

Then, we also remove from the EP the possibility to allow a bundle to have both after > 1.22 since it was discussed and was defined that to support the old versions users will need to provide a specific bundle for <= 1.15.

bparees commented 3 years ago

@camilamacedo86 so we can't check webhooks being v1beta1 at all under any circumstances?

if someone uses admissionReview with version v1beta1 what version of webhooks do they end up getting?

camilamacedo86 commented 3 years ago

Hi @bparees,

@camilamacedo86 so we can't check webhooks being v1beta1 at all under any circumstances?

Yes. We cannot check if the operators are or not configured with admissionregistration.k8s.io/v1beta1. See that the manifest has not been added to the bundle.

if someone uses admissionReview with version v1beta1 what version of webhooks do they end up getting?

If the person is using admissionregistration.k8s.io/v1beta1 it probably produces the data using the admissionReview/v1beta1 schema. The same for admissionregistration.k8s.io/v1 which will probably produce the data with the schema v1.

Then, when do the configuration:

  webhookdefinitions:
  - admissionReviewVersions:
    - v1beta1
    - v1

It means that the API will try to use admissionReview schema v1beta1 and, if it fails, will use v1. Anyway, it seems not matter too much since has not it be managed by OLM? And then, because of this, we do not add the manifests in the bundle? So, shows that what we need to ensure is that OLM will be using admissionregistration.k8s.io/v1 for 1.22. See here for example, it seems to be the case already. PS.: I raised this topic to try to further understand the k8s folks. You can check it here

bparees commented 3 years ago

If the person is using admissionregistration.k8s.io/v1beta1 it probably produces the data using the admissionReview/v1beta1 schema.

so we should at least catch and report those, right? Use of "admissionregistration.k8s.io/v1beta1" won't work in v1.22.

camilamacedo86 commented 3 years ago

so we should at least catch and report those, right? Use of "admissionregistration.k8s.io/v1beta1" won't work in v1.22.

Unfortunately not. Because:

webhookdefinitions[0].admissionReviewVersions means support admissionReview schema v1beta1 which still supported (https://kubernetes.io/docs/reference/using-api/deprecation-guide/#webhook-resources-v122) and we cannot use that to check admissionregistration api version
All bundles generated from kb/sdk layout project using webhook has both values in:

 webhookdefinitions:
  - admissionReviewVersions:
    - v1beta1
    - v1

See that it is scaffold by default e.g. And then, after discussion with k8s folks was decided to keep the default scaffold as it is now, adding both schema versions for admissionReviewVersions, since they still supported. (we need to support N-3 k8s versions with them anyway)

IMPORTANT However, it seems to not matter at all since OLM will be using admissionregistration.k8s.io/v1 to manage that. See here for example.

bparees commented 3 years ago

@camilamacedo86 i don't think that's what i'm asking. I'm asking if we see this: https://github.com/operator-framework/operator-sdk/blob/6b6c021400894281b0d35033e83b5671e4c5f1ac/testdata/go/v3/memcached-operator/config/webhook/manifests.yaml#L3

but with apiVersion: admissionregistration.k8s.io/v1beta1, can't we flag that as a problem?

camilamacedo86 commented 3 years ago

Hi @bparees,

@camilamacedo86 i don't think that's what i'm asking. I'm asking if we see this: https://github.com/operator-framework/operator-sdk/blob/6b6c021400894281b0d35033e83b5671e4c5f1ac/testdata/go/v3/memcached-operator/config/webhook/manifests.yaml#L3

As described above, this manifest is not added to the bundle and we have not this info.

but with apiVersion: admissionregistration.k8s.io/v1beta1, can't we flag that as a problem?

We cannot flag that because we have not this info, unfortunately.

bparees commented 3 years ago

As described above, this manifest is not added to the bundle and we have not this info.

ah, sorry that is the piece i was missing. Thanks.

bparees commented 3 years ago

/lgtm

bparees commented 3 years ago

@ecordell how is merge permission to this repo managed? (and i take it merges are handled manually?)

camilamacedo86 commented 3 years ago

Hi @bparees.

@ecordell how is merge permission to this repo managed? (and i take it merges are handled manually?)

I have the required permission to merge. Since we are here only removing the parts that cannot be done and it has +2 Ok it shows fine get merged.

operator-framework / enhancements

fix : operatorhub-bundle-validation we cannot check webhooks #76