Add more flexibility on how to configure certificates for the admission controllers via helm chart #561
Conversation
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
vasrem
force-pushed
the
feature/refactor_certificates_for_webhooks
branch
from
ccdcbcb
to
c80336c
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 7285020551
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
vasrem
force-pushed
the
feature/refactor_certificates_for_webhooks
branch
from
984b110
to
244cc84
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
vasrem
force-pushed
the
feature/refactor_certificates_for_webhooks
branch
from
244cc84
to
23b63a6
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
vasrem
force-pushed
the
feature/refactor_certificates_for_webhooks
branch
from
23b63a6
to
1a0a61b
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
| @@ -314,7 +314,8 @@ do | |||
| done | |||
|
|
|||
|
|
|||
| export ENABLE_ADMISSION_CONTROLLER=true | |||
| export ADMISSION_CONTROLLERS__ENABLED=true | |||
There was a problem hiding this comment.
I'm trying to understand the implications of changing the name of this env var or the name of the certificates. Wouldn't this break backward compatibility or what would happen at the moment of upgrading the operator?
There was a problem hiding this comment.
Could you elaborate on what level of backwards compatibility we need to maintain here? If I try not to break backwards compatibility I will end up with one of the following:
- Scattered values in Helm Chart related to admission controllers
- Inconsistency in admission controller related environment variables
- Multiple env variables handling the same thing
- And then the next question arises, who is going to remove those and when is the right time to remove them since we don't have any releases to say e.g. after 3 releases we remove those deprecated env variables?
With this PR, I try to bring some structure on the certificate story for the admission controllers and to do so, I needed to adjust a couple of things for consistency. I think I have done sufficient work to indicate these changes. There are at least 2 ways to deploy the stack based on what I found:
make deploy-setup-k8s- Helm Chart
For the former, the docs are updated. Even before if someone would set the ENABLE_ADMISSION_CONTROLLER env variable they would need to dive deeper to understand which secrets they should pre-populate if they didn't follow the docs. Actually, they would notice because the webhooks pods couldn't start by manually tinkering the cluster. Although I don't like this behavior, I kept it the same to not break your usual flow.
For the latter, to our knowledge we are the only consumers of the Helm Chart and we will handle that appropriately. If the chart was versioned, we would bump the major version to indicate a breaking change and then the admin would have to be careful.
There was a problem hiding this comment.
My main concern is changing the name of the var ENABLE_ADMISSION_CONTROLLER. Our downstream version (openshift) uses OLM to install this operator and we would have to make some changes there as far as I see it (we have additional files). So, I need to understand what this implies to us. @SchSeba @zeeke, what do you think?
I'm still trying to understand how we deploy the certificates in openshift. Need to dig on that further.
There was a problem hiding this comment.
OpenShift deployment doesn't rely on WEBHOOK_CA_BUNDLE var
https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/bindata/manifests/webhook/003-webhook.yaml#L8
Regarding the ENABLE_ADMISSION_CONTROLLER, we'll have to adjust the downstream deployment files, as you correctly stated:
I'm ok with the new name ADMISSION_CONTROLLERS_ENABLED
There was a problem hiding this comment.
Folks let me know if there is something for me to address here. If there is, please give me the branch or tag I should be doing the changes.
There was a problem hiding this comment.
thanks @zeeke for confirming
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
This commit changes the ENV variable that turns on the admission controllers to enable bundling of additional webhook related settings via the same prefix like certificate mode, CA etc. This is a cosmetic change. Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
This commit starts to make use of the new ADMISSION_CONTROLLERS__* environment variables when rendering manifests. It also adjusts the logic with which cert-manager related annotation is used. Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
This commit adjusts the manifests to use the new ADMISSION_CONTROLLERS__* environment variables and also adjusts the relevant documentation files to reflect the new changes. Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
Replace double underscores with underscores of admission controller related ENV variables to address feedback on the PR. Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
| spec: | ||
| selfSigned: {} | ||
| {{- end }} | ||
| {{- if and (not .Values.operator.admissionControllers.certificates.certManager.enabled) (.Values.operator.admissionControllers.certificates.custom.enabled) }} |
There was a problem hiding this comment.
it should be
if admissionControllers.enabled
{
if cert manager and certManager.generateSelfSigned {
} else if not cert manager and certificates.custom.enabled{
}
f23541d
to
75d4ad2
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
| cniBinPath: "/opt/cni/bin" | ||
| clusterType: "kubernetes" | ||
| admissionControllers: |
There was a problem hiding this comment.
@vasrem mind updating helm README with added parameters ?
im fine with updating in a separate PR as well.
forgot to mention this in the original PR
PR [1] changed operator's environment variable `ENABLE_ADMISSION_CONTROLLER` to `ADMISSION_CONTROLLERS_ENABLED`. Also, the following environment variable have been introduced as a replacement of the constants: - `operator-webhook-service` -> `ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_SECRET_NAME` - `network-resources-injector-secret` -> `ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_SECRET_NAME` refs: [1] k8snetworkplumbingwg/sriov-network-operator#561 Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
…#709) Today it's cumbersome to enable admission controllers for SRIOV Network Operator because the user needs to create a secret manually. With this PR k8snetworkplumbingwg/sriov-network-operator#561, it's possible to generate a self signed certificate so the user doesn't need to do manual steps to enable those admission controllers. This PR just updates the chart to the latest chart found in the `master` branch of https://github.com/k8snetworkplumbingwg/sriov-network-operator which at the time was k8snetworkplumbingwg/sriov-network-operator@233b99a. Another PR will enable proper support for admission controllers.
PR [1] changed operator's environment variable `ENABLE_ADMISSION_CONTROLLER` to `ADMISSION_CONTROLLERS_ENABLED`. Also, the following environment variable have been introduced as a replacement of the constants: - `operator-webhook-service` -> `ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_SECRET_NAME` - `network-resources-injector-secret` -> `ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_SECRET_NAME` refs: [1] k8snetworkplumbingwg/sriov-network-operator#561 Signed-off-by: Andrea Panattoni <apanatto@redhat.com> # Conflicts: # bundle/manifests/sriov-network-operator.clusterserviceversion.yaml # config/manifests/bases/sriov-network-operator.clusterserviceversion.yaml # manifests/stable/sriov-network-operator.clusterserviceversion.yaml
PR [1] changed operator's environment variable `ENABLE_ADMISSION_CONTROLLER` to `ADMISSION_CONTROLLERS_ENABLED`. Also, the following environment variable have been introduced as a replacement of the constants: - `operator-webhook-service` -> `ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_SECRET_NAME` - `network-resources-injector-secret` -> `ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_SECRET_NAME` refs: [1] k8snetworkplumbingwg/sriov-network-operator#561 Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
PR [1] changed operator's environment variable `ENABLE_ADMISSION_CONTROLLER` to `ADMISSION_CONTROLLERS_ENABLED`. Also, the following environment variable have been introduced as a replacement of the constants: - `operator-webhook-service` -> `ADMISSION_CONTROLLERS_CERTIFICATES_OPERATOR_SECRET_NAME` - `network-resources-injector-secret` -> `ADMISSION_CONTROLLERS_CERTIFICATES_INJECTOR_SECRET_NAME` refs: [1] k8snetworkplumbingwg/sriov-network-operator#561 Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
The goal of this PR is to add more flexibility on certificates that can be consumed by SRIOV Network Operator when the Admission Controllers are enabled. In particular, this PR focuses mostly on Helm Chart advancements to enable the following 4 scenarios:
Secretvalues.yamlCertificateresources that are already created externallyCertificateresourcesTesting
I don't seem to find an existing place to add tests. Let me know if there is a place I can add tests. In the meantime, I did some manual tests.
Case 1
Setup:
sriov-network-operator/doc/quickstart.md
Lines 34 to 45 in 7582f30
Expect:
Case 2
Setup:
sriov-network-operator/doc/quickstart.md
Lines 47 to 95 in 7582f30
Expect:
Case 3
Create the following secret and setup helm release:
Expect:
Case 4
Setup certificate like
sriov-network-operator/doc/quickstart.md
Lines 55 to 87 in 7582f30
Expect:
Case 5
Setup helm release:
Expect:
Case 6
Setup helm release:
Expect: