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

✨ Update the chart to prepare for PCH #12

Merged
merged 4 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 40 additions & 31 deletions chart/templates/controller.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
apiVersion: v1
kind: ServiceAccount
metadata:
name: transport-controller-sa
namespace: {{.Values.wds_cp_name}}-system
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you removed the namespace from the service account, mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will add it to the resources

name: transport-controller
namespace: {{.Release.Namespace}}
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: {{.Values.wds_cp_name}}-transport-controller-role
name: {{.Values.wds_cp_name}}-transport-controller
rules:
- apiGroups:
- ""
Expand Down Expand Up @@ -52,52 +52,61 @@ rules:
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: {{.Values.wds_cp_name}}-transport-controller-rolebinding
name: {{.Values.wds_cp_name}}-transport-controller
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: {{.Values.wds_cp_name}}-transport-controller-role
name: {{.Values.wds_cp_name}}-transport-controller
subjects:
- kind: ServiceAccount
name: transport-controller-sa
namespace: '{{.Values.wds_cp_name}}-system'
name: transport-controller
namespace: {{.Release.Namespace}}
---
apiVersion: v1
kind: ConfigMap
metadata:
name: transport-controller-config
namespace: {{.Values.wds_cp_name}}-system
namespace: {{.Release.Namespace}}
data:
get-kubeconfig.sh: |
#!/bin/bash
# this script receives a ControlPlane name and a parameter
# that determines whether to extract the ControlPlane's in-cluster kubeconfig
# or the external kubeconfig (if set to "true", the first will be retrieved).
# The function returns the requested kubeconfig's content in base64.
# it assumes the kubeconfig context is set to access the hosting cluster.

cpname="$1"
get_incluster_key="$2"

key=""
if [[ "$get_incluster_key" == "true" ]]; then
key=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.inClusterKey}');
else
key=$(kubectl get controlplane $cpname -o=jsonpath='{.status.secretRef.key}');
#!/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Get the in-cluster kubeconfig for KubeFlex Control Planes
# get-kubeconfig.sh cp_name guess_its_name
cp_name="${1%"-system"}" # cp name or cp namespace
guess_its_name="$2" # true: try guessing the name of the ibms CP
if [ "$cp_name" == "" ] ; then
if [ "$guess_its_name" == "true" ] ; then
for cp in `kubectl get controlplane -o name`; do
cp=${cp##*/} # separate just the CP name
if [ "$(kubectl get controlplane $cp -o jsonpath="{.metadata.labels['kflex\.kubestellar\.io/cptype']}")" == "imbs" ] ; then
if [ "$cp_name" == "" ] ; then
cp_name=$cp
else
>&2 echo "ERROR: found more than one Control Plane of type imbs!"
exit 1
fi
fi
done
if [ "$cp_name" == "" ] ; then
>&2 echo "ERROR: no Control Plane of type imbs found!"
exit 2
fi
else
>&2 echo "ERROR: no Control Plane name specified!"
exit 3
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 $cp_name -o=jsonpath='{.status.secretRef.inClusterKey}')
secret_name=$(kubectl get controlplane $cp_name -o=jsonpath='{.status.secretRef.name}')
secret_namespace=$(kubectl get controlplane $cp_name -o=jsonpath='{.status.secretRef.namespace}')
# get the kubeconfig in base64
kubectl get secret $secret_name -n $secret_namespace -o=jsonpath="{.data.$key}"
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: transport-controller
namespace: {{.Values.wds_cp_name}}-system
namespace: {{.Release.Namespace}}
spec:
replicas: 1
selector:
Expand All @@ -108,12 +117,12 @@ spec:
labels:
name: transport-controller
spec:
serviceAccountName: transport-controller-sa
serviceAccountName: transport-controller
initContainers:
- name: setup-wds-kubeconfig
image: quay.io/kubestellar/kubectl:1.27.8
imagePullPolicy: Always
command: [ "bin/sh", "-c", "sh /mnt/config/get-kubeconfig.sh {{.Values.wds_cp_name}} true | base64 -d > /mnt/shared/wds-kubeconfig"]
command: [ "bin/sh", "-c", "sh /mnt/config/get-kubeconfig.sh {{.Values.wds_cp_name}} false | base64 -d > /mnt/shared/wds-kubeconfig"]
volumeMounts:
- name: config-volume
mountPath: /mnt/config
Expand Down
2 changes: 1 addition & 1 deletion chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

# Set the name of the transport control plane
transport_cp_name: its1
transport_cp_name: ""

# Set the name of the WDS control plane
wds_cp_name: wds1