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

CRDs contain the fields mutated by the controller #1681

Closed
ngrigoriev opened this issue Nov 1, 2022 · 29 comments
Closed

CRDs contain the fields mutated by the controller #1681

ngrigoriev opened this issue Nov 1, 2022 · 29 comments
Assignees

Comments

@ngrigoriev
Copy link

I am deploying MetalLB with ArgoCD and I have noticed that some objects are never shown as synced.

A quick check revealed that at leat two CRDs (bgppeers.metallb.io and addresspools.metallb.io) contain the fields /spec/conversion/webhook/clientConfig/caBundle and this value appears to be updated by the controller.

While I can tell ArgoCD to ignore the specific differences, maybe it is a good idea to add the fields that are mutated by the controller to the managedFields? This way the resulting objects will be cleaner.

MetalLB version: 0.13.7
Kubernetes: latest K3S
Networking: K3S default (flannel)

Thanks!

@fedepaol
Copy link
Member

fedepaol commented Nov 2, 2022

Oh, I wasn't aware of that option, thanks for reporting this!
The CRDs are generated by controller-gen, so I need to see if there's a slick way to declare the caBundle as managed.

@TheAnachronism
Copy link

I'm uncertain if that was causing you any issues, but I had some pretty serious ones with my unorthodox setup.

I have my hardware rented at Hetzner and have two dedicated servers connected with a v-switch. I also have a cloud machine, which is connected by the network features of Hetzner with these two dedicated servers.
My K8S cluster is built upon VMs that run on these servers and MetalLB is providing IPs which could also be reached from my cloud machines.

Because ArgoCD was messing with these CRDs, the connection from the cloud machines to the load balancer services broke.
As one of the cloud machines is my "network entry point", I couldn't access most of my services.

So for anyone who is running into such an issue, make sure ArgoCD doesn't mess with the caBundle for these CRDs.

@ngrigoriev
Copy link
Author

ArgoCD is not messing with anything unless you ask for it :)

The problem is very simple. ArgoCD compares the K8S objects with their desired state, field by field. It ignores the fields that are known (defined) as "managed" fields and it can also ignore the fields if you configure it this way. Whenever it detects a difference, it will flag this object as "not in sync" - because it is different from your GIT repo. And if you configure ArgoCD to sync automatically....well, then you ask for the fight between ArgoCD trying to set it to the value you want vs a controller running in the cluster trying to update these field(s).

I try to use ArgoCD for everything in my cluster.

There is a similar challenge, for example, with SealedSecret object, but there it cannot be solved by defining the fields "managed" because sealed-secrets controller actually creates Secret objects by decrypting SealedSecret one. So, the actual secret values (e.g. data fields) have to be ignored. Just an example, not related to this issue.

Bottom line, it is nice to have a clear declaration that a particular field of an object is transient and should not be considered by GitOps tools (like ArgoCD and similar) for detecting the out-of-sync objects.

@TheAnachronism
Copy link

I'm not saying that it is ArgoCD's fault :D
Just wanted to note that it actually can cause issues if one doesn't do it correctly.

@ngrigoriev
Copy link
Author

The software is (almost ;) ) never at fault :)

Yes, that's exactly why I raised this [low-priority] issue. GitOps method needs help with determining if an object in K8S is out of sync or not.

@fedepaol
Copy link
Member

We'll get to it! Sorry but I've been busy with other stuff lately

@TheAnachronism
Copy link

OK, so I have no idea what changed, but for some reason, I have the same problem again.
I have an LB service for my SSH access to my Gitea instance. I cannot connect to this SSH service from my cloud machine, that is in the same network.
I've specified, that the caBundle should be ignored by ArgoCD, so that doesn't really seem the problem.

I don't know what exact constellation it is, but at some point I managed to get it working, then recreated the service again, and now it's broken.
Some things that I did, to fix it:

  • Recreated the different CRDs (maybe something changed again with the caBundle). I deleted the CRDs, recreated them and then just to make sure, restarted the controller, so it could do its stuff.
  • Changed the LB IP for the SSH service of Gitea, that didn't fix it either.

In general, I'm just wondering; What do I have to do, and in which order, so the CRDs get the correct certificates and my GitOps solution doesn't mess it up again?

@TheAnachronism
Copy link

Alright, so, some news here:

I've removed MetalLB completely from my cluster and just installed it normally with kustomize, so without the usage of ArgoCD.
I still have the same problem, that a new service isn't really reachable from my cloud machine, and I'm really at the end of my nerves on how to fix this.

The weirdest thing is, that I have a second instance of Gitea running and there it works. I can even delete the service, it gets recreated with ArgoCD and the connection still works.

Something, somewhere is just going wrong with new services and MetalLB...

@ngrigoriev
Copy link
Author

I have just reinstalled MetalLB (because I needed to recreate my little cluster)with ArgoCD and everything went up perfectly. This is what I used in my application.yaml:

  ignoreDifferences:
  - group: "apiextensions.k8s.io"
    kind: CustomResourceDefinition
    name: bgppeers.metallb.io
    jsonPointers:
    - /spec/conversion/webhook/clientConfig/caBundle
  - group: "apiextensions.k8s.io"
    kind: CustomResourceDefinition
    name: addresspools.metallb.io
    jsonPointers:
    - /spec/conversion/webhook/clientConfig/caBundle

@TheAnachronism
Copy link

Well this is what I had in the ArgoCD application:

  ignoreDifferences:
    - group: apiextensions.k8s.io
      jsonPointers:
        - /spec/conversion/webhook/clientConfig/caBundle
      kind: CustomResourceDefinition

Does the same thing, but still didn't realy work.
Anyways I kind of ArgoCD excluded, as I reinstalled MetalLB manually with kustomize and something is still going wrong.

@TheAnachronism
Copy link

Alright, so about my problem, just wanted to note that I fixed it, and it didn't have anything to do with MetalLB...
Still, thanks for the help^^

@CRASH-Tech
Copy link

same problem

@fedepaol
Copy link
Member

I'd rather modify the documentation to list this issue in the deployment section.

@jvstein
Copy link

jvstein commented Jan 25, 2023

I ran into the same issue when using tanka. Unfortunately, tanka doesn't have the same functionality to ignore changes in specific paths of the populated items like ArgoCD.

I had to disable the CRDs and deploy them separately before running tanka apply.

values: {
  crds: {
    enabled: false,
  },
},
helm upgrade --install --create-namespace --namespace metallb-system metallb-crds /path/to/metallb/charts/metallb/charts/crds

To me, this feels like more than a documentation issue.

@ArsenyBelorukov
Copy link

In the case of using ignoreDifferences Argocd just ignore these changes, but syncs old pem.
To not sync old pem use https://argo-cd.readthedocs.io/en/stable/user-guide/sync-options/#respect-ignore-difference-configs

@mgoodness
Copy link

To me, this feels like more than a documentation issue.

Agreed. The controller should use managedFields to claim fields that it mutates, so that other mechanisms (Tanka, Argo CD) can respect those changes.

@acelinkio
Copy link

Facing this issue as well. The workarounds are definitely functional but limiting.

I use ArgoCD ApplicationSets for deploying my helm charts and there is no way to dynamically set ignoreDifferences. Even with the recently added GoTemplating, but those only support string replacement. ignoreDifferences is a list. https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/applicationset/GoTemplate.md#limitations

@lindhe
Copy link
Contributor

lindhe commented Aug 16, 2023

@fedepaol any plans on fixing this any time soon? 🙃

@fedepaol
Copy link
Member

better late than never :P I filed open-policy-agent/cert-controller#118 against the library we use for cert rotation. If / when it gets merged, it will allow us to specify a field owner for the caBundle field.

@fedepaol fedepaol self-assigned this Aug 24, 2023
BigRedS added a commit to BigRedS/nuc that referenced this issue Sep 23, 2023
@sysadmiral
Copy link

Merged and released in 0.10.0: https://github.com/open-policy-agent/cert-controller/releases/tag/v0.10.0

@dusatvoj
Copy link

Any chance to fix that? I've installed new cluster with k8s 1.27, metallb 0.13.12 and argocd 2.9.5 and there's many times during reconciliation this ...
image

and this:
image

even there's no change @ metallb

@fedepaol
Copy link
Member

Cutting a release in these days with the fix for this

@benzman81
Copy link

same issue here using rancher fleet

@fedepaol
Copy link
Member

fedepaol commented Feb 1, 2024

@benzman81 mind testing with the latest release? It should have the managedFields set

@benzman81
Copy link

@fedepaol we ran into the issue today using the lates helm chart metallb:0.14.3. We currenlty fixed it by adding this to fleet.yaml:

...
diff:
  comparePatches:
    - apiVersion: apiextensions.k8s.io/v1
      kind: CustomResourceDefinition
      name: bgppeers.metallb.io
      operations:
        - op: remove
          path: /spec/conversion/webhook/clientConfig/caBundle
...

@Griznah
Copy link

Griznah commented Feb 5, 2024

I had the same issue today as well, with 0.14.3. I have selfHeal: true, so I just deleted the CRD to get back to sync, but this was ofc just temporary. Had to add ignoreDifferences.

@ChristianCiach
Copy link

ChristianCiach commented Mar 28, 2024

Workaround if you are using Carvel kapp:

apiVersion: kapp.k14s.io/v1alpha1
kind: Config

rebaseRules:
  - paths:
      - [data, ca.key]
      - [data, tls.crt]
      - [data, tls.key]
      - [data, ca.crt]
    type: copy
    sources: [existing, new]
    resourceMatchers:
     - kindNamespaceNameMatcher:
         kind: Secret
         namespace: metallb-system
         name: metallb-webhook-cert

  - paths:
      - [spec, conversion, webhook, clientConfig, caBundle]
      - [spec, conversion, webhook, clientConfig, service, port]
    type: copy
    sources: [existing, new]
    resourceMatchers:
     - kindNamespaceNameMatcher:
        kind: CustomResourceDefinition
        name: bgppeers.metallb.io

See https://carvel.dev/kapp/docs/v0.60.x/config/#rebaserules for documentation.

@lexfrei
Copy link

lexfrei commented Apr 4, 2024

For anyone who trying to google the problem, here is the summarised solution from this thread which worked for me:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: metallb
  namespace: argocd
spec:
  destination:
    namespace: metallb-system
    server: "https://kubernetes.default.svc"
  source:
    repoURL: "https://metallb.github.io/metallb"
    targetRevision: "0.14.4"
    chart: metallb
  project: default
  ignoreDifferences:
    - group: "apiextensions.k8s.io"
      kind: CustomResourceDefinition
      name: bgppeers.metallb.io
      jsonPointers:
        - /spec/conversion/webhook/clientConfig/caBundle
    - group: "apiextensions.k8s.io"
      kind: CustomResourceDefinition
      name: addresspools.metallb.io
      jsonPointers:
        - /spec/conversion/webhook/clientConfig/caBundle
  syncPolicy:
    automated:
      selfHeal: true
    syncOptions:
      - ServerSideApply=true
      - CreateNamespace=true
      - RespectIgnoreDifferences=true

@fedepaol
Copy link
Member

The managedFields property is set as we can see with

 k get validatingwebhookconfigurations.admissionregistration.k8s.io --show-managed-fields -o yaml | grep -b30 "manager: MetalLB"

and

 k get crd bgppeers.metallb.io --show-managed-fields -o yaml | grep -b10 "manager: MetalLB"

I'll close this issue and file one for the documentation, code wise there's nothing more we can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests