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

Sps Beispiel Chart erstellt #40

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Sps Beispiel Chart erstellt #40

wants to merge 18 commits into from

Conversation

hupling
Copy link

@hupling hupling commented Sep 4, 2024

Description

Short description or comments

Reference

Issues it-at-m/.github#14

@hupling hupling requested a review from a team September 4, 2024 08:14
@hupling hupling changed the title Chart erstellt Sps Beispiel Chart erstellt Sep 4, 2024
@hupling hupling linked an issue Sep 4, 2024 that may be closed by this pull request
3 tasks
@hupling hupling marked this pull request as draft September 4, 2024 08:18
@hupling
Copy link
Author

hupling commented Sep 4, 2024

@ejcsid hat ja schon gesagt, man soll den Ordner umbenennen, z. B. sps-beispiel

@hupling
Copy link
Author

hupling commented Sep 4, 2024

Ich verstehe immer noch nicht, warum wir ein separates Frontend-Chart (frontend-code) brauchen. Das sieht doch bei 95% der Projekte gleich aus. Besser ist das doch zentral zur als Template zur Verfügung zu stellen. Über die values.yml kann man ja ein anderes Image angeben.

metadata:
name: {{ .Chart.Name }}
labels:
app: {{ .Chart.Name }}
Copy link
Author

Choose a reason for hiding this comment

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

also hier braucht man auf alle Fälle noch das Annotations Feld. Um z.b. einen automatischen Rollout für die dev Umgebung zu garantieren https://docs.openshift.com/container-platform/4.16/openshift_images/triggering-updates-on-imagestream-changes.html

app: {{ .Chart.Name }}
data:
{{- with .Values.applicationYML }}
application.yml: |
Copy link
Author

Choose a reason for hiding this comment

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

warum willst du hier eine ganze application.yml einfügen, ist es nicht besser, dass über Umgebungsvariablen zu lösen?

@eidottermihi eidottermihi removed the request for review from a team September 4, 2024 08:39
serviceAccountName: {{ .Values.serviceAccount.name }}
securityContext:
{{- toYaml .Values.podSecurityContext | nindent 8 }}
containers:
Copy link
Author

Choose a reason for hiding this comment

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

also es fehlt die Konfig für einen Init-Container, aber wenn wir Appdynamics abschalten braucht man das nicht mehr

Copy link
Contributor

Choose a reason for hiding this comment

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

ich würde die Möglichkeit einen initContainer zu konfigurieren aufnehmen. Kann ja auch ohne AppD mal vorkommen, dass das ein Projekt braucht.

{{- with .Values.tolerations }}
tolerations:
{{- toYaml . | nindent 8 }}
{{- end }}
Copy link
Author

Choose a reason for hiding this comment

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

fehlt nicht noch der Lifecyle preStop, um einen Graceful-Shutdown zu garantieren

securityContext: {}
# capabilities:
# drop:
# - ALL
Copy link
Author

Choose a reason for hiding this comment

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

ist das nicht sinnvoll das zu aktivieren. also das hat der Kics-Check https://github.com/Checkmarx/kics ergeben. Ich glaube, Openshift, würde teile Davon auch automatisch setzen

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling so werden die Standard securityContext-Einstellung der kubernetes-Plattform verwendet

Copy link
Author

@hupling hupling Sep 5, 2024

Choose a reason for hiding this comment

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

aber der Container-Image braucht keine speziellen Linux Capabilities. Daher würde ich es einfach default-mäßig setzen. Daher sollten wir alles aktivieren, was Openshift default-mäßig automatisch setzt. So wird es auf alle Fälle auch auf Nicht-Openshift-Plattformen gesetzt

Copy link
Contributor

Choose a reason for hiding this comment

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

Kann man so machen, dann aber kommentieren. Ich befürchte, dass das sonst einfach kopiert wird, ohne zu verstehen was es bedeutet.

# drop:
# - ALL
# readOnlyRootFilesystem: true
# runAsNonRoot: true
Copy link
Author

Choose a reason for hiding this comment

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

das kann man auch aktivieren

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling siehe oben

Copy link
Author

Choose a reason for hiding this comment

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

type: ClusterIP
port: 8080

ingress:
Copy link
Author

Choose a reason for hiding this comment

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

braucht man überhaupt eine Ingress im Backend?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling deshalb ja auch enabled: false (s.u.),

Copy link
Author

Choose a reason for hiding this comment

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

aber können wir die Ingress nicht, komplett im Backend rauswerfen. Evtl. brauchen EAIs eine Route

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, verstehe. Ja, das Backend sollte keine ingress/route haben.

spec:
containers:
- name: wget
image: busybox
Copy link
Author

Choose a reason for hiding this comment

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

die frage ist, ob man das intern so machen soll, weil man ja nicht direkt von Docker pullen soll

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling M.E. passt das so. Die Tests laufen ja nur in Github.

Copy link
Author

Choose a reason for hiding this comment

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

also bei Sonarqube bin ich mir ziemlich sicher, dass mit einem helm install auch der Testpod ausgerollt wird. Das zeigt, dass das Helm-Chart richtig installiert wurde


image:
registry: ghcr.io
repository: it-at-m/refarch/refarch-backend
Copy link
Author

Choose a reason for hiding this comment

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

hier liegt ein Beispiel Image: https://github.com/it-at-m/sps

ghcr.io/it-at-m/sps/sps-backend:latest

@@ -0,0 +1,32 @@
{{- if .Values.autoscaling.enabled }}
apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
Copy link
Author

Choose a reason for hiding this comment

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

ich kenne mich mit dem Autoscaler nicht aus, daher kann ich auch kein feedback geben.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling das ist einfach der von Helm generierte HPA und wird im sample nicht genutzt (autoscaling.enabled=false).

Copy link
Author

@hupling hupling Sep 5, 2024

Choose a reason for hiding this comment

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

trotzdem sollte man wissen wie der Autoscaler funktioniert, wenn Leute fragen oder komplett den Code entfernen

@@ -0,0 +1,60 @@
{{- if .Values.ingress.enabled -}}
Copy link
Author

Choose a reason for hiding this comment

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

sollen wir das mit der ingress mit der annotation https://docs.openshift.com/container-platform/4.16/networking/routes/route-configuration.html#nw-ingress-creating-a-route-via-an-ingress_route-configuration oder auch eine openshift route zur Verfügung stellen

Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling wäre schon besser, wenn wir in gitHub ingress verwenden. route ist ja openshift-spezifisch.

@hupling
Copy link
Author

hupling commented Sep 4, 2024

@banzuu also Redudanzen habe ich nur einmal angemerkt

@hupling
Copy link
Author

hupling commented Sep 4, 2024

vieleicht auch noch die lint errors beheben https://github.com/it-at-m/helm-charts/actions/runs/10697760929/job/29655640764

charts/refarch-template/Chart.yaml Outdated Show resolved Hide resolved
charts/refarch-template/Chart.yaml Outdated Show resolved Hide resolved
charts/refarch-template/Chart.yaml Outdated Show resolved Hide resolved
charts/refarch-template/README.md Outdated Show resolved Hide resolved
charts/refarch-template/charts/backend/Chart.yaml Outdated Show resolved Hide resolved
charts/refarch-template/charts/frontend/values.yaml Outdated Show resolved Hide resolved

# Additional env on the output Deployment definition.
env: []

Copy link
Contributor

Choose a reason for hiding this comment

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

müssen hier nicht die Umgebungsvariable für die Anwendung stehen, z.B. TZ?

Copy link
Author

Choose a reason for hiding this comment

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

stimmt hier fehlen auf alle Fälle für das Backend die Timezone und der Java Heap

charts/refarch-template/charts/frontend/Chart.yaml Outdated Show resolved Hide resolved
securityContext: {}
# capabilities:
# drop:
# - ALL
Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling so werden die Standard securityContext-Einstellung der kubernetes-Plattform verwendet

# drop:
# - ALL
# readOnlyRootFilesystem: true
# runAsNonRoot: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@hupling siehe oben

@@ -0,0 +1,55 @@
refarch-gateway:
Copy link
Contributor

Choose a reason for hiding this comment

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

Die Werte in dieser Datei sind LHM-spezifisch und müssen raus bzw. dies könnte als Beispiel drin bleiben. Es muss aber dokumentiert werden, was die Werte bedeuten und wie diese zu setzen sind.

Copy link
Author

Choose a reason for hiding this comment

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

Sehe ich nicht so. Ich will ja, eine funktionierende Anwendung haben. siehe hier https://git.muenchen.de/ccse/cicd/sps/-/issues/609#note_853077

Copy link
Contributor

Choose a reason for hiding this comment

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

Wir arbeiten Open Source. D.h. das Repo muss so aufgebaut sein, dass es auch von nicht-LHM-Projekten genutzt werden kann. Wenn die Werte LHM-spezifisch sind und nicht dokumentiert werden, ist das m.E. kein guter Open Source Code. Wie gesagt: es kann beispielhaft so drin stehen, aber es MUSS so gut dokumentiert werden, dass nicht-LHM Projekte verstehen, für was die Werte stehen, wo sie genutzt werden und wie sie zu konfigurieren sind.

Copy link
Author

@hupling hupling Oct 7, 2024

Choose a reason for hiding this comment

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

nicht lhm spezifisch:

  env:
    - name: SPRING_PROFILES_ACTIVE
      value: no-security

musste ich wegen it-at-m/refarch#193 und it-at-m/refarch#180 reinmachen

    - name: SSO_ISSUER_URL
      value: https://sso.muenchen.de/auth/realms/muenchen.de

nicht lhm spezifisch:

    - name: SPRING_CLOUD_GATEWAY_ROUTES_0_ID
      value: backend
    - name: SPRING_CLOUD_GATEWAY_ROUTES_0_URI
      value: "http://a-backend:8080/"
    - name: SPRING_CLOUD_GATEWAY_ROUTES_0_PREDICATES_0
      value: Path=/api/beispielprojekt-backend-service/**
    - name: SPRING_CLOUD_GATEWAY_ROUTES_0_FILTERS_0
      value: RewritePath=/api/beispielprojekt-backend-service/(?<urlsegments>.*), /$\{urlsegments}
    - name: SPRING_CLOUD_GATEWAY_ROUTES_0_FILTERS_1
      value: RemoveResponseHeader=WWW-Authenticate

    - name: SPRING_CLOUD_GATEWAY_ROUTES_1_ID
      value: frontend
    - name: SPRING_CLOUD_GATEWAY_ROUTES_1_URI
      value: "http://a-frontend:8080/"
    - name: SPRING_CLOUD_GATEWAY_ROUTES_1_PREDICATES_0
      value: Path=/**
    - name: SPRING_CLOUD_GATEWAY_ROUTES_1_FILTERS_0
      value: RewritePath=/(?<urlsegments>.*), /$\{urlsegments}
    - name: SPRING_CLOUD_GATEWAY_ROUTES_1_FILTERS_1
      value: RemoveResponseHeader=WWW-Authenticate

das ist openshift spezifisch. Keine Ahnung wie das in Kubernetes funktioniert. Es gibt leider kein TestCluster für Kubernetes.
theoretisch kann ich auch einfach beispiel.example.de beim Host eintragen. https://github.com/SonarSource/helm-chart-sonarqube/blob/master/charts/sonarqube/values.yaml#L31 haben die bei Sonarqube auch so gemacht.


  ingress:
    enabled: false
    annotations:
      route.openshift.io/termination: "edge"
    className: openshift-default
    hosts:
      - host: beispiel.example.de
        paths:
          - path: /
            pathType: "ImplementationSpecific"
            backend:
              serviceName: '{{ include "refarch-gateway.fullname" . }}'
              servicePort: 8080
```

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

Successfully merging this pull request may close these issues.

Github Helm Chart
3 participants