Skip to content

Commit 9e2dc5b

Browse files
fix: address PR review feedback for Route reconciler and RBAC
- Fix annotation reconciliation loop: compare only managed annotations in semanticRouteEquals and merge (not replace) annotations on update, preventing infinite loop with OpenShift router controller - Remove unconstrained clusterrolebindings RBAC from base role by fixing Makefile controller-gen path to not recurse into distro/ subpackage - Clear isvc.Status.URL and Address on force-stop to avoid stale data - Add status update on reconcilePlatformPermissions failure for consistency - Add warning log when setRouteTargetPort falls back to first port
1 parent 38ddffe commit 9e2dc5b

7 files changed

Lines changed: 26 additions & 53 deletions

File tree

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ verify-helm-helpers-consistency:
130130
# Generate manifests e.g. CRD, RBAC etc.
131131
manifests: controller-gen kustomize yq
132132
@$(CONTROLLER_GEN) $(CRD_OPTIONS) paths=./pkg/apis/serving/... output:crd:dir=config/crd/full
133-
@$(CONTROLLER_GEN) rbac:roleName=kserve-manager-role paths={./pkg/controller/v1alpha1/inferencegraph,./pkg/controller/v1alpha1/trainedmodel,./pkg/controller/v1beta1/...} output:rbac:artifacts:config=config/rbac
133+
@$(CONTROLLER_GEN) rbac:roleName=kserve-manager-role paths={./pkg/controller/v1alpha1/inferencegraph,./pkg/controller/v1alpha1/trainedmodel,./pkg/controller/v1beta1/inferenceservice} output:rbac:artifacts:config=config/rbac
134134
@$(CONTROLLER_GEN) rbac:roleName=kserve-llmisvc-manager-role paths=./pkg/controller/v1alpha2/llmisvc output:rbac:artifacts:config=config/rbac/llmisvc
135135
@$(CONTROLLER_GEN) rbac:roleName=kserve-localmodel-manager-role paths=./pkg/controller/v1alpha1/localmodel output:rbac:artifacts:config=config/rbac/localmodel
136136
@$(CONTROLLER_GEN) rbac:roleName=kserve-localmodelnode-agent-role paths=./pkg/controller/v1alpha1/localmodelnode output:rbac:artifacts:config=config/rbac/localmodelnode

charts/kserve-resources/files/kserve/resources.yaml

Lines changed: 0 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

config/rbac/role.yaml

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -173,18 +173,6 @@ rules:
173173
- get
174174
- patch
175175
- update
176-
- apiGroups:
177-
- rbac.authorization.k8s.io
178-
resources:
179-
- clusterrolebindings
180-
verbs:
181-
- create
182-
- delete
183-
- get
184-
- list
185-
- patch
186-
- update
187-
- watch
188176
- apiGroups:
189177
- rbac.authorization.k8s.io
190178
resourceNames:

hack/setup/quick-install/kserve-knative-mode-full-install-with-manifests.sh

Lines changed: 0 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

hack/setup/quick-install/kserve-standard-mode-full-install-with-manifests.sh

Lines changed: 0 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/controller/v1beta1/inferenceservice/controller.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,9 @@ func (r *InferenceServiceReconciler) Reconcile(ctx context.Context, req ctrl.Req
392392

393393
if deploymentMode == constants.Standard || deploymentMode == constants.LegacyRawDeployment {
394394
if err := reconcilePlatformPermissions(ctx, r.Client, isvc); err != nil {
395+
if updateErr := r.updateStatus(ctx, isvc, deploymentMode); updateErr != nil {
396+
r.Log.Error(updateErr, "Error updating status after platform permissions reconcile failure")
397+
}
395398
return reconcile.Result{}, errors.Wrapf(err, "fails to reconcile platform permissions")
396399
}
397400
}

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ocp_route_reconciler.go

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ func (r *RawOCPRouteReconciler) Reconcile(ctx context.Context, isvc *v1beta1.Inf
9393
}
9494
}
9595
}
96+
isvc.Status.URL = nil
97+
isvc.Status.Address = nil
9698
isvc.Status.SetCondition(v1beta1.IngressReady, &apis.Condition{
9799
Type: v1beta1.IngressReady,
98100
Status: corev1.ConditionFalse,
@@ -137,7 +139,12 @@ func (r *RawOCPRouteReconciler) reconcileExposed(
137139
if !semanticRouteEquals(desiredRoute, existingRoute) {
138140
existingRoute.Spec = desiredRoute.Spec
139141
existingRoute.Labels = desiredRoute.Labels
140-
existingRoute.Annotations = desiredRoute.Annotations
142+
if existingRoute.Annotations == nil {
143+
existingRoute.Annotations = map[string]string{}
144+
}
145+
for k, v := range desiredRoute.Annotations {
146+
existingRoute.Annotations[k] = v
147+
}
141148
log.Info("Updating Route", "name", isvc.Name, "namespace", isvc.Namespace)
142149
if err := r.client.Update(ctx, existingRoute); err != nil {
143150
return ctrl.Result{}, fmt.Errorf("failed to update Route: %w", err)
@@ -325,6 +332,8 @@ func setRouteTargetPort(authEnabled bool, svc *corev1.Service) (intstr.IntOrStri
325332
}
326333
}
327334
if len(svc.Spec.Ports) > 0 {
335+
log.Info("Desired port not found, falling back to first port",
336+
"service", svc.Name, "desiredPort", desiredName, "fallbackPort", svc.Spec.Ports[0].Name)
328337
if svc.Spec.Ports[0].Name != "" {
329338
return intstr.FromString(svc.Spec.Ports[0].Name), nil
330339
}
@@ -360,7 +369,16 @@ func setRouteTimeout(route *routev1.Route, isvc *v1beta1.InferenceService) {
360369
}
361370

362371
func semanticRouteEquals(desired, existing *routev1.Route) bool {
363-
return equality.Semantic.DeepEqual(desired.Spec, existing.Spec) &&
364-
equality.Semantic.DeepEqual(desired.Labels, existing.Labels) &&
365-
equality.Semantic.DeepEqual(desired.Annotations, existing.Annotations)
372+
if !equality.Semantic.DeepEqual(desired.Spec, existing.Spec) {
373+
return false
374+
}
375+
if !equality.Semantic.DeepEqual(desired.Labels, existing.Labels) {
376+
return false
377+
}
378+
for key, val := range desired.Annotations {
379+
if existing.Annotations[key] != val {
380+
return false
381+
}
382+
}
383+
return true
366384
}

0 commit comments

Comments
 (0)