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

Kubernetes SD: Support networking.k8s.io/v1 Ingress #9205

Merged
merged 3 commits into from Aug 24, 2021

Conversation

tksm
Copy link
Contributor

@tksm tksm commented Aug 16, 2021

Signed-off-by: Takashi Kusumi tkusumi@zlab.co.jp

Fixes: #9199

This PR is to support the networking.k8s.io/v1 Ingress for Kubernetes v1.22 and later. The networking.k8s.io/v1beta1 Ingress that Kubernetes SD currently uses was removed in Kubernetes v1.22.

Since the networking.k8s.io/v1 Ingress is not available below Kubernetes v1.19, I made it support both networking.k8s.io/v1 and networking.k8s.io/v1beta1 for backward compatibility. Which Ingress API Kubernetes SD will use is decided based on the Kubernetes version. If the Kubernetes version >= 1.19, it will use networking.k8s.io/v1, otherwise it will use networking.k8s.io/v1beta1.

Notes for reviewers

  • networking.k8s.io/v1 Ingress is introduced in Kubernetes v1.19
    • v1 Ingress is GA since v1.19, so I made it check the Kubernetes version for the API availability rather than calling ServerSupportsVersion(), which makes many API requests.
  • I moved the functions for v1beta1 to V1beta1 suffixed functions. We can remove the suffixed functions after we stop support for Kubernetes below v1.19.
    • Conversion between v1 and v1beta1 needs the Kubernetes internal package, so I gave up on the conversion.

I confirmed that Ingress discovery with this PR worked in the following Kubernetes versions on minikube.

k8s version Ingress support Ingress used
v1.18.20 v1beta1 v1beta1
v1.19.14 v1beta1, v1 v1
v1.22.0 v1 v1

Here is my confirmation procedure.

Confirmation procedure on minikube

Start minikube

Start minikube with the specific Kubernetes version.

minikube start --kubernetes-version v1.22.0

Build the Prometheus image on minikube

eval $(minikube -p minikube docker-env)
make common-docker-amd64

Deploy the custom Prometheus with Ingress setting

Here is the manifest for custom Prometheus with Ingress setting. The image name for Prometheus might need to be changed.

https://gist.github.com/tksm/2de340ba5b057ce8f099f8541c8cb400#file-prometheus-deploy-yaml

kubectl apply -f https://gist.githubusercontent.com/tksm/2de340ba5b057ce8f099f8541c8cb400/raw/0ca2d976d3a49ff6cdf6af3a044448904755eb48/prometheus-deploy.yaml

Create an Ingress resource for test

For Kubernetes v1.19 and later. create an Ingress resource with v1.

https://gist.github.com/tksm/2de340ba5b057ce8f099f8541c8cb400#file-ingress-v1-yaml

kubectl apply -f https://gist.githubusercontent.com/tksm/2de340ba5b057ce8f099f8541c8cb400/raw/0ca2d976d3a49ff6cdf6af3a044448904755eb48/ingress-v1.yaml

For below Kubernetes v1.19, create an Ingress resource with v1beta1.

https://gist.github.com/tksm/2de340ba5b057ce8f099f8541c8cb400#file-ingress-v1beta1-yaml

kubectl apply -f https://gist.githubusercontent.com/tksm/2de340ba5b057ce8f099f8541c8cb400/raw/0ca2d976d3a49ff6cdf6af3a044448904755eb48/ingress-v1beta1.yaml

Check an Ingress target is added

Open the Prometheus Web UI.

minikube service prometheus

Check an ingress target is added and its labels are correctly set in Prometheus UI (/targets).

image

Signed-off-by: Takashi Kusumi <tkusumi@zlab.co.jp>
@tksm tksm requested a review from brancz as a code owner August 16, 2021 01:16
@brancz
Copy link
Member

brancz commented Aug 19, 2021

I'm a little worried about the maintainability aspect of this, we already have open PRs about adding more functionality to the ingress service discovery, is there any way we could make this work without duplication?

@tksm
Copy link
Contributor Author

tksm commented Aug 20, 2021

@brancz Thank you for your comment. I agree with you that it is difficult to maintain.

AFAIK, we cannot convert v1beta1 Ingress to v1 Ingress properly without importing k8s.io/kubernetes, which is not supported as libraries. So how about we introduce an adaptor for v1 and v1beta1?

I pushed my adaptor implementation to a different branch.

@tksm
Copy link
Contributor Author

tksm commented Aug 20, 2021

Alternatively, we can convert only necessary fields of v1beta1 Ingress to v1 Ingress manually. But I think it is prone to programming errors when we will see another field in the future.

I pushed the implementation with partial conversion to a different branch.

@brancz
Copy link
Member

brancz commented Aug 20, 2021

I think I like the adaptor implementation better as with the partial conversion the next contributor will probably assume that some value is there when it might not be because the partial conversion doesn't cover it yet. I'd be happy with moving ahead with the adaptor implementation.

Signed-off-by: Takashi Kusumi <tkusumi@zlab.co.jp>
@tksm
Copy link
Contributor Author

tksm commented Aug 20, 2021

@brancz Thank you for your advice! I added the adaptor implementation to this PR in 40981c4 .

@roidelapluie
Copy link
Member

Closing/reopening for CI to run

@brancz
Copy link
Member

brancz commented Aug 24, 2021

lgtm on green

@roidelapluie
Copy link
Member

roidelapluie commented Aug 24, 2021

There are linting failures, could you please look at them @tksm :

discovery/kubernetes/ingress_adaptor.go:55:3: S1011: should replace loop with `hosts = append(hosts, tls.Hosts...)` (gosimple)
		for _, host := range tls.Hosts {
		^
discovery/kubernetes/ingress_adaptor.go:110:3: S1011: should replace loop with `hosts = append(hosts, tls.Hosts...)` (gosimple)
		for _, host := range tls.Hosts {
		^

Signed-off-by: Takashi Kusumi <tkusumi@zlab.co.jp>
@tksm
Copy link
Contributor Author

tksm commented Aug 24, 2021

@roidelapluie Thank you for pointing it out. I fixed the lint issue in 3ab8559 .

@brancz
Copy link
Member

brancz commented Aug 24, 2021

Squash merging to make the cherry-pick simpler.

@brancz brancz merged commit 2ec6c7d into prometheus:main Aug 24, 2021
@tksm tksm deleted the k8s-networking-v1 branch August 24, 2021 23:41
@brancz brancz mentioned this pull request Aug 27, 2021
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this pull request Aug 29, 2021
…I for `role: ingress` and `role: endpointslices`

This should fix service discovery for these roles in Kubernetes v1.22 and newer versions.
See https://kubernetes.io/docs/reference/using-api/deprecation-guide/#ingress-v122

The corresponding change in Prometheus - prometheus/prometheus#9205
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this pull request Aug 29, 2021
…I for `role: ingress` and `role: endpointslices`

This should fix service discovery for these roles in Kubernetes v1.22 and newer versions.
See https://kubernetes.io/docs/reference/using-api/deprecation-guide/#ingress-v122

The corresponding change in Prometheus - prometheus/prometheus#9205
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.

Kubernetes SD fails to discover Ingress in Kubernetes v1.22
3 participants