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: save YAML spec used to generate support bundle/preflight #1713

Merged
merged 5 commits into from
Jan 4, 2025

Conversation

nvanthao
Copy link
Member

Description, Motivation and Context

As of now, it is not possible to know which final YAML spec is used to generate a support bundle.
Knowing the spec used to generate a support bundle will aid with further troubleshooting, to understand the content of a support bundle, e.g. which collectors, analyzers are used, any render, conditional error?

Demo: https://asciinema.org/a/rTzxKjMRQezLAfNpTLEqJGESZ

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@nvanthao nvanthao added the type::feature New feature or request label Dec 31, 2024
@nvanthao nvanthao requested a review from a team as a code owner December 31, 2024 04:03
@nvanthao nvanthao force-pushed the gerard/sc-114303/save-spec branch from c24a3a1 to 8d3d641 Compare December 31, 2024 04:03
@hedge-sparrow
Copy link
Member

while testing this I noticed that the output spec gets wrapped oddly:

input spec:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: sample
spec:
  collectors:
    - clusterResources: {}
    - clusterInfo:
        exclude: false
  analyzers:
    - clusterPodStatuses:
        checkName: "Pod(s) health status(es)"
        outcomes:
          - fail:
              title: "Pod {{ .Name }} is unable to pull images"
              when: "== ImagePullBackOff"
              message: "A Pod, {{ .Name }}, is unable to pull its image. Status is: {{ .Status.Reason }}. Message is: {{ .Status.Message }}"
          - warn:
              title: "Pod {{ .Name }} is unhealthy"
              when: "!= Healthy"
              message: "A Pod, {{ .Name }}, is unhealthy with a status of: {{ .Status.Reason }}. Message is: {{ .Status.Message }}"
          - pass:
              title: "Pod {{ .Name }} is healthy"
              message: "Pod {{ .Name }} is healthy"

spec.yaml from output bundle:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  creationTimestamp: null
spec:
  analyzers:
  - clusterPodStatuses:
      checkName: Pod(s) health status(es)
      outcomes:
      - fail:
          message: 'A Pod, {{ .Name }}, is unable to pull its image. Status is: {{
            .Status.Reason }}. Message is: {{ .Status.Message }}'
          when: == ImagePullBackOff
      - warn:
          message: 'A Pod, {{ .Name }}, is unhealthy with a status of: {{ .Status.Reason
            }}. Message is: {{ .Status.Message }}'
          when: '!= Healthy'
      - pass:
          message: Pod {{ .Name }} is healthy
  collectors:
  - clusterResources: {}
  - clusterInfo:
      exclude: false
status: {}

@nvanthao
Copy link
Member Author

nvanthao commented Jan 1, 2025

The YAML wrapping and structure ordering differences are expected - we're re-using the same method as the --dry-run flag to render the YAML. While the formatting might look different, the core configuration (collectors + analyzers) remains functionally identical.

// ToYaml returns a yaml document/multi-doc of all the parsed specs
// This function utilises reflection to iterate over all the fields
// of the TroubleshootKinds object then marshals them to yaml.
func (kinds *TroubleshootKinds) ToYaml() (string, error) {
rawList := []string{}
obj := reflect.ValueOf(*kinds)
for i := 0; i < obj.NumField(); i++ {
field := obj.Field(i)
if field.Kind() != reflect.Slice {
continue
}
// skip empty slices to avoid empty yaml documents
for count := 0; count < field.Len(); count++ {
val := field.Index(count)
yamlOut, err := yaml.Marshal(val.Interface())
if err != nil {
return "", err
}
rawList = append(rawList, string(yamlOut))
}
}
return strings.Join(rawList, "---\n"), nil
}

Regarding the missing title field - I noticed it's actually not defined in our Outcome struct, which explains why it's omitted in the output. We'll need to update our documentation to reflect the correct struct fields.

type SingleOutcome struct {
When string `json:"when,omitempty" yaml:"when,omitempty"`
Message string `json:"message,omitempty" yaml:"message,omitempty"`
URI string `json:"uri,omitempty" yaml:"uri,omitempty"`
}
type Outcome struct {
Fail *SingleOutcome `json:"fail,omitempty" yaml:"fail,omitempty"`
Warn *SingleOutcome `json:"warn,omitempty" yaml:"warn,omitempty"`
Pass *SingleOutcome `json:"pass,omitempty" yaml:"pass,omitempty"`
}

Please let me know if this is fine.

hedge-sparrow
hedge-sparrow previously approved these changes Jan 2, 2025
Copy link
Member

@hedge-sparrow hedge-sparrow left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

I like this clean approach of reusing redactors. It however comes with limitations of missing some cases such as redacting TLS certs & keys, complex commands in runPod, exec or run collectors
I tested using this spec and TLS params were not redacted. For this specific case, I think we can add a redactor similar to https://troubleshoot.sh/docs/redact/aws-credentials/ to redact this

---
apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
spec:
  collectors:
    - postgres:
        collectorName: pg
        uri: postgresql://user:password@hostname:5432/defaultdb?sslmode=require
        tls:
          cacert: |
            -----BEGIN CERTIFICATE-----
            MIIC0zCCAbugAwIBAgIUFfxLmF5NnpsVWjZZEYMsOziWLJ0wDQYJKoZIhvcNAQEL
            BQAwEjEQMA4GA1UEAwwHUm9vdCBDQTAeFw0yNTAxMDIxMjM0MjZaFw0yNjAxMDIx
            MjM0MjZaMBIxEDAOBgNVBAMMB1Jvb3QgQ0EwggEiMA0GCSqGSIb3DQEBAQUAA4IB
            DwAwggEKAoIBAQDz9qD7kebmAMODM/nZapSKxzR3Zmfiyej3ZIpUNsx+J1iVnMY0
            du8Nd/coCvrl3zWMNp0o3aC3asf/9pYdluq9nhzlhJ/4lEc3d1O+cvrChB6ZugaO
            YQtNt5yVgaAWPDSUN5y6iR2tefy7blImqmSonbX5CKcowBKmaqxhOM5HfzzYH5Mm
            4goAckHMddxjaCQLKFLi2yluI/4ibR/f5HJkwUWJskXHXqDtUXfyblGYrQiFV3xY
            wjk0lodokTUiSw/Od6klRFaRyCzVnpVu/U5Nu4XICllhKWq4fRl0HO5lVsxYtxMy
            SDkzI9b32H+GJHMnSx9HZS4wxwikFqGJf6zjAgMBAAGjITAfMB0GA1UdDgQWBBQ8
            DrTUuKHO1CUe5xCyglC/j+cYijANBgkqhkiG9w0BAQsFAAOCAQEAwsP3Z9IO7XR/
            3jaIVdaS9qtZyyAzmUpAOGceX32KZYcUKge7P9Uxoc8eEbo6Pl1z/lFL4bRx9fMo
            ncIp6v6uDN/3T3jvB1bC72nmHXwtnqj66Sb13Oww8gksmS8YVWSgrJa2PULKF7Vm
            fkUuatJCaTiJ92tu11+tWOmhLVFP57YuszQyK/rPxajbNcWjWhURep25Z6qVfRT6
            g4Ezs00DeclA5T5rGN6MPnbwc1n/s9blGivY/ThyOBliFUqIs7fiQ64P1u6iPpGp
            akrDN/vtroh3becWAgRbUmSiDEaMZyDfFSHNoHh5B4ubsitPrr5VTWkYZNMV17oY
            jdp5rEI1xQ==
            -----END CERTIFICATE-----
          clientCert: |
            -----BEGIN CERTIFICATE-----
            MIIDETCCAfmgAwIBAgIUOsEL8px9x/r6KSsMwfC32J+DcLswDQYJKoZIhvcNAQEL
            BQAwEjEQMA4GA1UEAwwHUm9vdCBDQTAeFw0yNTAxMDIxMjM0MjZaFw0yNjAxMDIx
            MjM0MjZaMBExDzANBgNVBAMMBnNlcnZlcjCCASIwDQYJKoZIhvcNAQEBBQADggEP
            ADCCAQoCggEBAJ0gphBdscEVO47pG/dl2kWAEnF3O11QHCaxi/2hiiu+R4BfRdO1
            oa6C6FgIlSuj+KCSyYIuFY3YZkOe7+zhjeMxaTfZZZzZKWnL2t7l84Qn9XWXrZiq
            y9xjMyEb1NBm06uy8bgAEtk8gbnkHVuHGSw4mi5MWmP/H9dm6MSjJacYqPdi35cG
            zuuqTHw9hS6VQkuDaaqerrJSfE12gNX3YhTKbdTGtgVRJiI9z7tJJs3LtDpD0H6s
            APzcXQOjRmNHUdYlCrrA0L5qEmEAZ5J+unpbqO+UZhZJoJZySaFnslFQnnvtrJDy
            nEgiyp6yqLYRO0zSXdAuobxNmnpcUiqjRfkCAwEAAaNgMF4wHAYDVR0RBBUwE4IL
            ZXhhbXBsZS5jb22HBH8AAAEwHQYDVR0OBBYEFJQijKu7eBhTqqIh0x9KKtn9RXlC
            MB8GA1UdIwQYMBaAFDwOtNS4oc7UJR7nELKCUL+P5xiKMA0GCSqGSIb3DQEBCwUA
            A4IBAQCctYs1ooIZs3tjKXsGctuz4df2ZFUeKXhTTCAoy5CJxW9Qq4dqKe3J0Cy9
            Ms6sANSXRABXlnkCzrepsaG9dbJRjIsfyNOGv+eCSKyObvXM0eShnHN7rGwpjLE0
            qC+Yozo9tbsEgSMpnlNyB9sm6QK/aR4b9cAq4k5nf9Vi56YMQBNoQheQyD+CfGWa
            C+VOJs1I4aq9iMf/cEh/i4u7dd9xuRxp62Xlbk67McorXBlLDDXvAXvbeZf3MR/Z
            /esYq/TucZeEx5XjVepahbCvNwbtELKk1xlrH78oKAYUCW4/WwFesCiGgwqi6mUh
            BJ0p6Kp9fgaRuE270p6NetwHoWio
            -----END CERTIFICATE-----
          clientKey: |
            -----BEGIN PRIVATE KEY-----
            MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCdIKYQXbHBFTuO
            6Rv3ZdpFgBJxdztdUBwmsYv9oYorvkeAX0XTtaGuguhYCJUro/igksmCLhWN2GZD
            nu/s4Y3jMWk32WWc2Slpy9re5fOEJ/V1l62YqsvcYzMhG9TQZtOrsvG4ABLZPIG5
            5B1bhxksOJouTFpj/x/XZujEoyWnGKj3Yt+XBs7rqkx8PYUulUJLg2mqnq6yUnxN
            doDV92IUym3UxrYFUSYiPc+7SSbNy7Q6Q9B+rAD83F0Do0ZjR1HWJQq6wNC+ahJh
            AGeSfrp6W6jvlGYWSaCWckmhZ7JRUJ577ayQ8pxIIsqesqi2ETtM0l3QLqG8TZp6
            XFIqo0X5AgMBAAECggEAPyzXWBEz3PwafDVBp1DuV69Muw5Dchs5ll0gehOvKDNT
            MEweGScYIMBFhs+8mlVNK9KY7px00hlF1L5cnRN7JvPA6FGiR1QREJaEI8a1CFfA
            m00m4REa8jt4XUGBaWFOjeRex6pP6cQoLIOJQjmpZ1xCsYbFeRskxEh+IkGua+Ye
            yZG3AFSWhei5YkxCZ7kvg820hkPwYGu8082oWgNQDB9qE7H9E6iVL0UvczipiqXl
            fcCHaw5iKHHKSJIueEfpFgSOzYu8ZaWPPz8dKs2GuW0JWs+RYyAUTE0f7F2uS+8u
            Oc65HhJkEpGx4FVNrOL2AGBq3raTSM+BnqlVYkab2QKBgQDU0NCw8/22qLwy3yKK
            8xq5ECsXvUIVd7iDnN5yffGXpAJ4NsQPiJQZlucqa7PpEwgoYRIyFMX69DTG3wET
            DPiMf/NZgCSekFV6UAuGlvKn9nEkFpdHCKPx7S0FZbUuo2ycb6monfY2nQoW53yx
            qIBAxcrt6UNmJvxhAVormHtXdwKBgQC9Av0Lsy+79en32kHj8gjQujJKd1DCTU/o
            pP02AVNveKfgmhC18CkOaTvh80Q1IOC62tNoPqrEd2kNIilP0W/FOIbeYo30Lrja
            UTXj2/Ivei8BNLFg28YPOnF4Izzkx0W0Rsgnp7dMzp4+wwNjAiKGkjyMdEhsL34V
            /ZM4WpWKDwKBgC+FOTRqJxskbnHFlYcFZdAxJg40+o6knxT0cE+Mg+fifZKuV/VI
            ABn+sjustQ20bDvoARIhxVuWMDrADRNd8BofcA1qKcMmY4/eU9SH3ENZKkZurPT1
            nvYkicsMvPpfD5+W54F5VEM5qckXg6aAA7Ny9y9MyPoEdpKKpMHbWJ9dAoGASuGg
            Nr5qruCiLNt/NztwWqEpw265xAC1I8oZtweXcpYujED9VdcrrNXsL3wdDZ9U6TJA
            hxAsv2E/cUCTdVfYHB5k8D3DV3YbLgL2gqtkq4KQlL23eFQZh3bz9VCgk1KPPvi7
            21oKuJczAlJoSRVTcFUHP+3hs1qtbTDk3nKkw+kCgYBEe0N6HgveemCVf7SM4wHK
            EoajmtOXlieCCEcWha/NsdaleXQmFhOgJkjirBj6XGFxEXX8+W2yy8DD027XgR5k
            D5fuQkMNOmIzIKtgRML+W+vC5UUH0i9DIYNxJ5QAk++388bIrvd7ZLvUxIpW+JH/
            2Qi6wSRvRTbThG6ZL93YKw==
            -----END PRIVATE KEY-----

I'm however not sure if we can find a good enough way to redact user crafted commands. Perhaps we would just redact entire commands and environment variables.

@banjoh
Copy link
Member

banjoh commented Jan 2, 2025

    - http:
        collectorName: healthz
        get:
          url: http://api:3000/healthz
          timeout: 5s
          headers:
            Authorization: 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c'

The auth token here does not get redacted as well

@xavpaice
Copy link
Member

xavpaice commented Jan 2, 2025

I like this clean approach of reusing redactors. It however comes with limitations of missing some cases such as redacting TLS certs & keys, complex commands in runPod, exec or run collectors I tested using this spec and TLS params were not redacted. For this specific case, I think we can add a redactor similar to https://troubleshoot.sh/docs/redact/aws-credentials/ to redact this

This is a gap in the redactors, which would block this change - there's no way we can or should store private keys and there's no way to supply them to Troubleshoot otherwise.

I'm however not sure if we can find a good enough way to redact user crafted commands. Perhaps we would just redact entire commands and environment variables.

Let's be very mindful of adding complexity if we can.

This task is getting more complex the more we look at it, and we may need to divert and get a design review before adding more code.

@nvanthao
Copy link
Member Author

nvanthao commented Jan 3, 2025

As discussed,

I've updated the PR to have extra TLS private key redactor for the final YAML file. Please let me know how it goes.
Demo: https://asciinema.org/a/ibWCV4pfF8TodvsdUMGV66fCx

@nvanthao nvanthao requested a review from banjoh January 3, 2025 01:28
@nvanthao nvanthao force-pushed the gerard/sc-114303/save-spec branch from 57601cc to 9e6affc Compare January 3, 2025 04:18
@nvanthao
Copy link
Member Author

nvanthao commented Jan 3, 2025

For context, I decided to redact using yamlPath instead of regex for clarity. As of now, only these collectors are supporting TLS parameter config

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

One more little issue needed still

pkg/redact/redact.go Outdated Show resolved Hide resolved
@nvanthao nvanthao requested a review from banjoh January 3, 2025 21:09
@nvanthao nvanthao merged commit f6f51ac into main Jan 4, 2025
24 checks passed
@nvanthao nvanthao deleted the gerard/sc-114303/save-spec branch January 4, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants