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

chore(traefik): remove unused entrypoint #1139

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

mnencia
Copy link
Contributor

@mnencia mnencia commented Dec 29, 2023

I noticed that the Traefik configuration includes the option --entrypoints.tcp=true,
which generates an entry point named tcp without configuring it.
While this option doesn’t cause any harm, it is entirely unnecessary.
Therefore, I suggest removing it.

mysticaltech
mysticaltech previously approved these changes Dec 29, 2023
Copy link
Collaborator

@mysticaltech mysticaltech left a comment

Choose a reason for hiding this comment

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

Indeed, good catch!

@aleksasiriski
Copy link
Member

It is required for "additional ports" option I've added to traefik a while ago.

@mnencia
Copy link
Contributor Author

mnencia commented Dec 30, 2023

@aleksasiriski can you show me an example of usage? It doesn't feel right to have it always added to the configuration.

@aleksasiriski
Copy link
Member

I think I added to "always" because it causes no harm, but it can be changed to be added only when the additional_ports isn't an empty list. But the current state of this PR breaks additional_ports functionality which I use for load balancing port 22 for my git instance.

@mnencia mnencia force-pushed the mnencia/remove-unused-entrypoint branch from 8ad781c to a78a147 Compare January 4, 2024 14:44
aleksasiriski
aleksasiriski previously approved these changes Jan 4, 2024
locals.tf Show resolved Hide resolved
@mysticaltech mysticaltech dismissed stale reviews from aleksasiriski and themself via ec1d589 January 6, 2024 03:05
@mnencia mnencia marked this pull request as draft January 8, 2024 09:17
Signed-off-by: Marco Nenciarini <mnencia@kcore.it>
@mnencia mnencia force-pushed the mnencia/remove-unused-entrypoint branch from ec1d589 to f94d3c5 Compare January 11, 2024 12:29
@mnencia
Copy link
Contributor Author

mnencia commented Jan 11, 2024

I have tried hard to reproduce a case that requires that line, but I failed. @aleksasiriski, if you have an example, please provide it to me.

I create a cluster with this branch, and the following configuration

traefik_additional_ports = [{name = "ssh", port = 2222, exposedPort = 2222}]

and after deploying the following code I'm able to ssh in without issues.

---
apiVersion: traefik.io/v1alpha1
kind: IngressRouteTCP
metadata:
  name: ssh
spec:
  entryPoints:
    - ssh
  routes:
  - match: HostSNI(`*`)
    services:
    - name: ssh
      namespace: default
      port: 2222
---
apiVersion: v1
kind: Service
metadata:
  name: ssh
  labels:
    app: ssh
spec:
  ports:
  - port: 2222
  selector:
    app: ssh
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: ssh
  labels:
    app: ssh
spec:
  replicas: 1
  selector:
    matchLabels:
      app: ssh
  template:
    metadata:
      labels:
        app: ssh
    spec:
      containers:
      - name: ssh
        image: lscr.io/linuxserver/openssh-server:latest
        ports:
        - containerPort: 2222
        env:
        - name: PUID
          value: "1000"
        - name: PGID
          value: "1000"
        - name: TZ
          value: "Europe/Rome"
        - name: USER_NAME
          value: "mnencia"
        - name: PUBLIC_KEY
          value: "ssh-ed25519 REDACTED mnencia@laptop"

@aleksasiriski
Copy link
Member

It's great to hear that it works without that line, I haven't had the time to test it out but now you did! Then it really isn't needed after all.

@aleksasiriski aleksasiriski marked this pull request as ready for review January 11, 2024 14:04
Copy link
Member

@aleksasiriski aleksasiriski left a comment

Choose a reason for hiding this comment

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

LGTM

@mnencia mnencia merged commit 5161738 into master Jan 12, 2024
3 checks passed
@mnencia mnencia deleted the mnencia/remove-unused-entrypoint branch January 12, 2024 11:22
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.

None yet

4 participants