-
Notifications
You must be signed in to change notification settings - Fork 78
Add optional network policy object for deploy of step-certificates #172
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| {{- if .Values.networkpolicy.enabled -}} | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: {{ include "step-certificates.fullname" . }}-policy | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "step-certificates.labels" . | nindent 4 }} | ||
| {{- with .Values.networkpolicy.annotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: {{ include "step-certificates.name" . }} | ||
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| ingress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: {{ .Values.service.targetPort }} | ||
| - protocol: TCP | ||
| port: {{ .Values.service.port }} | ||
| {{- if .Values.networkpolicy.allow }} | ||
| from: | ||
| {{- range .Values.networkpolicy.allow }} | ||
| - ipBlock: | ||
| cidr: {{ . | quote }} | ||
| {{- end }} | ||
| {{- end }} | ||
| egress: | ||
| - ports: | ||
| - protocol: TCP | ||
| port: 443 | ||
| - protocol: TCP | ||
| port: 80 | ||
|
Comment on lines
+33
to
+38
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the egress for port 80 should be enabled by default, having 443 makes sense, but I would add the list in the values.yaml, including 443 there, and do a range to create the list of ports. It also makes sense to add
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, 80 also makes sense for ACME HTTP-01 challenges, but they both should be configurable from the values.yaml, as you might want to also add UDP 53 for ACME DNS-01 or some other custom thing for OIDC webhooks, ...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the DNS resolution affected by this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DNS should be fine, as the Kubes already allows DNS resolution to make its way out of the cluster. |
||
| {{- if .Values.networkpolicy.allow }} | ||
| to: | ||
| {{- range .Values.networkpolicy.allow }} | ||
| - ipBlock: | ||
| cidr: {{ . | quote }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with networkPolicy, but isn't the targetPolicy enough?
.Values.service.portis used by the ingress (ingress.yaml). I'm unsure if it should be here, as the selector matches the pod. If it is necessary, it should be only there if the ingress is enabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You won't need this if you haven't enabled the ingress and only expect to service acme requests from other containers in the same cluster, but then if that's the case, you're not going to need to enable the Policy at all as that applies to communication outside of the Kubernetes cluster.
So we need to allow the service port inbound if we want hosts outside kubernetes to be able to access acme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, you think we won't need
.Values.service.Port(which is on the ingres and service) because we go in via the.Values.service.targetPort(on the container), or maybe vice-versa? It might be that we can get rid of targetPort if either Service or Ingres are enabled, and get rid of Port if neither are. TBH I'm not 100% certain how kubes works on this, so I opened both to be safe (as its no extra risk if the request bypasses the ingres). If anyone has a definite on this Im happy for guidance?