Kynerno has been updated to v1.17.1. Update the document to adapt to the changes#993
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request updates the Kyverno integration documentation from version 1.6.3 to 1.17.1. Key changes include revised installation instructions for namespaces, configmaps, and RBAC resources, a comprehensive list of commands for deploying Kyverno CRDs, and an updated deployment manifest for the admission controller. The demo section has also been updated to utilize the ValidatingPolicy resource instead of the older ClusterPolicy. Review feedback identifies a critical missing value for the replicas field in the deployment YAML, broken markdown link syntax across several lines, inconsistent colon usage in headings, and missing RBAC deployment steps for the host cluster context.
01b4af6 to
f98d612
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Kyverno integration documentation to version v1.17.1, reflecting changes in installation manifests and the demo policy. The review feedback identifies a missing value for the replicas field in the deployment example and suggests consolidating the CRD installation commands into a single manifest for improved readability. Minor typographical errors involving full-width colons were also noted.
f98d612 to
614b110
Compare
Signed-off-by: changzhen <changzhen5@huawei.com>
614b110 to
b29527c
Compare
| Deploy Service resources: | ||
|
|
||
| You need to extract lines L88317-L88450 from the [Kyverno v1.17.1 installation file](https://github.com/kyverno/kyverno/releases/download/v1.17.1/install.yaml). | ||
|
|
||
| Deploy Kyverno Deployments resources: | ||
|
|
||
| You need to extract lines L88451-L89043 from the [Kyverno v1.17.1 installation file](https://github.com/kyverno/kyverno/releases/download/v1.17.1/install.yaml). | ||
|
|
||
| Please note that we need to add the startup argument `--kubeconfig=/etc/kubeconfig` to all deployments so that Kyverno can access the Karmada control plane. Using `kyverno-admission-controller` as an example, the updated content is shown below: | ||
|
|
||
| <details> | ||
|
|
||
| <summary>unfold me to see the yaml</summary> | ||
|
|
There was a problem hiding this comment.
I'm not quite sure this approach is going to work. It seems like a refactoring from old installation steps without taking into account new components in latest Kyverno versions.
Only for the sake of an example, let's take a look at this argument:
- --backgroundServiceAccountName=system:serviceaccount:kyverno:kyverno-background-controller
- There's no background-controller installed
I'm not even sure if creating the Deployment alone will even work. ServiceAccount kyverno-admission-controller is still needed on host cluster.
I kindly suggest to wait until the following MR is included in a new version of Kyverno: kyverno/kyverno#14668
Then, all the YAML included in this MR can be replaced with the following values.yaml to install Kyverno from a Helm Chart:
crds:
install: false
migration:
enabled: false
admissionController:
container:
extraArgs:
kubeconfig: /etc/kubeconfig
extraVolumes:
- name: kubeconfig
secret:
defaultMode: 420
secretName: kubeconfig
extraVolumeMounts:
- mountPath: /etc/kubeconfig
name: kubeconfig
subPath: kubeconfig
webhooksCleanup:
enabled: false
backgroundController:
enabled: false
cleanupController:
enabled: false
reportsController:
enabled: falselike this:
helm repo add kyverno https://kyverno.github.io/kyverno/
helm repo update
helm template kyverno kyverno/kyverno --version <NEW_VERSION_HERE> -n kyverno --values values.yaml | yq '. | select(.kind == "Deployment")' > deployment.yamlThere was a problem hiding this comment.
Thanks a lot, @cmontemuino
I'm not quite sure this approach is going to work. It seems like a refactoring from old installation steps without taking into account new components in latest Kyverno versions.
I tried a basic use case, and it worked fine. Let me try the other use cases in the quick start.
I kindly suggest to wait until the following MR is included in a new version of Kyverno: kyverno/kyverno#14668
It seems to be integrated. Let me try this way.
There is an additional parameter to be concerned about, that is, serverIP. It needs to use the ClusterIP and port of the kyverno service. Does Helm support this setting?
There was a problem hiding this comment.
It's integrated in "main" branch, but not in a release tag afaik
There was a problem hiding this comment.
Thanks for the reminder. I'll try the Helm installation method after the new release is available.
RainbowMango
left a comment
There was a problem hiding this comment.
Could we make the installation topology a bit clearer? In particular, I think the document should explicitly describe how Kyverno, Karmada, and the host cluster are installed and related to each other, since this can be quite confusing for readers.
I don't think this doc needs to demonstrate all Kyverno features. If necessary, a few diagrams/images would help a lot.
| ## Kyverno Installations | ||
|
|
||
| In this case, we will use Kyverno v1.6.3. Related deployment files are from [here](https://github.com/kyverno/kyverno/blob/release-1.6/config/install.yaml). | ||
| In this case, we will use Kyverno v1.17.1. Related deployment files are from [here](https://github.com/kyverno/kyverno/releases/download/v1.17.1/install.yaml). |
There was a problem hiding this comment.
The link is for downloading the file. Same as below.
But from the reader's perspective, they might need to check it online instead of downloading it.
Is there a way to link the file in the repo?
There was a problem hiding this comment.
I understand your concern, but I did not find such a file in the kyverno repository. The previous file was replaced with https://github.com/kyverno/kyverno/blob/release-1.17/config/install-latest-testing.yaml, but this is for testing purposes.
There was a problem hiding this comment.
I think this is also the reason why @cmontemuino suggests using Helm, so that users do not need to be aware of these specific deployment files. However, since we need to handle two apiservers separately, it might not be possible to achieve a user-transparent process at this stage.
| Deploy Kyverno CRDs: https://github.com/kyverno/kyverno/blob/release-1.6/config/install.yaml#L12-L7291 | ||
| Deploy namespace: | ||
|
|
||
| You need to extract lines L1-L9 from the [Kyverno v1.17.1 installation file](https://github.com/kyverno/kyverno/releases/download/v1.17.1/install.yaml). |
That's very well said. Once the new Helm version is released in the kyverno repository, I will try again and provide some topology diagrams to illustrate the relationships between them. |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Kynerno has been updated to v1.17.1. Update the document to adapt to the changes in Kynerno.
Which issue(s) this PR fixes:
Fixes #421
Special notes for your reviewer: