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

Fix dashboard issue with integration nginx_ingress_controller #6249

Merged
merged 2 commits into from
May 24, 2023

Conversation

gsantoro
Copy link
Contributor

What does this PR do?

Change the mapping type of the field nginx_ingress_controller.access.upstream.name to fix an error on the overview dashboard of the integration nginx_ingress_controller.

This error was preventing the top upstreams panel to correctly display any value

Checklist

  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

Author's Checklist

  • the package correctly builds and all tests pass

How to test this PR locally

  1. Setup a dev environment with elastic stack, fleet and kubernetes integration installe
  2. install ingress-nginx helm chart from https://kubernetes.github.io/ingress-nginx
  3. Add a demo application with the following manifests

deployment.yaml

apiVersion: apps/v1
kind: Deployment
metadata:
  name:  demo
  namespace: default
  labels:
    app:  demo
spec:
  selector:
    matchLabels:
      app: demo
  replicas: 1
  strategy:
    rollingUpdate:
      maxSurge: 25%
      maxUnavailable: 25%
    type: RollingUpdate
  template:
    metadata:
      labels:
        app:  demo
    spec:
      # initContainers:
        # Init containers are exactly like regular containers, except:
          # - Init containers always run to completion.
          # - Each init container must complete successfully before the next one starts.
      containers:
      - name:  demo
        image:  nginx:1.23.1-alpine
        resources:
          requests:
            cpu: 100m
            memory: 100Mi
          limits:
            cpu: 100m
            memory: 100Mi
        livenessProbe:
          tcpSocket:
            port: 80
          initialDelaySeconds: 5
          timeoutSeconds: 5
          successThreshold: 1
          failureThreshold: 3
          periodSeconds: 10
        ports:
        - containerPort:  80
          name:  http
        volumeMounts:
        - name: localtime
          mountPath: /etc/localtime
      volumes:
        - name: localtime
          hostPath:
            path: /usr/share/zoneinfo/Europe/London
      restartPolicy: Always

service.yaml

apiVersion: v1
kind: Service
metadata:
  name: demo
  namespace: default
spec:
  selector:
    app: demo
  type: ClusterIP
  sessionAffinity: None
  sessionAffinityConfig:
    clientIP:
      timeoutSeconds: 10800
  ports:
  - name: demo
    protocol: TCP
    port: 8080
    targetPort: 80
    # If you set the `spec.type` field to `NodePort` and you want a specific port number,
    # you can specify a value in the `spec.ports[*].nodePort` field.
    # nodePort: 8080

ingress.yaml to add an ingress rule to expose the service

apiVersion: networking.k8s.io/v1                                                                                                                                                                                   
kind: Ingress                                                                                                                                                                                                      
metadata:                                                                                                                                                                                                          
  name: demo-localhost                                                                                                                                                                                             
  namespace: default                                                                                                                                                                                               
spec:                                                                                                                                                                                                              
  ingressClassName: nginx                                                                                                                                                                                          
  rules:                                                                                                                                                                                                           
  - host: demo.localdev.me                                                                                                                                                                                         
    http:                                                                                                                                                                                                          
      paths:                                                                                                                                                                                                       
      - backend:                                                                                                                                                                                                   
          service:                                                                                                                                                                                                 
            name: demo                                                                                                                                                                                             
            port:                                                                                                                                                                                                  
              number: 80                                                                                                                                                                                           
        path: /                                                                                                                                                                                                    
        pathType: Prefix

client.yaml to simulate traffic

apiVersion: batch/v1
kind: CronJob
metadata:
  name: demo-client
  namespace: default
spec:
  schedule: "*/1 * * * *"
  jobTemplate:
    spec:
      template:
        spec:
          containers:
          - name: demo-client
            image: curlimages/curl:7.86.0
            args: ['/bin/sh', '-c', ' curl --header "Host: demo.localdev.me"  ingress-nginx-controller.ingress-nginx.svc.cluster.local']
          restartPolicy: OnFailure

Related issues

Screenshots

The following screenshot shows that the panl top upstreams is now being filled. It was showing error before.

Screenshot 2023-05-18 at 13 44 31

@gsantoro gsantoro added the bug Something isn't working label May 18, 2023
@gsantoro gsantoro self-assigned this May 18, 2023
@gsantoro gsantoro requested a review from a team as a code owner May 18, 2023 12:52
@elasticmachine
Copy link

elasticmachine commented May 18, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-05-18T12:53:22.229+0000

  • Duration: 14 min 31 sec

Test stats 🧪

Test Results
Failed 0
Passed 28
Skipped 0
Total 28

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (2/2) 💚
Files 100.0% (2/2) 💚 2.958
Classes 100.0% (2/2) 💚 2.958
Methods 92.0% (23/25) 👍 0.196
Lines 94.428% (322/341) 👍 2.395
Conditionals 100.0% (0/0) 💚

@MichaelKatsoulis
Copy link
Contributor

@ruflin or @jsoriano do you think that updating this field from text to keyword would constitute a breaking change? I think it may but it fixes an bug because text type was breaking a dashboard.

@ruflin
Copy link
Member

ruflin commented May 22, 2023

That .name is a text field seems to be a bug as it would conflict with generic ECS patterns that .name is a keyword and could have a text multi field if needed. So I would argue this change as a bug fix is fine. But I agree, some users might consider this a breaking change. It would be nice to provide these users a workaround, for example have a template ready they could add to @custom to enable a multi field with .text on it too. It would still mean, they have to adjust their query.

@gsantoro gsantoro merged commit c17d59c into elastic:main May 24, 2023
3 checks passed
@gsantoro gsantoro deleted the feature/ingress-nginx-fix branch May 24, 2023 07:53
@MichaelKatsoulis
Copy link
Contributor

@gsantoro create a follow up Issue to offer users this workaround suggested by Nicolas

@elasticmachine
Copy link

Package nginx_ingress_controller - 1.7.2 containing this change is available at https://epr.elastic.co/search?package=nginx_ingress_controller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nginx Ingress controller maps nginx_ingress_controller.access.upstream.name as text breaking dashboard
4 participants