[4.10.x] fix(subscription): do not panic when validating a subscription to an API without context - #1790
Merged
Merged
Conversation
Author
|
Cherry-pick of 43db68d has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
paulatulis
force-pushed
the
mergify/bp/4.10.x/pr-1781
branch
from
August 13, 2026 14:24
db29a9b to
e47441a
Compare
paulatulis
approved these changes
Aug 13, 2026
paulatulis
force-pushed
the
mergify/bp/4.10.x/pr-1781
branch
from
August 13, 2026 16:44
e47441a to
6d94edc
Compare
|
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.



This is an automatic copy of pull request #1781 done by Mergify.
https://gravitee.atlassian.net/browse/APIM-14905
Problem
validateContextRefscalledGetName()on the API's and application'scontextRefwithout checking whether either exists.ContextRef()returnsspec.contextRefverbatim, so an unset context yields a nil pointer wrapped in a non-nil interface, and the admission webhook panics instead of returning a validation error:The user gets a stack trace instead of a reason, and nothing is stored.
Fix
Guard each read with the existing
HasContext()onContextAwareObject, and give each failure its own actionable message:contextRef→ "unable to subscribe to API [x] because it does not reference a management context"contextRef→ equivalent message for the applicationThe mismatch branch is byte-identical to master, so the existing
create_withContext_and_contextMissmatch_test.gois unaffected.Behaviour note for the reviewer
The ticket asks for "if neither side has a context, let the subscription through". This PR rejects that case instead, for two reasons:
Application.contextRefis required by its CRD andApiDefinition/ApiV4DefinitioncontextRefis optional, so "neither" is unreachable in practice — the case that actually panics is API-without-context plus application-with-one, matching the reported stack trace.controllers/apim/subscription/internal/update.go:50), so admitting a subscription to a context-less API would create a resource that can never reconcile.Happy to flip to "skip the check" if you'd rather match the ticket's literal wording.
Tests
test/integration/admission/subscription/create_withContext_and_apiWithoutContext_test.go— API with no context, application with one, asserting the admission error. The subscription is handed to the admission controller directly and never applied: the reconciler cannot build an APIM client for a context-less API, so the resource would never reach a terminal state.
internal/admission/subscription/validate_test.go— table-driven, all six context combinations. Note it lives in package becausevalidateContextRefsis unexported, and CI'smake unitonly runstest/unit/..., so it does not execute in CI as things stand.go test ./internal/...is green today if we want to extend that target (separately — it also revives two dormantinternal/k8stests).Cherry-pick of 43db68d has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally