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.

Sign up for GitHub

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

Jump to bottom

Add CRD schema and fix JSON errors #941

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Add CRD schema and fix JSON errors #941

merged 3 commits into from
Sep 9, 2022

Conversation

snorwin
Copy link
Contributor

@snorwin snorwin commented Sep 1, 2022

The lack of proper schema validation can lead to errors when using libraries that rely on K8s API responses (JSON) to match Go structs. In particular, if there are errors in the unmarshaling a SealedSecret resource, the controller will stop watching and processing all the resources of that kind. To avoid such problems, an OpenAPI scheme was introduced wherever possible.

Description of the change

  • OpenAPI schema generation using controller-gen
  • preserve unknown fields only for .spec.encryptedData
  • ignore errors when unmarshaling .spec.encryptedData as the structure isn't ensured by any schema

Benefits

  • controller still valid processes sealed secrets even if there are any invalid ones
  • OpenAPI schema guides the user to create SealedSecret and prevents structural errors

Possible drawbacks
None.

Applicable issues
Fixes #82 #505

@snorwin snorwin temporarily deployed to vmware-image-builder September 1, 2022 15:48 Inactive
@snorwin snorwin mentioned this pull request Sep 2, 2022
Closed
@mkmik
Copy link
Collaborator

mkmik commented Sep 2, 2022

awesome I was meaning to do this since forever but I was lazy; thank you!

@mkmik
Copy link
Collaborator

mkmik commented Sep 2, 2022 •
edited
Loading

Unfortunately I can no longer approve the CI workflows :-(

@snorwin
Copy link
Contributor Author

snorwin commented Sep 2, 2022 •
edited
Loading

As we now enable validation maybe it would make sense to set Data in .spec.template as optional too, what are you thinking?

	// Keys that should be templated using decrypted data
	// +optional
	Data map[string]string json:"data,omitempty"

@mkmik
Copy link
Collaborator

mkmik commented Sep 2, 2022

yeah making that optional seems reasonable

@snorwin snorwin temporarily deployed to vmware-image-builder September 2, 2022 08:51 Inactive
@snorwin snorwin force-pushed the add-schema-and-fix-json-issues branch from 88a2d93 to fdbcb7e Compare September 2, 2022 14:38
@snorwin snorwin had a problem deploying to vmware-image-builder September 2, 2022 14:39 Failure
@snorwin snorwin temporarily deployed to vmware-image-builder September 5, 2022 08:42 Inactive
@snorwin
Copy link
Contributor Author

snorwin commented Sep 8, 2022

The issues related to this causing a lot of trouble, can someone please check @josvazg.

@josvazg josvazg self-assigned this Sep 8, 2022
@josvazg josvazg added the backlog Issues/PRs that will be included in the project roadmap label Sep 8, 2022
@josvazg
Copy link
Collaborator

josvazg commented Sep 8, 2022

Hi @snorwin, thanks for this fix

I will review on our side and we can hopefully have it merged soon

@josvazg
Copy link
Collaborator

josvazg commented Sep 8, 2022 •
edited
Loading

Seems CONTROLLER_GEN has not been given a default value, so running make manifests would usually fail.

Can we give it a default in the Makefile? Maybe CONTROLLER_GEN ?= controller-gen

Otherwise I would expect the development docs would explain the expected way to call make manifests with parameters.

@snorwin snorwin temporarily deployed to vmware-image-builder September 8, 2022 16:11 Inactive
josvazg
josvazg approved these changes Sep 9, 2022
View reviewed changes
Copy link
Collaborator

@josvazg josvazg left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks a lot for this contribution!

@josvazg josvazg merged commit 7d6ae6e into bitnami-labs:main Sep 9, 2022
@snorwin snorwin deleted the add-schema-and-fix-json-issues branch September 9, 2022 09:29
@stefancoetzee-xneelo
Copy link

Shouldn't we have updated the patch version as we're touching the schema?

@vizv vizv mentioned this pull request Sep 17, 2022
Merged
This was referenced Sep 21, 2022
Merged
Merged
@alemorcuq alemorcuq mentioned this pull request Sep 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josvazg josvazg josvazg approved these changes

@alvneiayu alvneiayu Awaiting requested review from alvneiayu alvneiayu is a code owner

@agarcia-oss agarcia-oss Awaiting requested review from agarcia-oss agarcia-oss is a code owner

@alemorcuq alemorcuq Awaiting requested review from alemorcuq

Assignees

@josvazg josvazg

Labels
backlog Issues/PRs that will be included in the project roadmap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add/restore openapi schema for CRD
4 participants
@snorwin @mkmik @josvazg @stefancoetzee-xneelo