`kctrl package release` creates an invalid OpenAPI spec from Helm Chart #1630

nebhale · 2024-10-02T22:20:40Z

What steps did you take:

Given the following:

# helm/Chart.yaml
apiVersion: v2
name: test
version: 0.1.0

# helm/values.yaml
credentials:
  username: null
  password: null

# package-build.yml
apiVersion: kctrl.carvel.dev/v1alpha1
kind: PackageBuild
metadata:
  creationTimestamp: null
  name: test.nebhale.com
spec:
  template:
    spec:
      app:
        spec:
          deploy:
          - kapp: {}
          template:
          - helmTemplate:
              path: helm
          - ytt:
              paths:
              - '-'
          - kbld: {}
      export:
      - includePaths:
        - helm

# package-resources.yml
apiVersion: data.packaging.carvel.dev/v1alpha1
kind: Package
metadata:
  creationTimestamp: null
  name: test.nebhale.com.0.0.0
spec:
  refName: test.nebhale.com
  releasedAt: null
  template:
    spec:
      deploy:
      - kapp: {}
      fetch:
      - git: {}
      template:
      - helmTemplate:
          path: helm
      - ytt:
          paths:
          - '-'
      - kbld: {}
  valuesSchema:
    openAPIv3: null
  version: 0.0.0

---
apiVersion: data.packaging.carvel.dev/v1alpha1
kind: PackageMetadata
metadata:
  creationTimestamp: null
  name: test.nebhale.com
spec:
  displayName: test
  longDescription: test.nebhale.com
  shortDescription: test.nebhale.com

---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
  annotations:
    kctrl.carvel.dev/local-fetch-0: .
  creationTimestamp: null
  name: test
spec:
  packageRef:
    refName: test.nebhale.com
    versionSelection:
      constraints: 0.0.0
  serviceAccountName: test-sa
status:
  conditions: null
  friendlyDescription: ""
  observedGeneration: 0

run kctrl package release

What happened:
The following, invalid OpenAPIV3 was created. Specifically .spec.valuesSchema.openAPIv3.properties.credentials.properties.[password|username].type="null" is invalid as null is not a valid type by spec.

# carvel-artifacts/package.yml
apiVersion: data.packaging.carvel.dev/v1alpha1
kind: Package
metadata:
  creationTimestamp: null
  name: test.nebhale.com.0.0.0+build.1727907345
spec:
  refName: test.nebhale.com
  releasedAt: "2024-10-02T22:15:55Z"
  template:
    spec:
      deploy:
      - kapp: {}
      fetch:
      - imgpkgBundle:
          image: index.docker.io/bhale382/test@sha256:be8d6e5d70f56cf37a5beff589c52d235e852474f0b9dc805d5199166627fd2c
      template:
      - helmTemplate:
          path: helm
      - ytt:
          paths:
          - '-'
      - kbld:
          paths:
          - '-'
          - .imgpkg/images.yml
  valuesSchema:
    openAPIv3:
      properties:
        credentials:
          properties:
            password:
              default: "null"
              type: "null"
            username:
              default: "null"
              type: "null"
          type: object
      type: object
  version: 0.0.0+build.1727907345

What did you expect:
I'd have expected a valid type to be put into the schema.

Anything else you would like to add:
It appears if the following had been used, the type would have been string.

# helm/values.yaml
credentials:
  username:
  password:

Unfortunately after a run through ytt, the nulls are put into place and that behavior is not preserved. I'd be open to this being the behavior in the place of null as well.

Environment:

kapp Controller version (execute kubectl get deployment -n kapp-controller kapp-controller -o yaml and the annotation is kbld.k14s.io/images): N/A
Kubernetes version (use kubectl version): N/A

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

The text was updated successfully, but these errors were encountered:

100mik · 2024-10-03T09:58:40Z

I am wondering if this is resolved if PackageBuild does not have a ytt section. Maybe init should not be adding it for helm only scenarios.

nebhale · 2024-10-03T13:58:46Z

I'm doing a pre-pass with ytt directly for other reasons, so I don't think it's the one in PackageBuild that's the real problem.

mamachanko · 2025-01-22T08:32:00Z

I am wondering if this is resolved if PackageBuild does not have a ytt section. Maybe init should not be adding it for helm only scenarios.

the issue remains if the ytt section is omitted

❯ git --no-pager d
diff --git a/reproduce/package-build.yml b/reproduce/package-build.yml
index 85188266f..a611767ca 100644
--- a/reproduce/package-build.yml
+++ b/reproduce/package-build.yml
@@ -15,9 +15,6 @@ spec:
           template:
           - helmTemplate:
               path: helm
-          - ytt:
-              paths:
-              - '-'
           - kbld: {}
       export:
       - imgpkgBundle:
diff --git a/reproduce/package-resources.yml b/reproduce/package-resources.yml
index e04a2f141..d2924ce0c 100644
--- a/reproduce/package-resources.yml
+++ b/reproduce/package-resources.yml
@@ -16,9 +16,6 @@ spec:
       template:
       - helmTemplate:
           path: helm
-      - ytt:
-          paths:
-          - '-'
       - kbld: {}
   valuesSchema:
     openAPIv3: null

❯ kctrl package release
...

❯ yq .spec.valuesSchema <carvel-artifacts/packages/test.mamachanko.com/package.yml
openAPIv3:
  properties:
    credentials:
      description: helm/values.yaml
      properties:
        password:
          default: "null"
          type: "null"
        username:
          default: "null"
          type: "null"
      type: object
  type: object

mamachanko · 2025-01-22T10:38:00Z

I've been looking at possible paths ahead.

When a field's value is null

# a field without an explicit type
anything: null

then, kapp-controller could export its OpenAPI v3 schema type by

assuming a nullable, lowest-common denominator type, e.g. string

anything:
  type: string
  nullable: true
  default: null

or,

omitting it from the values schema, or

assuming it to be any

anything:
  oneOf:
  - type: integer
    default: null
    nullable: true
  - type: number
    default: null
    nullable: true
  - type: boolean
    default: null
    nullable: true
  - type: string
    default: null
    nullable: true
  - type: object
    default: null
    nullable: true
  - type: array
    default: null
    nullable: true
    items: {}

There's no easy way to express this other than a union of all types and have them nullable (see: https://swagger.io/docs/specification/v3_0/data-models/data-types/?t#any-type).

The description would have to be attached to each object in oneOf. That's repetitive, but not obviously wrong. After all I think this should motivate the package author to revisit this field in their Helm chart.

My personal take is that ^ options are ordered worst to best. Will see whether I can draft a PR for the any approach.

Furthermore, authors of Helm chart's can include a JSON schema file values.schema.json (see: https://helm.sh/docs/topics/charts/#schema-files) in their chart. However, whether kctrl should consider this JSON schema is a follow-up imo, because anything might be missing from the JSON schema, the schema might disagree with values.yaml, etc.

@nebhale @100mik

nebhale · 2025-01-22T13:27:37Z

I don’t think “omitting it from the values schema” is a viable option anywhere in the list. Practically, the chart I was working with when I opened this issue had a handful of these kinds of keys and they were the values that most commonly needed to be set by users of the chart. Leaving them out really feels like the wrong option in that case, especially if you start thinking that the schema is used for automated input UI generation or validation.

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

mamachanko · 2025-01-22T14:33:40Z

@nebhale @100mik have a look at #1677. mind you, it's desperate for clean up, but it's proofing the any approach.

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

mamachanko · 2025-01-23T14:53:25Z

@nebhale @100mik have a look at #1677. mind you, it's desperate for clean up, but it's proofing the any approach.

polished. ready for review. 🔬

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

nebhale added bug This issue describes a defect or unexpected behavior carvel-triage This issue has not yet been reviewed for validity labels Oct 2, 2024

carvel-bot added this to Carvel Oct 2, 2024

100mik added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Oct 3, 2024

renuy moved this to Prioritized Backlog in Carvel Oct 4, 2024

mamachanko added a commit to mamachanko/kapp-controller that referenced this issue Jan 22, 2025

WIP treat null type as any

2e22d0d

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

mamachanko linked a pull request Jan 22, 2025 that will close this issue

Treat null value as any type when generating schema from Helm chart #1677

Open

4 tasks

mamachanko added a commit to mamachanko/kapp-controller that referenced this issue Jan 23, 2025

Treat null value as any type when generating schema from Helm chart

668024c

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

mamachanko added a commit to mamachanko/kapp-controller that referenced this issue Jan 27, 2025

Treat null value as any type when generating schema from Helm chart

d64e8fe

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

mamachanko added a commit to mamachanko/kapp-controller that referenced this issue Jan 28, 2025

Treat null value as any type when generating schema from Helm chart

49bb275

fixes carvel-dev#1630 Signed-off-by: Max Brauer <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`kctrl package release` creates an invalid OpenAPI spec from Helm Chart #1630

`kctrl package release` creates an invalid OpenAPI spec from Helm Chart #1630

nebhale commented Oct 2, 2024

100mik commented Oct 3, 2024

nebhale commented Oct 3, 2024

mamachanko commented Jan 22, 2025

mamachanko commented Jan 22, 2025 •

edited

Loading

nebhale commented Jan 22, 2025

mamachanko commented Jan 22, 2025

mamachanko commented Jan 23, 2025

kctrl package release creates an invalid OpenAPI spec from Helm Chart #1630

kctrl package release creates an invalid OpenAPI spec from Helm Chart #1630

Comments

nebhale commented Oct 2, 2024

100mik commented Oct 3, 2024

nebhale commented Oct 3, 2024

mamachanko commented Jan 22, 2025

mamachanko commented Jan 22, 2025 • edited Loading

nebhale commented Jan 22, 2025

mamachanko commented Jan 22, 2025

mamachanko commented Jan 23, 2025

`kctrl package release` creates an invalid OpenAPI spec from Helm Chart #1630

`kctrl package release` creates an invalid OpenAPI spec from Helm Chart #1630

mamachanko commented Jan 22, 2025 •

edited

Loading