-
Notifications
You must be signed in to change notification settings - Fork 10
apps: require either letsencrypt or a seperate issuer #2493
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
I am not an expert on configuring the schema, but I did some testing to ensure that it gets validated when letsencrypt is enabled, which is the default in Welkin. I added some comments with suggestions that seemed to achieve what you wanted.
Co-authored-by: anders-elastisys <[email protected]>
Ah thanks, i think i assumed that extraIssuers would be unset completely instead of an empty array |
@@ -6634,6 +6651,17 @@ properties: | |||
extraIssuers: | |||
title: Extra Issuers | |||
type: array | |||
items: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests complains about missing title
and description
, could you add this as well? :)
@@ -6634,6 +6651,17 @@ properties: | |||
extraIssuers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to also be complaining about missing description for this
@@ -6634,6 +6651,17 @@ properties: | |||
extraIssuers: | |||
title: Extra Issuers | |||
type: array | |||
items: | |||
$ref: "https://github.com/datreeio/CRDs-catalog/raw/refs/heads/main/cert-manager.io/clusterissuer_v1.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit worried about the security and performance of fetching these over the Internet, which is why we haven't done it like this before. Not sure how to proceed with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance shouldn't really be noticable, but it might not work in an airgapped situation
Security-wise i'm not too worried, although it's probably a good idea to pin a commit hash
Another way to do it is $ref: "file://./compliantkubernetes-apps/config/schemas/clusterissuer_v1.json"
, just have to switch directory before running yajsv, could maybe do it with a submodule as well to reduce maintenance burden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way I looked at before was to pass referenced schemas via yajsv -r path/to/schemas
, but at the time decided against because it made local references exceedingly verbose as they needed to be absolute URLs (matching the $id
of the referenced schemas)
Warning
This is a public repository, ensure not to disclose:
What kind of PR is this?
Required: Mark one of the following that is applicable:
Optional: Mark one or more of the following that are applicable:
Important
Breaking changes should be marked
kind/admin-change
orkind/dev-change
depending on typeCritical security fixes should be marked with
kind/security
What does this PR do / why do we need this PR?
There needs to be a clusterissuer, but some users may have their own issuer like sectigo or harica
Information to reviewers
You can test the fetching manually with
yajsv
Checklist