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

[openapi] empty default should not be specified for required objects with required properties #2987

Open
2 tasks done
diafour opened this issue Nov 8, 2022 · 1 comment
Open
2 tasks done
Labels
area/testing Pull requests that update testing code priority/backlog Issues that have the least priority source/deckhouse-team Issue created by Deckhouse team status/needs-triage Issues or PRs awaiting triage.

Comments

@diafour
Copy link
Member

diafour commented Nov 8, 2022

Preflight Checklist

  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Version

main

Affected modules

  required: ["addressPools"]
...
  addressPools:
    type: array
    default: []
required:
  - auth
...
  auth:
    type: object
    default: {}

Expected Behavior

OpenAPI cases test should not fail for this setup:

MODULE/openapi/values.yaml

type: object
properties:
  internal:
    type: object
    required:
    - someCert
    properties:
      nonRequired:
        type: string
      cert:
        type: object
        default: {}
        required:
          - ca
          - crt
          - key
        properties:
          ca:
            type: string
            x-examples: ["YjY0ZW5jX3N0cmluZwo="]
          crt:
            type: string
            x-examples: [ "YjY0ZW5jX3N0cmluZwo=" ]
          key:
            type: string
            x-examples: [ "YjY0ZW5jX3N0cmluZwo=" ]        

MODULE/openapi/openapi-case-tests.yaml

negative:
  values:
  - internal:
      nonRequired: "some-value"

Actual Behavior

make test-openapi fails because negative case validates successfully.

Steps To Reproduce

No response

Additional Information

The root cause is using default:{} and required for required object. We think it is a valid schema, and JSON schema spec tells us it is:

The default keyword specifies a default value. This value is not used to fill in missing values during the validation process.
Source: JSON Schema spec/Generic keywords

And here is the implementation of the "validation process" in go-openapi/validate:

	createdFromDefaults := map[string]bool{}

	// Property types:
	// - regular Property
	for pName := range o.Properties {
		...

		// Recursively validates each property against its schema
		if v, ok := val[pName]; ok {
			...
		} else if pSchema.Default != nil {
			// If a default value is defined, creates the property from defaults
			// NOTE: JSON schema does not enforce default values to be valid against schema. Swagger does.
			createdFromDefaults[pName] = true
			...
		}
	}

	// Check required properties
	if len(o.Required) > 0 {
		for _, k := range o.Required {
IT IS OK IF REQUIRED FIELD IS NOT SPECIFIED BUT HAS DEFAULT
-----------------------------------------------------v
			if v, ok := val[k]; !ok && !createdFromDefaults[k] {
				res.AddErrors(errors.Required(o.Path+"."+k, o.In, v))
				continue
			}
		}
	}

Source: go-openapi/validate/object_validator.go

It seems we need to remove default: {} from all required certs and other such properties.

Logs

No response

@diafour diafour changed the title [openapi] empty default should not be specified for required fields [openapi] empty default should not be specified for required objects with required properties Nov 8, 2022
@EvgenySamoylov EvgenySamoylov added type/bug priority/backlog Issues that have the least priority area/testing Pull requests that update testing code source/deckhouse-team Issue created by Deckhouse team and removed type/bug labels Nov 9, 2022
@deckhouse-BOaTswain
Copy link
Collaborator

This issue has been automatically put in the triage queue because it has not had
recent activity. The team will reconsider the status of this issue. Thank you
for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Pull requests that update testing code priority/backlog Issues that have the least priority source/deckhouse-team Issue created by Deckhouse team status/needs-triage Issues or PRs awaiting triage.
Projects
None yet
Development

No branches or pull requests

3 participants