resolve environment variables in httpGet probe path

[bachmanity1]

Enhancement Description

• One-line enhancement description (can be used as a release note): resolve environment variables in httpGet probe path
• Kubernetes Enhancement Proposal:
• Discussion Link: feat: resolve environment variables in httpGet probe path kubernetes#128957
• Primary contact (assignee): @bachmanity1
• Responsible SIGs: sig/node
• Enhancement target (which target equals to which milestone):
  • Alpha release target (x.y):
  • Beta release target (x.y):
  • Stable release target (x.y):
• Alpha
  • KEP (k/enhancements) update PR(s):
  • Code (k/k) update PR(s):
  • Docs (k/website) update PR(s):

Please keep this description up to date. This will help the Enhancement Team to track the evolution of the enhancement efficiently.

[bachmanity1]

/sig node

[bachmanity1]

In the PR linked above, it was mentioned that this feature might require a KEP since it introduces a breaking change. However, I believe this would impact a very very small number of users, if any, as I don’t think environment variable expansion syntax is commonly used in the http path. Even if it is used that way, there would need to be a corresponding environment variable defined in the container spec for it to actually cause an issue. Requiring a KEP for this feels like overkill to me. I’m curious to hear what others in SIG Node think about this.

[aojea]

In the PR linked above, it was mentioned that this feature might require a KEP since it introduces a breaking change. However, I believe this would impact a very very small number of users, if any, as I don’t think environment variable expansion syntax is commonly used in the http path. Even if it is used that way, there would need to be a corresponding environment variable defined in the container spec for it to actually cause an issue. Requiring a KEP for this feels like overkill to me. I’m curious to hear what others in SIG Node think about this.

There are multiple factors why a KEP is required, one is to discuss with all the SIGs involved, and this is clearly impacting SIG network, existing implementation of probes has several problems with the network model #4559.

Other reasons to require a KEP are to evaluate the impact and the return of investment and the supportability, probes are very critical to the system and bugs there are very complex to detect , the blast radius of something wrong there is very large, you may introduce the feature for some niche use case but maintainers need to support that feature forever, so we need to balance the cost of maintaining the features vs the number of users that will benefit from it and if there is a small number, if they can use another option, like in this case is exec probes.

Feature gates protect us of possible problems, imaging you introduce this feature without the feature gate and later we receive a security issue because the injection is used by malicious actors and we need to disable the feature (I also will like @tallclair to review this KEP as he already warned about the problems with existing HTTP/TCP probers in kubernetes/kubernetes#99425

Per example, if you introduce a change like this without feature gates and later we need to disable the feature because of a security problem we didn't know, we need to figure out what to do to not break users with a probe like

       readinessProbe:
          httpGet:
            path: "${CONTEXT_PATH}/actuator/health/readiness"
            port: 8080
            scheme: HTTP

[tallclair]

Yes, I agree this needs a KEP, but you might want to have a more casual discussion before then to see whether it's worth taking this to the KEP phase.

Which environment variable context are variables being substituted from? Using the kubelet's context seems like a no-go, since this would leak the Kubelet environment to pods, which run in a different trust domain. Using the container's environment seems complicated, and would probably require a new CRI call to read the environment variables. I'm not sure the use case would justify that complexity.

[bachmanity1]

Which environment variable context are variables being substituted from? Using the kubelet's context seems like a no-go, since this would leak the Kubelet environment to pods, which run in a different trust domain.

My intention was to use container's environment variable context.

Using the container's environment seems complicated, and would probably require a new CRI call to read the environment variables. I'm not sure the use case would justify that complexity.

Yes, it would require an additional CRI call before starting the container, but once the environment variables are cached, no further calls would be needed. You can check out the reference implementation. That said, I agree it’s overly complex and may not be worth the effort. Instead, how about resolving only predefined environment variables? This approach wouldn’t require additional CRI calls, and the implementation would be much simpler.

[sftim]

Let's move this discussion to the PR that adds the text of the proposal, @bachmanity1.