helm charts for decco and other resources #28
Conversation
erwinvaneyk
left a comment
There was a problem hiding this comment.
I've left the original manifests in so we can refactor the deployment/test processes before cleaning them up and not having to do all of it synchronously.
I was about to ask if we will delete original manifests, but this works for me.
LGTM
| @@ -0,0 +1,16 @@ | |||
| apiVersion: apiextensions.k8s.io/v1beta1 | |||
There was a problem hiding this comment.
Maybe you could move the crds to the crds/ directory, which seems to be a best practice in Helm (see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/)
| kind: Namespace | ||
| metadata: | ||
| labels: | ||
| decco-project: system |
There was a problem hiding this comment.
Is there a reason/convention why we are using this label?
There was a problem hiding this comment.
In pkg/space/space.go:
func (c *SpaceRuntime) createNetPolicy() error {
if c.Space.Spec.Project == "" {
return nil
}
peers := []netv1.NetworkPolicyPeer{
{
NamespaceSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"decco-project": deccov1beta2.ReservedProjectName,
},
},
},
. . .
Based on that, I guess you can deploy multiple Spaces that will be included in the same NetworkPolicy. None of the DDUs on staging use the 'Project' field, which makes sense since they're all isolated deployments.
| @@ -0,0 +1,15 @@ | |||
| # This template is used to force the evaluation of vars that are required using the | |||
deploy/decco/values.yaml
Outdated
|
|
||
|
|
||
| # This last value is a secret as well, but is actually optional. | ||
| # slack_webhook_for_dns_update_failure: '' |
There was a problem hiding this comment.
It is optional, so you could remove the comment to highlight that the default value is ''.
| fieldRef: | ||
| apiVersion: v1 | ||
| fieldPath: metadata.namespace | ||
| image: quay.io/kubernetes-ingress-controller/nginx-ingress-controller:0.19.0 |
There was a problem hiding this comment.
Maybe extract this image as a parameter
| k8s-app: default-http-backend | ||
| spec: | ||
| containers: | ||
| - image: gcr.io/google_containers/defaultbackend:1.0 |
There was a problem hiding this comment.
Maybe extract this image as a parameter
| @@ -0,0 +1,11 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
I have a feeling this secret is not used anywhere and we might be able to delete it.
There was a problem hiding this comment.
I'm always happy to remove extraneous stuff, but I want to make sure I understand why.
AFAICT, the only App that gets deployed to a Space in the decco Namespace is kplane-usermgr, which has one HTTP endpoint. Still, removing this secret would prohibit TCP endpoints from being added in the future. Is that right?
There was a problem hiding this comment.
Unless things have changed, AFAIK kplane-usermgr is never deployed into the decco namespace. Normally, a space resource is created in the decco namespace, resulting in a new namespace (the "kplane namespace"), where kplane-usermgr is then deployed. Is my understanding correct?
There was a problem hiding this comment.
Never mind, I think I'm wrong. Space resources do get created in decco ns, but they need the tcp-cert secret. So ignore what I said. It's all well.
Helm charts for decco! This also includes the resources that were appended by kplane-tests, some of which are shared class resources that get their own simple chart since other components besides decco depend on them.
The only changes to the templates are for helm templating, no others were necessary.
I've left the original manifests in so we can refactor the deployment/test processes before cleaning them up and not having to do all of it synchronously.