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

feat: get official field doc #457

Merged
merged 20 commits into from
May 31, 2023
Merged

feat: get official field doc #457

merged 20 commits into from
May 31, 2023

Conversation

golgoth31
Copy link
Contributor

@golgoth31 golgoth31 commented May 23, 2023

Closes #456

πŸ“‘ Description

Make an http call to the oficial k8S api reference page to get field documentation

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

This is currently a first preview to be discussed.

@golgoth31 golgoth31 requested review from a team as code owners May 23, 2023 21:06
@golgoth31 golgoth31 changed the title WIP: feat: Add official doc to output feat: Add official doc to output May 23, 2023
@golgoth31 golgoth31 changed the title feat: Add official doc to output feat: get official field doc May 23, 2023
@golgoth31
Copy link
Contributor Author

a simple exemple to show what it could looks like
before (from version v0.3.4):
image

after (from main):
image

@matthisholleville
Copy link
Contributor

Thank you for your contribution. I think it is an important and interesting information to display to the user. What do you think @AlexsJones ?

Can you test to deploy the following object and check that in case an object returns several errors, the information remains digestible please?

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: fake-ingress
spec:
  rules:
  - host: fake-ingress.example.com
    http:
      paths:
      - backend:
          service:
            name: fake-service
            port:
              number: 80
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - fake-ingress.example.com
    secretName: fake-secret

You should have this result with actual version :

0 k8sgpt/fake-ingress(fake-ingress)
- Error: Ingress k8sgpt/fake-ingress does not specify an Ingress class.
- Error: Ingress uses the service k8sgpt/fake-service which does not exist.
- Error: Ingress uses the secret k8sgpt/fake-secret as a TLS certificate which does not exist.

@golgoth31
Copy link
Contributor Author

Hi,
with the actual version, I have:
0 default/fake-ingress(fake-ingress)

  • Error: Ingress default/fake-ingress does not specify an Ingress class.
  • Error: Ingress uses the service default/fake-service which does not exist.
  • Error: Ingress uses the secret default/fake-secret as a TLS certificate which does not exist.
    Error: Ingress default/fake-ingress doesn't specify an Ingress class. Ingress uses the service default/fake-service, which doesn't exist. Ingress uses the secret default/fake-secret as a TLS certificate, which doesn't exist.

Solution:

  1. Create the fake-service & fake-secret objects.
  2. Add annotation "kubernetes.io/ingress.class: [class-name]" to fake-ingress definition.
  3. Apply the changes to the cluster.

with the patch, I have:
0 default/fake-ingress(fake-ingress)

  • Error: Ingress default/fake-ingress does not specify an Ingress class.
    Official Doc: ingressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource, by a transitive connection (controller -> IngressClass -> Ingress resource). Although the kubernetes.io/ingress.class annotation (simple constant name) was never formally defined, it was widely supported by Ingress controllers to create a direct binding between Ingress controller and Ingress resources. Newly created Ingress resources should prefer using the field. However, even though the annotation is officially deprecated, for backwards compatibility reasons, ingress controllers should still honor that annotation if present.
  • Error: Ingress uses the service default/fake-service which does not exist.
  • Error: Ingress uses the secret default/fake-secret as a TLS certificate which does not exist.
    Error: Ingress default/fake-ingress does not specify an Ingress class.
    Solution:
  1. Define an Ingress class using the ingressClassName field in your YAML file.
  2. If needed for backwards compatibility, add the kubernetes.io/ingress.class annotation.
  3. Make sure the Ingress is referencing a valid service and secret.

@matthisholleville
Copy link
Contributor

Thank you. Shoudn't we have Official Doc part for each errors ?

@golgoth31
Copy link
Contributor Author

I only extract the higher level field. Il have to find Γ  way to parse also the subfields from the doc

@golgoth31
Copy link
Contributor Author

I have an other way to find the information, I'll made some tests ... :)

@golgoth31
Copy link
Contributor Author

Hi,
I have modified the patch to get the schema from the current server and extract data from it. The result is like this:

1 default/fake-ingress(fake-ingress)
- Error: Ingress default/fake-ingress does not specify an Ingress class.
  Kubernetes Doc: ingressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource, by a transitive connection (controller -> IngressClass -> Ingress resource). Although the `kubernetes.io/ingress.class` annotation (simple constant name) was never formally defined, it was widely supported by Ingress controllers to create a direct binding between Ingress controller and Ingress resources. Newly created Ingress resources should prefer using the field. However, even though the annotation is officially deprecated, for backwards compatibility reasons, ingress controllers should still honor that annotation if present.
- Error: Ingress uses the service default/fake-service which does not exist.
  Kubernetes Doc: service references a service as a backend. This is a mutually exclusive setting with "Resource".
- Error: Ingress uses the secret default/fake-secret as a TLS certificate which does not exist.
  Kubernetes Doc: secretName is the name of the secret used to terminate TLS traffic on port 443. Field is left optional to allow TLS routing based on SNI hostname alone. If the SNI host in a listener conflicts with the "Host" header field used by an IngressRule, the SNI host is used for termination and value of the "Host" header is used for routing.

I've added a new field in the common.Failure struct to store the kubernetes doc string.
Once the kind, group and discovery client are set in the kubernetes.K8sApiReference struct, any field can be retreived by getting the path (eg: "spec.rules.http.paths.backend.service")

@matthisholleville
Copy link
Contributor

Thank you very much @golgoth31 ! We really appreciate the contribution and we could use it as option with --with-doc flag on analysis. Can you please add flag to use this option ?

renovate bot and others added 14 commits May 28, 2023 21:46
…pt-ai#451)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
* feat: add configuration api route

Signed-off-by: Matthis Holleville <matthish29@gmail.com>

* feat: rename cache methods

Signed-off-by: Matthis Holleville <matthish29@gmail.com>

---------

Signed-off-by: Matthis Holleville <matthish29@gmail.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…pt-ai#458)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…pt-ai#455)

* Update list.go

Signed-off-by: Krishna Dutt Panchagnula <krishnadutt123@gmail.com>

* fix: updated list.go to handle k8sgpt cache list crashing issue

Signed-off-by: Krishna Dutt Panchagnula <krishnadutt123@gmail.com>

---------

Signed-off-by: Krishna Dutt Panchagnula <krishnadutt123@gmail.com>
Co-authored-by: Alex Jones <alexsimonjones@gmail.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…t to 51ee8ae (k8sgpt-ai#464)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: Johannes Kleinlercher <johannes@kleinlercher.at>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…pt-ai#465)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: Rakshit Gondwal <rakshitgondwal3@gmail.com>
Co-authored-by: Thomas Schuetz <38893055+thschue@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…pt-ai#469)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
…pt-ai#458)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…pt-ai#465)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
renovate bot and others added 2 commits May 28, 2023 21:49
…pt-ai#469)

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Signed-off-by: golgoth31 <golgoth31@users.noreply.github.com>
@golgoth31
Copy link
Contributor Author

Thank you very much @golgoth31 ! We really appreciate the contribution and we could use it as option with --with-doc flag on analysis. Can you please add flag to use this option ?

Thank you,
I've added the with-doc flag, to ease the use of the flag, I move the getting of the openapi schema in the runanalisys function.
I enforced the with-doc to false in server mode. I can make a PR to the protobuf if needed.

Copy link
Contributor

@matthisholleville matthisholleville left a comment

Choose a reason for hiding this comment

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

Left few comments. Good job ! Can you please add usage example in README please ?

pkg/analysis/output.go Show resolved Hide resolved
pkg/kubernetes/apireference.go Outdated Show resolved Hide resolved
pkg/kubernetes/apireference.go Outdated Show resolved Hide resolved
Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
Copy link
Contributor

@matthisholleville matthisholleville left a comment

Choose a reason for hiding this comment

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

LGTM.

Another review @AlexsJones ?

Signed-off-by: David Sabatie <david.sabatie@notrenet.com>
@golgoth31
Copy link
Contributor Author

The readme has been updated

@AlexsJones
Copy link
Member

This is a great first-time contribution thanks @golgoth31

@AlexsJones AlexsJones merged commit f9621af into k8sgpt-ai:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature]: get official field doc
6 participants