Deploy shoot-cert-service on garden runtime cluster #314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gardener-robot
added
kind/api-change
gardener-robot-ci-3
added
reviewed/ok-to-test
MartinWeindel
force-pushed
the
enh/deploy-runtime
branch
from
579ef96 to
55dec76
Compare
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
MartinWeindel
force-pushed
the
enh/deploy-runtime
branch
from
55dec76 to
b35170c
Compare
gardener-robot-ci-2
added
reviewed/ok-to-test
|
/assign |
MartinWeindel
force-pushed
the
enh/deploy-runtime
branch
from
b35170c to
06c3ca4
Compare
gardener-robot-ci-2
added
reviewed/ok-to-test
MartinWeindel
force-pushed
the
enh/deploy-runtime
branch
from
06c3ca4 to
c11d626
Compare
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
MartinWeindel
changed the title
|
/assign |
marc1404
reviewed
Dec 12, 2024
Comment on lines
+87
to
+89
| if !strings.HasPrefix(s, "-----BEGIN CERTIFICATE-----") || !strings.HasSuffix(s, "-----END CERTIFICATE-----") { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("certificate"), shorten(s), "invalid certificate, expected PEM format)")) | ||
| } |
There was a problem hiding this comment.
The same code is repeated below, and it could be extracted into a 🤏 helper function such as validateCertificates.
Just as a thought: It'd be more thorough trying to decode the PEM content to validate or do you think that's too much for a quick validation?
Comment on lines
+153
to
+178
| Entry("Valid specification of CA", config.Configuration{ | ||
| IssuerName: "gardener", | ||
| CA: validCA, | ||
| }, BeEmpty()), | ||
| Entry("Invalid specification of both ACME and CA", config.Configuration{ | ||
| IssuerName: "gardener", | ||
| ACME: validACME, | ||
| CA: validCA, | ||
| }, ConsistOf( | ||
| PointTo(MatchFields(IgnoreExtras, Fields{ | ||
| "Type": Equal(field.ErrorTypeInvalid), | ||
| "Field": Equal("acme"), | ||
| "Detail": Equal("only one of ACME or CA can be specified"), | ||
| })), | ||
| )), | ||
| Entry("Invalid specification of none of ACME and CA", config.Configuration{ | ||
| IssuerName: "gardener", | ||
| ACME: nil, | ||
| CA: nil, | ||
| }, ConsistOf( | ||
| PointTo(MatchFields(IgnoreExtras, Fields{ | ||
| "Type": Equal(field.ErrorTypeRequired), | ||
| "Field": Equal("acme"), | ||
| "Detail": Equal("at least one of ACME or CA must be specified"), | ||
| })), | ||
| )), |
There was a problem hiding this comment.
Unless I missed something the unhappy paths of validateCA could be covered as well.
Comment on lines
+59
to
+61
| extensionscontroller.GetCluster = func(context.Context, client.Reader, string) (*extensions.Cluster, error) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Just for me to understand the changes better: Why should extensionscontroller.GetCluster always return nil when running on the Garden Runtime cluster?
Comment on lines
+8
to
+10
| # TODO: This label approach is deprecated and no longer needed in the future. Remove them as soon as gardener/[email protected] has been released. | ||
| networking.resources.gardener.cloud/from-policy-pod-label-selector: all-scrape-targets | ||
| networking.resources.gardener.cloud/from-policy-allowed-ports: '[{"port":{{ .Values.configuration.serverPortHttp }},"protocol":"TCP"}]' |
There was a problem hiding this comment.
🧹 Can we act on this TODO since gardener/gardener has just hit v1.110? :)
| replicaCount: 1 | ||
|
|
||
| images: | ||
| cert-management: any-repo:any-tag |
There was a problem hiding this comment.
Again, just for me to understand: I assume the image value is overridden somewhere else?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to categorize this PR?
/area control-plane
/kind enhancement
What this PR does / why we need it:
With the introduction of operation extensions, it is possible to deploy an extension on the Garden runtime cluster.
Adjustments both to the deployment of the shoot-cert-service itself and the deployment of the cert-management are needed to deal with the different environment on the Garden runtime cluster. In contrast to the deployment in the shoot namespace on the seed, deploy host and target are the same here. Moreover, some features like
shootIssuers,dnsChallengeOnShoot,alertingare not relevant in this context. Prometheus scraping and Plutono dashboards are also not supported.Addtionally, the default issuer can now be a
CAissuer instead aACMEissuer to support test and private cloud scenarios.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Part of gardener/gardener#9635
Release note: