Allow tag constraints in FunctionConfig CRs - #1186
Open
mozesl-nokia wants to merge 6 commits into
Open
mozesl-nokia wants to merge 6 commits into
mozesl-nokia wants to merge 6 commits into
Conversation
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com> Co-authored-by: Daniel Gyorgy <daniel.gyorgy@nokia.com> Assisted-by: Cursor:grok-4.6
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
✅ Deploy Preview for kpt-porch ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mozesl-nokia
requested review from
efiacor,
kispaljr and
liamfallon
as code owners
September 17, 2026 12:45
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved semver matching, cache preloading, pod constraint, warmup, and ordering issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds semver constraint support for FunctionConfig tags, updating executor matching, defaults, documentation, and tests.
Changes:
- Adds semver validation, normalization, and matching.
- Applies constraints to Go, binary, and pod execution.
- Updates manifests, API/CRD documentation, and test coverage.
File summaries
| File | Summary |
|---|---|
test/e2e/crd/function_config_test.go |
Adds semver constraint e2e coverage. |
pkg/util/image/image.go |
Adds constraint matching; critical unresolved range-overlap matching issue remains. |
pkg/util/image/image_test.go |
Adds image constraint matching tests. |
pkg/engine/builtinruntime.go |
Uses constraint-aware Go executor matching. |
func/internal/podcachemanager.go |
Applies constraints to pod settings and warmup; unresolved out-of-range overrides and latest warmup issues remain. |
func/internal/podcachemanager_unit_test.go |
Tests constrained pod settings. |
func/internal/executableevaluator_test.go |
Updates binary cache test setup. |
docs/content/en/docs/6_configuration_and_deployments/configurations/components/function-runner-config/function-configuration.md |
Expands constraint configuration guidance. |
docs/content/en/docs/5_architecture_and_components/function-runner/interactions.md |
Documents constraint lookup behavior. |
docs/content/en/docs/5_architecture_and_components/function-runner/functionality/function-evaluation.md |
Documents executor constraints. |
docs/content/en/docs/2_concepts/functions.md |
Updates FunctionConfig examples. |
deployments/porch/23-function-configurations.yaml |
Converts default tags to semver ranges. |
controllers/main.go |
Updates cache prepopulation; invalid tags are not validated on startup. |
controllers/functionconfigs/functionconfigreconciler.go |
Validates and normalizes constraints; deduplication does not preserve first-item order. |
controllers/functionconfigs/functionconfigreconciler_test.go |
Tests validation, normalization, and cache matching. |
api/porchconfig/v1alpha1/function_config_types.go |
Updates FunctionConfig tag documentation. |
api/generated/crds/config.porch.kpt.dev_functionconfigs.yaml |
Updates generated CRD descriptions. |
Review details
Suppressed comments (2)
controllers/main.go:354
- FunctionConfigs are preloaded into the live caches before informer reconciliation, but this path never runs the new semver validation. An invalid
tagsvalue can therefore be served after every restart even thoughReconcilewould reject it, and the invalid cache entry is not removed by that error path. Validate/normalize before prepopulation or skip invalid objects.
store.UpdateBinaryCache(&obj.Spec)
func/internal/podcachemanager.go:263
- This condition only makes TTL/queue/max-pod lookups fall back when the image is outside the PodExecutor constraints; the request path still fetches and passes this same
PodExecutortogetFuncEvalPodClient, soCreatePodapplies itstemplateOverrides(including service account, security context, and resources) to out-of-range images. Select the PodExecutor only whenparsed.Tagsatisfies its tags, otherwise pass nil/default configuration so all PodExecutor settings obey the constraint.
entry, ok := pcm.functionConfigMap.GetFunctionConfig(parsed.BaseName)
if !ok || entry.Spec.PodExecutor == nil || !imageutil.MatchesAnyConstraint(parsed.Tag, entry.Spec.PodExecutor.Tags) {
return pcm.podTTL, pcm.maxWaitlistLength, pcm.maxParallelPodsPerFunction
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+58
to
+63
| func MatchesConfigTags(request string, tags []string) bool { | ||
| if MatchesAnyConstraint(request, tags) { | ||
| return true | ||
| } | ||
| _, err := FindBestSemverMatch(request, tags) | ||
| return err == nil |
Contributor
Author
There was a problem hiding this comment.
This looks quite hard to solve without support from the semver lib. They have several PRs for this, some of which have been waiting for years for review...
Comment on lines
+376
to
+382
| tag := entry.Spec.PodExecutor.Tags[0] | ||
| if tag != "" && tag != "*" { | ||
| if _, err := semver.NewVersion(tag); err != nil { | ||
| klog.V(3).Infof("Skipping warmup for %q: Tags[0]=%q is a semver constraint, not a concrete version", entry.Spec.Image, tag) | ||
| continue | ||
| } | ||
| image = fmt.Sprintf("%s:%s", entry.Spec.Image, tag) |
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
Contributor
Author
|
It may be a good idea to get kptdev/kpt#4636 merged first, and the we could use the regclient lister here for the pod cache warmup |
Signed-off-by: Mózes László Máté <laszlo.mozes@nokia.com>
|
mozesl-nokia
changed the title
Contributor
Author
|
Considering the scope of what I will need to do to make cache warmup work here, I think I will wait until #1119 is merged. |
mozesl-nokia
added
documentation
This file contains hidden or 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
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.
Allow tag constrains in FunctionConfig CRs
Description
Related Issue(s)
Type of Change
Checklist
Additional Notes (Optional)
AI Disclosure
If so, please describe how: