✨ Update the chart to prepare for PCH #12

francostellari · 2024-04-05T18:31:05Z

Summary

The purpose of this PR is to prepare the OTP chart for use by KubeStellar PCH.
In the future, we will use the new chart to simplify the user experience and incorporate the step 9 of the common setup into step 8.

Key changes:

the embedded CP namespace has been removed from the chart. This requires now to specify --namespace <cp>-system in the helm install command (this is automatically taken care by KubeFlex during the PCH installation)
the script for obtaining the ITS kubeconfig has been updated to allow for empty transport_cp_name, which will default to the first and unique CP of type imbs
the script for obtaining the WDS kubeconfig has been updated to accept the namespace wds-system along the wds CP name (this to allow simplifications in the PCH)

Referring to step 9 of https://github.com/kubestellar/kubestellar/blob/main/docs/content/direct/examples.md, the new Helm installation command would be:

helm --kube-context kind-kubeflex upgrade --install ocm-transport-plugin oci://ghcr.io/kubestellar/ocm-transport-plugin/chart/ocm-transport-plugin --version ${OCM_TRANSPORT_PLUGIN} \
 --namespace wds1-system \
 --set transport_cp_name=imbs1 \
 --set wds_cp_name=wds1

However, in the future, we plan to modify the existing KS PCH to add a second container which will automatically install this OTP chart along with the KS chart along the lines proposed below: #12 (comment)

One the new PCH will be available step 9 of the common setup instructions will not be necessary anymore and will not be exposed to the everyday user.

Related issue(s)

Fixes #

francostellari · 2024-04-05T18:31:44Z

/cc @nirrozenbaum
/cc @MikeSpreitzer
/cc @ezrasilvera
/cc @pdettori

nirrozenbaum · 2024-04-08T09:31:42Z

chart/templates/controller.yaml

@@ -16,13 +16,12 @@
 apiVersion: v1
 kind: ServiceAccount
 metadata:
-  name: transport-controller-sa
-  namespace: {{.Values.wds_cp_name}}-system


you removed the namespace from the service account, mistake?

@nirrozenbaum the namespace of all of the resources will be determined by the --namespace set by helm.
This is the same behavior used for the kubestellar: https://github.com/kubestellar/kubestellar/blob/dda831ca719a85b8b1ef63627b81d34b8a344e1e/chart/templates/controller-manager.yaml#L3
The PCH will run the helm chart with the namespace used by the controlplane

@francostellari that is very strange to see in one place that no namespace is specified while in the ClusterRoleBinding the namespace is specified (Release.Namespace). why not using Release.NS in both?

@nirrozenbaum I believe the use of the ns for the sa in the binding subject is mandatory... so it cannot be removed.
On the other hand adding namespace: {{.Release.Namespace}} ma be ok but that is not what the ks chart does for the PCH... I would prefer to keep the same behavior to reduce differences down the road.
@pdettori perhaps you can comment if there is a preference. I'm ok to add it if necessary.

from my understanding the result would be the same whether we specify Release.Namespace in serviceaccount or not (and then the helm chart namespace is used, which is exactly Release.Namespace). In general, I always prefer being explicit in such cases to keep consistency and being very clear.
I suggest doing the same for KS repo.

ok, I will add it to the resources

nirrozenbaum · 2024-04-08T09:34:25Z

chart/templates/controller.yaml

+  get-its-config.sh: |
+    #!/bin/env bash
+    its_name="${1%"-system"}" # ITS name or ITS namespace
+    if [ "$its_name" = "" ] ; then
+        for cp in `kubectl get controlplane -o name`; do
+            cp=${cp##*/}
+            if kubectl get controlplane $cp -o=jsonpath='{.metadata.labels}' | grep "imbs" &> /dev/null ; then
+                if [ "$its_name" = "" ] ; then
+                    its_name=$cp
+                else
+                    >&2 echo "ERROR: found more than one Control Plane of type imbs!"
+                    exit 1
+                fi
+            fi
+        done
+        if [ "$its_name" = "" ] ; then
+            >&2 echo "ERROR: no Control Plane of type imbs found!"
+            exit 2
+        fi
    fi
-
-    # get secret details
-    secret_name=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.name}')
-    secret_namespace=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.namespace}')
-
+    key=$(kubectl get controlplane $its_name -o=jsonpath='{.status.secretRef.inClusterKey}')
+    secret_name=$(kubectl get controlplane $its_name -o=jsonpath='{.status.secretRef.name}')
+    secret_namespace=$(kubectl get controlplane $its_name -o=jsonpath='{.status.secretRef.namespace}')
+    # get the kubeconfig in base64
+    kubectl get secret $secret_name -n $secret_namespace -o=jsonpath="{.data.$key}"
+  get-wds-config.sh: |
+    #!/bin/env bash
+    wds_name="${1%"-system"}" # WDS name or WDS namespace
+    key=$(kubectl get controlplane $wds_name -o=jsonpath='{.status.secretRef.inClusterKey}')
+    secret_name=$(kubectl get controlplane $wds_name -o=jsonpath='{.status.secretRef.name}')
+    secret_namespace=$(kubectl get controlplane $wds_name -o=jsonpath='{.status.secretRef.namespace}')
    # get the kubeconfig in base64
    kubectl get secret $secret_name -n $secret_namespace -o=jsonpath="{.data.$key}"


why do we split this into two functions that are doing almost the same?

@nirrozenbaum I agree that there is some code in common and I can combine them into a single script if you prefer.
The ITS script implements additional functionality to match the behavior required in the PCH.
I thought to keep them separated for clarity.
Let me know if you prefer to combine them.

is it possible to implement the extra logic of getting the ITS cp name in a different place and then in this script keep using it as it was before?

I unified the 2 scripts... testing before pushing commit

francostellari · 2024-04-08T15:24:34Z

@nirrozenbaum I implemented your request, please take another look when you have time

francostellari · 2024-04-08T15:36:35Z

I removed the out-of-cluster option since it was never used. Here the true/false is for guessing the name.

…

On Mon, Apr 8, 2024 at 11:32 AM Nir Rozenbaum ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In chart/templates/controller.yaml <#12 (comment)> : > @@ -119,7 +120,7 @@ spec: - name: setup-wds-kubeconfig image: quay.io/kubestellar/kubectl:1.27.8 imagePullPolicy: Always - command: [ "bin/sh", "-c", "sh /mnt/config/get-wds-config.sh {{.Values.wds_cp_name}} | base64 -d > /mnt/shared/wds-kubeconfig"] + command: [ "bin/sh", "-c", "sh /mnt/config/get-config.sh {{.Values.wds_cp_name}} false | base64 -d > /mnt/shared/wds-kubeconfig"] why false? wds is running in the hosting cluster, isn't it? — Reply to this email directly, view it on GitHub <#12 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL5TXIWIRVS5734LJ6C2JZTY4KZ7BAVCNFSM6AAAAABFZTHM2CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBWG44TOMRVGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Franco

MikeSpreitzer · 2024-04-09T18:59:56Z

chart/templates/controller.yaml

-    else
-      key=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.key}');
+  get-config.sh: |
+    #!/bin/env bash


Comments describing how this script is used, like in the lines being deleted, would be good

MikeSpreitzer · 2024-04-09T19:02:14Z

chart/templates/controller.yaml

+    if [ "$cp_name" == "" ] ; then
+      if [ "$guess_its_name" == "true" ] ; then
+        for cp in `kubectl get controlplane -o name`; do
+            cp=${cp##*/}


Why is this needed?
If it makes a difference, why would the kubectl get controlplane $cp below work?

@MikeSpreitzer can you please be more clear on what "this" you are rereferring to?

I am referring to the line that says cp=${cp##*/}. This is stripping off a prefix that never actually appears, as far as I can tell.

@MikeSpreitzer in my testing, it was returning the full name of the cp with the prefix.
I'm not sure why it would be different.
I don't think this is hurting so I would suggest to keep it to be conservative and cover my exception.

Sorry, I was wrong about what -o name does. I find that it prefixes the name with the resource.apiGroup and a slash, which i think is what you are saying you saw too.

mspreitz@mjs13 kubestellar % kubectl --context kind-kubeflex get controlplane -o name controlplane.tenancy.kflex.kubestellar.org/imbs1 controlplane.tenancy.kflex.kubestellar.org/wds1 mspreitz@mjs13 kubestellar % kubectl --context kind-kubeflex version Client Version: v1.28.7 Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3 Server Version: v1.29.2

So it looks like this prefix stripping is correct and necessary.

MikeSpreitzer · 2024-04-09T20:06:56Z

chart/templates/controller.yaml

+      if [ "$guess_its_name" == "true" ] ; then
+        for cp in `kubectl get controlplane -o name`; do
+            cp=${cp##*/}
+            if kubectl get controlplane $cp -o=jsonpath='{.metadata.labels}' | grep "imbs" &> /dev/null ; then


Grepping through all the labels for "imbs" is a bit fragile, and it will be worse when we change that to "its". I suggest using the following instead.

mspreitz@mjs13 ocm-transport-plugin % kubectl --context kind-kubeflex get controlplanes imbs1 -o jsonpath="{.metadata.labels['kflex\.kubestellar\.io/cptype']}" imbs

No, I can not explain why the dots need to be escaped, but they do.

mspreitz@mjs13 ocm-transport-plugin % kubectl --context kind-kubeflex get controlplanes imbs1 -o jsonpath="{.metadata.labels['kflex.kubestellar.io/cptype']}" | wc 0 0 0

@MikeSpreitzer thank you, I tried several combinations but could not get it to work so I surrendered to grep.
I will change it in the new version to use your filter.

Yes, I also spent a lot of time being stymied by this undocumented mystery. One day I stumbled across someone else's code using the solution.

MikeSpreitzer · 2024-04-09T20:12:01Z

chart/templates/controller.yaml

-      key=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.inClusterKey}');
-    else
-      key=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.key}');
+  get-config.sh: |


This is specifically getting a kubeconfig; why the name change?

@MikeSpreitzer I will change it back.

MikeSpreitzer · 2024-04-11T06:44:16Z

@francostellari: I do not understand the remark in the opening comment that says "The future PCH will use it this way" and shows the same thing as in "The new direct Helm installation command is" except for not specifying the --namespace. Why will the PCH not have to specify the namespace in the helm --kube-context kind-kubeflex ... command (but that specification is needed when an ordinary user issues the otherwise-same command)? Doesn't the kubeconfig specify the default namespace?

francostellari · 2024-04-11T10:28:52Z

@MikeSpreitzer This chart will be automatically installed along with the KS chart using a modified version of KS PCH.

Roughly, we will have a second container in KS PCH looking something like this:

          - name: "{{.HookName}}-otp"
            image: quay.io/kubestellar/helm:3.14.0
            imagePullPolicy: IfNotPresent
            args:
              - upgrade
              - --install
              - -n
              - "{{.Namespace}}"
              - otp
              - oci://ghcr.io/kubestellar/ocm-transport-plugin/transport-controller
              - --version
              - "0.1.4"
              - --set
              - "wds_cp_name={{.ControlPlaneName}}"
            env:
            - name: XDG_CACHE_HOME
              value: /tmp/helm/.cache

Correspondingly in the docs, step 9 will not be needed and not exposed to users, but will be achieved by step 8 using the new PCH.

Noticeably, we need the OTP chart to install in the "{{.Namespace}}" specified by Kubeflex, and find the the ITS kubeconfig without being told the name... which is what this PR is trying to achieve.

However, I will not start working on these new PR until all my PR are merged, the new charts are generated, and I come back from vacation.

Signed-off-by: francostellari <[email protected]>

Signed-off-by: francostellari <[email protected]> Enable empty transport_cp_name Signed-off-by: francostellari <[email protected]>

Signed-off-by: francostellari <[email protected]>

Signed-off-by: francostellari <[email protected]> Mike's comments Signed-off-by: francostellari <[email protected]> Mike's comments Signed-off-by: francostellari <[email protected]> Mike's comments Signed-off-by: francostellari <[email protected]> Mike's comments Signed-off-by: francostellari <[email protected]>

MikeSpreitzer · 2024-04-11T16:04:36Z

So the command that the opening comment exhibits under "The future PCH will use it this way" is intended to be what that second container in the new KS PCH will do? I see a difference regarding the namespace: the second container does specify it, while the opening comment shows removing the specification of the namespace.

MikeSpreitzer · 2024-04-11T16:06:01Z

Also, the opening comment's "The future PCH will use it this way" sets TWO chart values, while the second container in the new KS PCH sets only one.

francostellari · 2024-04-11T18:58:10Z

Also, the opening comment's "The future PCH will use it this way" sets TWO chart values, while the second container in the new KS PCH sets only one.

@MikeSpreitzer note that for the PCH we don't need to specify the transport_cp_name, the default empty value will let the container look for the unique CP of type IMBS, consistently with the current behavior of the KS chart.
However, you can also change the draft PCH to include forcing transport_cp_name=""

MikeSpreitzer

Looks Good To Me.
Thanks!

MikeSpreitzer

LGTM

nirrozenbaum

/lgtm
/approve

nirrozenbaum reviewed Apr 8, 2024

View reviewed changes

MikeSpreitzer reviewed Apr 9, 2024

View reviewed changes

francostellari force-pushed the prep-for-pch branch 4 times, most recently from 2ed94b1 to c94ca90 Compare April 9, 2024 20:48

francostellari added 4 commits April 11, 2024 07:28

remove ns

7e93c2b

Signed-off-by: francostellari <[email protected]>

Enable empty transport_cp_name

ab4251b

Signed-off-by: francostellari <[email protected]> Enable empty transport_cp_name Signed-off-by: francostellari <[email protected]>

address comments

1efdf56

Signed-off-by: francostellari <[email protected]>

francostellari force-pushed the prep-for-pch branch from c94ca90 to 2320d8e Compare April 11, 2024 11:29

MikeSpreitzer reviewed Apr 11, 2024

View reviewed changes

MikeSpreitzer approved these changes Apr 11, 2024

View reviewed changes

nirrozenbaum approved these changes Apr 11, 2024

View reviewed changes

nirrozenbaum merged commit a2e2bd6 into kubestellar:main Apr 11, 2024
7 checks passed

francostellari mentioned this pull request May 6, 2024

✨ Update PCHs to include ocm status-addon and transport-plugin kubestellar/kubestellar#2086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Update the chart to prepare for PCH #12

✨ Update the chart to prepare for PCH #12

francostellari commented Apr 5, 2024 •

edited

Loading

francostellari commented Apr 5, 2024

nirrozenbaum Apr 8, 2024

francostellari Apr 8, 2024

nirrozenbaum Apr 8, 2024

francostellari Apr 8, 2024

nirrozenbaum Apr 8, 2024

francostellari Apr 8, 2024

nirrozenbaum Apr 8, 2024

francostellari Apr 8, 2024

nirrozenbaum Apr 8, 2024

francostellari Apr 8, 2024

francostellari commented Apr 8, 2024

francostellari commented Apr 8, 2024 via email

MikeSpreitzer Apr 9, 2024

MikeSpreitzer Apr 9, 2024

francostellari Apr 9, 2024

MikeSpreitzer Apr 10, 2024

francostellari Apr 11, 2024

MikeSpreitzer Apr 11, 2024

MikeSpreitzer Apr 9, 2024 •

edited

Loading

francostellari Apr 9, 2024

MikeSpreitzer Apr 10, 2024

MikeSpreitzer Apr 9, 2024

francostellari Apr 9, 2024

MikeSpreitzer commented Apr 11, 2024

francostellari commented Apr 11, 2024 •

edited

Loading

MikeSpreitzer commented Apr 11, 2024

MikeSpreitzer commented Apr 11, 2024 •

edited

Loading

francostellari commented Apr 11, 2024

MikeSpreitzer left a comment •

edited

Loading

MikeSpreitzer left a comment

nirrozenbaum left a comment

✨ Update the chart to prepare for PCH #12

✨ Update the chart to prepare for PCH #12

Conversation

francostellari commented Apr 5, 2024 • edited Loading

Summary

Related issue(s)

francostellari commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francostellari commented Apr 8, 2024

francostellari commented Apr 8, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeSpreitzer commented Apr 11, 2024

francostellari commented Apr 11, 2024 • edited Loading

MikeSpreitzer commented Apr 11, 2024

MikeSpreitzer commented Apr 11, 2024 • edited Loading

francostellari commented Apr 11, 2024

MikeSpreitzer left a comment • edited Loading

Choose a reason for hiding this comment

MikeSpreitzer left a comment

Choose a reason for hiding this comment

nirrozenbaum left a comment

Choose a reason for hiding this comment

francostellari commented Apr 5, 2024 •

edited

Loading

MikeSpreitzer Apr 9, 2024 •

edited

Loading

francostellari commented Apr 11, 2024 •

edited

Loading

MikeSpreitzer commented Apr 11, 2024 •

edited

Loading

MikeSpreitzer left a comment •

edited

Loading