Skip to content

OCPBUGS-22332: route: Improve errors for missing custom-host permissions #1677

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.

Sign up for GitHub

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

Merged
openshift-merge-bot merged 1 commit into from
May 28, 2024
Merged

OCPBUGS-22332: route: Improve errors for missing custom-host permissions #1677

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 36 additions & 14 deletions pkg/route/hostassignment/assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"fmt"

authorizationv1 "k8s.io/api/authorization/v1"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
apiequality "k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/endpoints/request"
Expand All @@ -15,8 +15,20 @@ import (
"github.com/openshift/library-go/pkg/route"
)

// HostGeneratedAnnotationKey is the key for an annotation set to "true" if the route's host was generated
const HostGeneratedAnnotationKey = "openshift.io/host.generated"
const (
// HostGeneratedAnnotationKey is the key for an annotation set to "true"
// if the route's host was generated.
HostGeneratedAnnotationKey = "openshift.io/host.generated"
// routeHostPermissionErrMsg is the error message for trying to set a
// route's spec.host field without the required permission.
routeHostPermissionErrMsg = "you do not have permission to set the host field of the route"
// routeSubdomainPermissionErrMsg is the error message for trying to set
// a route's spec.subdomain field without the required permission.
routeSubdomainPermissionErrMsg = "you do not have permission to set the subdomain field of the route"
// routeTLSPermissionErrMsg is the error message for trying to set a
// route's spec.tls field or subfields without the required permission.
routeTLSPermissionErrMsg = "you do not have permission to set certificate fields on the route"
)

type HostnameGenerator interface {
GenerateHostname(*routev1.Route) (string, error)
Expand Down Expand Up @@ -65,9 +77,9 @@ func AllocateHost(ctx context.Context, route *routev1.Route, sarc route.SubjectA
}
if !res.Status.Allowed {
if hostSet {
return field.ErrorList{field.Forbidden(field.NewPath("spec", "host"), "you do not have permission to set the host field of the route")}
return field.ErrorList{field.Forbidden(field.NewPath("spec", "host"), routeHostPermissionErrMsg)}
}
return field.ErrorList{field.Forbidden(field.NewPath("spec", "tls"), "you do not have permission to set certificate fields on the route")}
return field.ErrorList{field.Forbidden(field.NewPath("spec", "tls"), routeTLSPermissionErrMsg)}
}
}

Expand Down Expand Up @@ -140,6 +152,16 @@ func certificateChangeRequiresAuth(route, older *routev1.Route, opts route.Route
}
}

// validateImmutableField is equivalent to apimachinery.ValidateImmutableField
// except that it uses a custom error message.
func validateImmutableField(newVal, oldVal interface{}, fldPath *field.Path, errMsg string) field.ErrorList {
allErrs := field.ErrorList{}
if !apiequality.Semantic.DeepEqual(oldVal, newVal) {
allErrs = append(allErrs, field.Invalid(fldPath, newVal, errMsg))
}
return allErrs
}

// ValidateHostUpdate checks if the user has the correct permissions based on the updates
// done to the route object. If the route's host/subdomain has been updated it checks if
// the user has "update" permission on custom-host subresource. If only the certificate
Expand Down Expand Up @@ -183,10 +205,10 @@ func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc r
}
if !res.Status.Allowed {
if hostChanged {
return apimachineryvalidation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host"))
return field.ErrorList{field.Invalid(field.NewPath("spec", "host"), route.Spec.Host, routeHostPermissionErrMsg)}
}
if subdomainChanged {
return apimachineryvalidation.ValidateImmutableField(route.Spec.Subdomain, older.Spec.Subdomain, field.NewPath("spec", "subdomain"))
return field.ErrorList{field.Invalid(field.NewPath("spec", "subdomain"), route.Spec.Subdomain, routeSubdomainPermissionErrMsg)}
}

// if tls is being updated without host being updated, we check if 'create' permission exists on custom-host subresource
Expand All @@ -213,18 +235,18 @@ func ValidateHostUpdate(ctx context.Context, route, older *routev1.Route, sarc r
}
if !res.Status.Allowed {
if route.Spec.TLS == nil || older.Spec.TLS == nil {
return apimachineryvalidation.ValidateImmutableField(route.Spec.TLS, older.Spec.TLS, field.NewPath("spec", "tls"))
return validateImmutableField(route.Spec.TLS, older.Spec.TLS, field.NewPath("spec", "tls"), routeTLSPermissionErrMsg)
}
errs := apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.CACertificate, older.Spec.TLS.CACertificate, field.NewPath("spec", "tls", "caCertificate"))
errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.Certificate, older.Spec.TLS.Certificate, field.NewPath("spec", "tls", "certificate"))...)
errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"))...)
errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.Key, older.Spec.TLS.Key, field.NewPath("spec", "tls", "key"))...)
errs := validateImmutableField(route.Spec.TLS.CACertificate, older.Spec.TLS.CACertificate, field.NewPath("spec", "tls", "caCertificate"), routeTLSPermissionErrMsg)
errs = append(errs, validateImmutableField(route.Spec.TLS.Certificate, older.Spec.TLS.Certificate, field.NewPath("spec", "tls", "certificate"), routeTLSPermissionErrMsg)...)
errs = append(errs, validateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"), routeTLSPermissionErrMsg)...)
errs = append(errs, validateImmutableField(route.Spec.TLS.Key, older.Spec.TLS.Key, field.NewPath("spec", "tls", "key"), routeTLSPermissionErrMsg)...)

if opts.AllowExternalCertificates {
if route.Spec.TLS.ExternalCertificate == nil || older.Spec.TLS.ExternalCertificate == nil {
errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"))...)
errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate, older.Spec.TLS.ExternalCertificate, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
} else {
errs = append(errs, apimachineryvalidation.ValidateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"))...)
errs = append(errs, validateImmutableField(route.Spec.TLS.ExternalCertificate.Name, older.Spec.TLS.ExternalCertificate.Name, field.NewPath("spec", "tls", "externalCertificate"), routeTLSPermissionErrMsg)...)
}
}
return errs
Expand Down