fastify-cli icon indicating copy to clipboard operation
fastify-cli copied to clipboard

isDocker is not working on Kubernetes v1.23

Open QuanTran91 opened this issue 2 years ago • 10 comments

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.1.1

Plugin version

No response

Node.js version

16

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

20.04

Description

Plugin is docker is not work on EKS (Elastic Kubernetes Service) 1.23

Steps to Reproduce

Run app in EKS as pod Observe the log that it is running on 127.0.0.1

Expected Behavior

No response

QuanTran91 avatar Jan 10 '23 10:01 QuanTran91

I would say it is a upstream issue. and you are allowed to pass the address by yourself.

climba03003 avatar Jan 10 '23 12:01 climba03003

I would say that isDocker works as expected since K8s doesn't implement a Docker engine, but it would be nice if the fastify-cli adds some detection logic to use 0.0.0.0 as default in K8s environments. I lost 2h yesterday because of this

sinedied avatar Jan 11 '23 16:01 sinedied

Ah! That's the catch. Do you know how can we detect that we running on K8s?

mcollina avatar Jan 11 '23 17:01 mcollina

Seems like you can check for an environment variable: https://stackoverflow.com/questions/36639062/how-do-i-tell-if-my-container-is-running-inside-a-kubernetes-cluster

sinedied avatar Jan 11 '23 20:01 sinedied

Would you accept a PR for this?

sinedied avatar Jan 11 '23 20:01 sinedied

Would you accept a PR for this?

Yes, and I see more complete check on this package https://github.com/ntfs32/is-kubernetes/blob/main/index.js The download is 1 / week, so I have a bit worry on the stability. But, the detection look solid to me.

climba03003 avatar Jan 12 '23 04:01 climba03003

Ok, I'll propose a PR then. Though I would be against adding an external dependency for something that is a 1-liner, as the environment variable is enough (according to the docs, it's always present: https://kubernetes.io/docs/tutorials/services/connect-applications-service/#environment-variables).

sinedied avatar Jan 12 '23 08:01 sinedied

Though I would be against adding an external dependency for something that is a 1-liner, as the environment variable is enough

I have no idea will this change in future but multiple method for detection doesn't hurt.

climba03003 avatar Jan 12 '23 08:01 climba03003

Given that any change to this would break the entire k8s ecosystem, it's very unlikely. I'd rather thrust the official doc than than an unknown repo using a detection methods based on synchronous file reads, which could delay the startup a bit.

I've sent the PR BTW.

sinedied avatar Jan 12 '23 08:01 sinedied

I'd rather thrust the official doc than than an unknown repo using a detection methods based on synchronous file reads, which could delay the startup a bit.

The check is OR which means it only execute when the first one fail. When the environment is always exist, there will be no file read operation. Again, fallback detection methods doesn't hurt in anyway.

When a environment really doesn't set one, then the application is no-way out either. Even worse then consuming sometime for reading file-system.

climba03003 avatar Jan 12 '23 08:01 climba03003