feat: Add conversion webhook for ArgoCD v1alpha1 to v1beta1 migration

[svghadi]

What type of PR is this?
/kind enhancement

What does this PR do / why we need it:
This PR deprecates ArgoCD v1alpha CRD and adds a new ArgoCD v1beta1 CRD along with conversion webhook to automate migration of ArgoCD v1alpha1 resources to v1beta1.

Changes:

• Add new ArgoCD v1beta1 crd.
• Mark ArgoCD v1alpha1 as deprecated & add back the removed dex & sso deprecated fields in ArgoCD v1alpha1.
• Add explicit conversion funcs for converting ArgoCD v1alpha1 to v1beta1.
• Configure conversion webhook server to run on 9443 port in operator-controller-manager deployment.
• Disable single namespace install for operator. OLM doesn't support single namespace installation mode for operators with conversion webhooks.
• Use server side validation for "kubectl apply" as client side apply results into failure due to exceeding annotation size limit.
• Add kuttl e2e tests
• Add documentation for manual install of operator with conversion webhook support.

Follow-up PRs (yet to be created):

• Add CI job for testing conversion webhook
• Replace all ArgoCD v1alpha1 references with v1beta1 in codebase
• Update api docs

Have you updated the necessary documentation?

• Documentation update is required by this PR.
• Documentation has been updated. (Partially)

How to test changes / Special notes to the reviewer:
Operator Installation:

• Follow tests/olm/README.md from this PR to install the operator via OLM.

Manual Test:

• Create a ArgoCD with v1alpha1 version

  kubectl apply -f - <<EOF
  apiVersion: argoproj.io/v1alpha1
  kind: ArgoCD
  metadata:
    name: argocd
  spec:
    dex:
      config: test-config
  EOF

• Observe deprecation warning for kubectl apply
Warning: ArgoCD v1alpha1 is deprecated, please use v1beta1 instead.

• Verify that conversion to v1beta1 didn't result in data loss

  $ kubectl get argocd/argocd -o yaml
  apiVersion: argoproj.io/v1beta1
  kind: ArgoCD
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"argoproj.io/v1alpha1","kind":"ArgoCD","metadata":{"annotations":{},"name":"argocd","namespace":"default"},"spec":{"dex":{"config":"test-config"}}}
    name: argocd
  spec:
    sso:
      dex:
        config: test-config
      provider: dex

Automated Test:

• kubectl kuttl test ./tests/olm --config ./tests/kuttl-tests.yaml

[jaideepr97]

awesome work @svghadi 🎉

Thinking a little future facing, we currently have convertFrom and convertTo functions to move between alpha1/beta1
Is that enough to support future use cases when we may have a beta2/beta3 as well, without many changes?

[jaideepr97]

Also, I think we will need to go into the operator and update all the v1alpha1 references to v1beta1 in the reconciler logic, either in this PR or a follow up one

And probably think about what we need to do to support the webhook installation via non-OLM installation as well

[iam-veeramalla]

Can you confirm the same sed command works on both mac and linux ? sometimes there is a little syntaxing issues.

[svghadi]

Yes. It works on both mac & linux.

[iam-veeramalla]

I hope you verified the compatibility with this new version.

[svghadi]

I didn't find any compatibility matrix for opm and operator-sdk or olm. However I did use this new version to create registry image for operator testing, didn't see any issues while creation or running the registry image.

Also this change has nothing to do with the conversion webhook or crd upgrade. I made it for my local test environment as older version didn't support mac(arm). If needed, we can always revert this change.

[iam-veeramalla]

deprecated fields will have more priority ?

[svghadi]

Here, the idea of conversion webhook is for smooth migration of existing v1alpha1 users to v1beta1. If any existing users are using deprecated fields that means they have not updated their sso configurations to use new keys hence having more priority to deprecated fields in such cases makes more sense to me.

[jaideepr97]

I think its fine either way, the current checks prevent a confict like that from happening in the first place
i.e, they can't have the same fields with conflicting info in .spec.dex and .spec.sso.dex in their v1alpha1 CRs anyway

[iam-veeramalla]

I am yet to complete the review on this PR, reviewed some of it. Will continue the review tomorrow.

[svghadi]

Thinking a little future facing, we currently have convertFrom and convertTo functions to move between alpha1/beta1
Is that enough to support future use cases when we may have a beta2/beta3 as well, without many changes?

Yes. Whenever we add a new version, we have to decide on which version to store in etcd(hub version) and define convertFrom & convertTo for other version(spokes) along with few configuration changes.

Also, I think we will need to go into the operator and update all the v1alpha1 references to v1beta1 in the reconciler logic, either in this PR or a follow up one

Yes. Since there will be lot of files changed, I didn't make it in this PR as it will be difficult to review. I will create follow up PR for this.

And probably think about what we need to do to support the webhook installation via non-OLM installation as well

For non-OLM install, the webhook will work as expected provided user handles certificate generation and injection which can be automated on k8s using cert-manager and on openshift using Service CA Operator. I have updated the installation docs with steps for the same in this PR.

[jaideepr97]

Yes. Whenever we add a new version, we have to decide on which version to store in etcd(hub version) and define convertFrom & convertTo for other version(spokes) along with few configuration changes.

I don't think we need to keep changing the hub version. From my understanding, it doesn't really matter which version is the hub, it's just there to cut down the # of potential transformations we would have to worry about. We could probably just define v1beta1 as the hub indefinitely
Instead of defining a new pair of convertFrom/convertTo functions each time we have a new version, would it make sense to have a single convertFrom/convertTo function pair, which takes in a non-hub version as input and then uses an interface (probably a map) to figure out which conversion functions should be applied?
That way we only have to define conversion functions for individual fields going forwards

[iam-veeramalla]

I am yet to complete the review on this PR, reviewed some of it. Will continue the review tomorrow.

Please don't block the PR on my review. I see @jaideepr97 is actively working on this one.

I am not available for the whole next week. @svghadi , please feel free to merge the PR if you have approval from Jaideep.

[svghadi]

Instead of defining a new pair of convertFrom/convertTo functions each time we have a new version, would it make sense to have a single convertFrom/convertTo function pair, which takes in a non-hub version as input and then uses an interface (probably a map) to figure out which conversion functions should be applied?
That way we only have to define conversion functions for individual fields going forwards

@jaideepr97 - Can you please elaborate on this? Are you suggesting that we create a single, generic convertFrom/convertTo function?

My understanding about spokes is that each spoke needs to implement the Convertible interface from kubebuilder for conversion, which means each spoke would have its own convertFrom/convertTo function.

[jaideepr97]

My understanding about spokes is that each spoke needs to implement the Convertible interface from kubebuilder for conversion, which means each spoke would have its own convertFrom/convertTo function.

Ah okay
I also just realized that each convertFrom/convertTo will also be in its own package
nevermind, this is fine

[jaideepr97]

@svghadi curious, why is this change required?

[svghadi]

It's a local testing change. With GOARCH hardcoded to amd64, the build command creates binary for amd64 on my mac(arm64) local machine. If we don't specify anything, it will default to the host arch on which it builds.

[svghadi]

We should update docker build commands in Makefile to use --platform flag to make cross-buidling of images easy.

[jaideepr97]

@svghadi tested the PR and it works great 👍 Please remove the changes from the 0.7.0 bundle for now
once the master branch is set up for 0.8.0, these changes will automatically get there, so I don't want to block this PR just for that

[svghadi]

@jaideepr97 - I have removed the changes from 0.7.0 bundle. However code gen CI job is failing due to this.

[jaideepr97]

@svghadi we should be having a PR to set up 0.8.0 soon, once that is merged please rebase your PR so we can get this merged as well

[svghadi]

Rebased the PR with 0.8.0 changes. It is ready for merge. Thanks

[abdennour]

i like it