fix: issues with mindthegap deplotment #660

faiq · 2024-05-15T00:02:34Z

What problem does this PR solve?:

This adds trust-manager to allow us to use self hosted helm charts in mindthegap. See: kubernetes-sigs/cluster-api-addon-provider-helm#104

We can think about this in a couple of different parts:

Installing trust-manager and CRDs. We need to install trust manager so that we can create a certificate bundle with our self signed cert to caaph-controller-manager. We pull the CRDs separately because we are unable to use them in our templates because they aren't available at that time. See the comment in the script for more details.
Modifying CAAPH to use the bundle. This is done through a job it has a corresponding Service Account/Role/Role Binding.
Various little fixes. ie changing repository url and versions to using v

I tested this by creating a docker cluster.

The helm chart proxies use this value

- apiVersion: addons.cluster.x-k8s.io/v1alpha1
  kind: HelmChartProxy
  metadata:
    creationTimestamp: "2024-05-14T23:41:09Z"
    finalizers:
    - helmchartproxy.addons.cluster.x-k8s.io
    generation: 1
    name: node-feature-discovery-docker-cluster-cilium-helm-addon
    namespace: default
    ownerReferences:
    - apiVersion: cluster.x-k8s.io/v1beta1
      kind: Cluster
      name: docker-cluster-cilium-helm-addon
      uid: e0e943e4-6d63-4d85-8d37-f63160ff93e3
    resourceVersion: "3282"
    uid: e04b6b62-5f39-4528-87f7-5928d2afd4dd
  spec:
    chartName: node-feature-discovery
    clusterSelector:
      matchLabels:
        cluster.x-k8s.io/cluster-name: docker-cluster-cilium-helm-addon
    namespace: node-feature-discovery
    options:
      enableClientCache: false
      install:
        createNamespace: true
      timeout: 10m0s
      upgrade:
        maxHistory: 10
    releaseName: node-feature-discovery
    repoURL: oci://mindthegap.mindthegap.svc/charts

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

charts/cluster-api-runtime-extensions-nutanix/templates/job.yaml

charts/cluster-api-runtime-extensions-nutanix/templates/mindthegap.yaml

jimmidyson · 2024-05-15T08:08:41Z

charts/cluster-api-runtime-extensions-nutanix/Chart.lock

@@ -0,0 +1,6 @@
+dependencies:
+- name: trust-manager


I'm not keen on trust-manager being a requirement for CAREN - shouldn't this be a user deployment configuration rather than a strict requirement? It could conflict with existing trust-manager deployment for example.

We can keep this functionality until we implement #497

We might be able to then set this behind a boolean gate. If that is the case, i'm not sure what the implication for konvoy would be. How should we template it?

On the other hand, CAREN is already a highly opinionated and has a ton of dependencies adding another isn't that big of a deal IMO

dkoshkin

Did you observe a cert error when testing? From CAAPH code it looks like its always setting the insecure flag.

dkoshkin · 2024-05-15T19:44:35Z

charts/cluster-api-runtime-extensions-nutanix/values.yaml

+trust-manager:
+  app:
+    trust:
+      namespace: caren-system


Is it not possible to get this from {{ .Release.Namespace }}? This won't work if helm install --namespace is set to anything other than caren-system right?

values.yaml doesn't use templates :( helm/helm#2492

I ran make dev with this PR but removed the above trust-manager config and it seems to work correctly and already deploys the trust-manager chart into the correct namespace:

kubectl get pods -n caren-system NAME READY STATUS RESTARTS AGE cluster-api-runtime-extensions-nutanix-7f5dc9ffdd-gxl8r 1/1 Running 0 65s mindthegap-c57f655d5-k9sp4 1/1 Running 0 88s trust-manager-865cff7789-c2vc6 1/1 Running 0 88s

The chart handles this correctly https://github.com/cert-manager/trust-manager/blob/2cb20f7d742d44a9eb3f2ebf62c415fffb34aa57/deploy/charts/trust-manager/templates/_helpers.tpl#L55

I think we're good to remove this and not hardcode the namespace?

the issue is with referencing the secret, where the pod was scheduled was never the issue

this comment shows why we need to set trust namespace cert-manager/trust-manager#120 (comment)

dkoshkin · 2024-05-15T19:58:56Z

hack/examples/overlays/clusterclasses/aws/kustomization.yaml.tmpl

@@ -3,6 +3,7 @@

 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
+namespace: caren-system


Adding the namespace here directly makes it more difficult to reuse. See error message:

$ kubectl apply -f https://raw.githubusercontent.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/40853b83071e4804c5253ddf5981b77a1deb7b29/charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/aws-cluster-class.yaml -n kube-system the namespace from the provided object "caren-system" does not match the namespace "kube-system". You must pass '--namespace=caren-system' to perform this operation.

this can be an issue down the line for sure 🤔

hack/addons/kustomize/nfd/kustomization.yaml.tmpl

dkoshkin · 2024-05-15T20:03:23Z

charts/cluster-api-runtime-extensions-nutanix/templates/helm-config.yaml

@@ -10,36 +10,37 @@ data:
  cilium: |
    ChartName: cilium
    ChartVersion: 1.15.0
-    RepositoryURL: oci://mindthegap.{{ .Release.Namespace }}.svc
+    RepositoryURL: oci://mindthegap.caren-system.svc/charts


This won't work if deployed in a different namespace?

that is correct. it needs to be deployed in that namespace, this is a limitation of trust-manager needing trust namespace set. this comment explains it

cert-manager/trust-manager#120 (comment)

if helm was able to use templating in values.yaml we could get around this

jimmidyson · 2024-05-16T14:37:06Z

I've pushed kubernetes-sigs/cluster-api-addon-provider-helm#238 to fix this issue in CAAPH without requiring trust-manager - I think this approach is very clean and easy to work with and as such feel we should rework this PR based on that approach.

**What problem does this PR solve?**: This adds trust-manager to allow us to use self hosted helm charts in mindthegap. See: kubernetes-sigs/cluster-api-addon-provider-helm#104 We can think about this in a couple of different parts: 1. Installing trust-manager and CRDs. We need to install trust manager so that we can create a certificate bundle with our self signed cert to caaph-controller-manager. We pull the CRDs separately because we are unable to use them in our templates because they aren't available at that time. See the comment in the script for more details. 2. Modifying CAAPH to use the bundle. This is done through a job it has a corresponding Service Account/Role/Role Binding. 3. Various little fixes. ie changing repository url and versions to using `v` I tested this by creating a docker cluster. The helm chart proxies use this value ```yaml - apiVersion: addons.cluster.x-k8s.io/v1alpha1 kind: HelmChartProxy metadata: creationTimestamp: "2024-05-14T23:41:09Z" finalizers: - helmchartproxy.addons.cluster.x-k8s.io generation: 1 name: node-feature-discovery-docker-cluster-cilium-helm-addon namespace: default ownerReferences: - apiVersion: cluster.x-k8s.io/v1beta1 kind: Cluster name: docker-cluster-cilium-helm-addon uid: e0e943e4-6d63-4d85-8d37-f63160ff93e3 resourceVersion: "3282" uid: e04b6b62-5f39-4528-87f7-5928d2afd4dd spec: chartName: node-feature-discovery clusterSelector: matchLabels: cluster.x-k8s.io/cluster-name: docker-cluster-cilium-helm-addon namespace: node-feature-discovery options: enableClientCache: false install: createNamespace: true timeout: 10m0s upgrade: maxHistory: 10 releaseName: node-feature-discovery repoURL: oci://mindthegap.mindthegap.svc/charts ``` **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**:  **Special notes for your reviewer**:

faiq changed the title ~~Faiq/adds trust manager~~ feat: adds trust manager May 15, 2024

github-actions bot added the feature label May 15, 2024

faiq requested a review from jimmidyson May 15, 2024 00:46

faiq force-pushed the faiq/adds-mindthegap-container branch from 12bf273 to d774961 Compare May 15, 2024 00:57

faiq force-pushed the faiq/adds-trust-manager branch 2 times, most recently from 8838217 to 69b428f Compare May 15, 2024 01:20

faiq commented May 15, 2024

View reviewed changes

charts/cluster-api-runtime-extensions-nutanix/templates/job.yaml Outdated Show resolved Hide resolved

faiq commented May 15, 2024

View reviewed changes

charts/cluster-api-runtime-extensions-nutanix/templates/mindthegap.yaml Outdated Show resolved Hide resolved

jimmidyson reviewed May 15, 2024

View reviewed changes

faiq force-pushed the faiq/adds-trust-manager branch from 927d390 to fd6c326 Compare May 15, 2024 18:38

faiq force-pushed the faiq/adds-mindthegap-container branch from d774961 to b3bd6cc Compare May 15, 2024 18:39

faiq added 7 commits May 15, 2024 12:40

feat: adds trust manager to chart

823cb9b

feat: uses self hosted helm charts with mindthegap

dc5f0ca

fix: version number for nutanix csi

92f5dbd

fix: minor linting fixups

79428c5

fix: pre-commit

051e0c7

fix: remove old copyright and remove helm hook

3396a26

fix: updates items with new namespace

4052587

faiq force-pushed the faiq/adds-trust-manager branch from fd6c326 to 4052587 Compare May 15, 2024 18:40

faiq added 2 commits May 15, 2024 12:46

fix: precommit errors

c222806

fix: adds namespace to ct lint

40853b8

dkoshkin reviewed May 15, 2024

View reviewed changes

github-actions bot added feature and removed feature labels May 15, 2024

faiq added do not merge labels May 16, 2024

faiq force-pushed the faiq/adds-trust-manager branch 3 times, most recently from 9e3f803 to f33435c Compare May 16, 2024 18:36

faiq force-pushed the faiq/adds-trust-manager branch from f33435c to 4efc09a Compare May 16, 2024 18:37

fix: remove trust manager

254dacf

faiq force-pushed the faiq/adds-trust-manager branch from 4efc09a to 254dacf Compare May 16, 2024 18:38

faiq changed the title ~~feat: adds trust manager~~ fix: issues with mindthegap deplotment May 16, 2024

github-actions bot added fix and removed feature labels May 16, 2024

faiq merged commit 0cd5b10 into faiq/adds-mindthegap-container May 16, 2024
15 of 18 checks passed

faiq deleted the faiq/adds-trust-manager branch May 16, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: issues with mindthegap deplotment #660

fix: issues with mindthegap deplotment #660

Uh oh!

faiq commented May 15, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jimmidyson May 15, 2024

Uh oh!

faiq May 15, 2024

Uh oh!

dkoshkin left a comment •

edited

Loading

Uh oh!

dkoshkin May 15, 2024

Uh oh!

faiq May 15, 2024

Uh oh!

dkoshkin May 16, 2024

Uh oh!

faiq May 16, 2024 •

edited

Loading

Uh oh!

faiq May 16, 2024

Uh oh!

dkoshkin May 15, 2024

Uh oh!

faiq May 15, 2024

Uh oh!

Uh oh!

dkoshkin May 15, 2024

Uh oh!

faiq May 15, 2024

Uh oh!

jimmidyson commented May 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

fix: issues with mindthegap deplotment #660

fix: issues with mindthegap deplotment #660

Uh oh!

Conversation

faiq commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkoshkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faiq May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmidyson commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

faiq commented May 15, 2024 •

edited

Loading

dkoshkin left a comment •

edited

Loading

faiq May 16, 2024 •

edited

Loading

jimmidyson commented May 16, 2024 •

edited

Loading