Skip to content
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

Final changes for RC2 based on sig-net review feedback #898

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Final changes for RC2 based on sig-net review feedback #898

Show file tree
Hide file tree
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
21 changes: 12 additions & 9 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ type HTTPRouteSpec struct {
//
// +optional
// +kubebuilder:validation:MaxItems=16
// +kubebuilder:default={{matches: {{path: {type: "Prefix", value: "/"}}}}}
// +kubebuilder:default={{matches: {{path: {type: "PathPrefix", value: "/"}}}}}
Rules []HTTPRouteRule `json:"rules,omitempty"`
}

Expand Down Expand Up @@ -160,7 +160,7 @@ type HTTPRouteRule struct {
//
// +optional
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:default={{path:{ type: "Prefix", value: "/"}}}
// +kubebuilder:default={{path:{ type: "PathPrefix", value: "/"}}}
Matches []HTTPRouteMatch `json:"matches,omitempty"`

// Filters define the filters that are applied to requests that match
Expand Down Expand Up @@ -209,15 +209,15 @@ type HTTPRouteRule struct {
// Valid PathMatchType values are:
//
// * "Exact"
// * "Prefix"
// * "PathPrefix"
// * "RegularExpression"
//
// Prefix and Exact paths must be syntactically valid:
// PathPrefix and Exact paths must be syntactically valid:
//
// - Must begin with the `/` character
// - Must not contain consecutive `/` characters (e.g. `/foo///`, `//`).
//
// +kubebuilder:validation:Enum=Exact;Prefix;RegularExpression
// +kubebuilder:validation:Enum=Exact;PathPrefix;RegularExpression
type PathMatchType string

const (
Expand All @@ -232,7 +232,10 @@ const (
//
// For example, `/abc`, `/abc/` and `/abc/def` match the prefix
// `/abc`, but `/abcd` does not.
PathMatchPrefix PathMatchType = "Prefix"
//
// "PathPrefix" is semantically equivalent to the "Prefix" path type in the
// Kubernetes Ingress API.
PathMatchPathPrefix PathMatchType = "PathPrefix"
robscott marked this conversation as resolved.
Show resolved Hide resolved

// Matches if the URL path matches the given regular expression with
// case sensitivity.
Expand All @@ -248,12 +251,12 @@ const (
type HTTPPathMatch struct {
// Type specifies how to match against the path Value.
//
// Support: Core (Exact, Prefix)
// Support: Core (Exact, PathPrefix)
//
// Support: Custom (RegularExpression)
//
// +optional
// +kubebuilder:default=Prefix
// +kubebuilder:default=PathPrefix
Type *PathMatchType `json:"type,omitempty"`

// Value of the HTTP path to match against.
Expand Down Expand Up @@ -426,7 +429,7 @@ type HTTPRouteMatch struct {
// specified, a default prefix match on the "/" path is provided.
//
// +optional
// +kubebuilder:default={type: "Prefix", value: "/"}
// +kubebuilder:default={type: "PathPrefix", value: "/"}
Path *HTTPPathMatch `json:"path,omitempty"`

// Headers specifies HTTP request header matchers. Multiple match values are
Expand Down
8 changes: 0 additions & 8 deletions apis/v1alpha2/policy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,4 @@ type PolicyTargetReference struct {
//
// +optional
Namespace *Namespace `json:"namespace,omitempty"`

// ClassName is the name of the class this policy should apply to. When
// unspecified, the policy will apply to all classes that support it.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +optional
ClassName *string `json:"className,omitempty"`
}
7 changes: 7 additions & 0 deletions apis/v1alpha2/referencepolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,11 @@ type ReferencePolicyTo struct {
//
// * Service
Kind Kind `json:"kind"`

// Name is the name of the referent. When unspecified or empty, this policy
// refers to all resources of the specified Group and Kind in the local
// namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, woot!

//
// +optional
Name *ObjectName `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Name be added to ReferenceFrom for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the structural similarities of from and to, I think they're fundamentally different. Since this policy is in the referent namespace, it's helpful to be able to control which of your resources may be referenced. For example only granting access to a specific cert in a namespace that contains many.

On the other hand restricting access from resources you don't control by name is not particularly helpful. The resource owner in the other namespace can just use the allowed name, so it becomes quite easy to bypass any controls intended by the referent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your certificate example, the reference policy allows all gateways from a namespace to reference a specific named certificate. A reasonable extension of that would be to let a specific gateway reference a specific certificate. The names can be aligned by controllers or by convention. However, control over the name of the referee does make this more of an operational safety mechanism than a security one, so maybe it is better not to muddy those concerns.

Not a blocker for me, anyway :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, control over the name of the referee does make this more of an operational safety mechanism than a security one, so maybe it is better not to muddy those concerns.

That's a great way to differentiate these. Agree that it's best not to muddy those concerns yet, but if we start getting feedback/feature requests asking for from.name, we should probably revisit this.

}
4 changes: 2 additions & 2 deletions apis/v1alpha2/validation/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func validateHTTPPathMatch(path *gatewayv1a2.HTTPPathMatch, fldPath *field.Path)
}

switch *path.Type {
case gatewayv1a2.PathMatchExact, gatewayv1a2.PathMatchPrefix:
case gatewayv1a2.PathMatchExact, gatewayv1a2.PathMatchPathPrefix:
if !strings.HasPrefix(*path.Value, "/") {
allErrs = append(allErrs, field.Invalid(fldPath.Child("path"), path, "must be an absolute path"))
}
Expand All @@ -132,7 +132,7 @@ func validateHTTPPathMatch(path *gatewayv1a2.HTTPPathMatch, fldPath *field.Path)
}
case gatewayv1a2.PathMatchRegularExpression:
default:
pathTypes := []string{string(gatewayv1a2.PathMatchExact), string(gatewayv1a2.PathMatchPrefix), string(gatewayv1a2.PathMatchRegularExpression)}
pathTypes := []string{string(gatewayv1a2.PathMatchExact), string(gatewayv1a2.PathMatchPathPrefix), string(gatewayv1a2.PathMatchRegularExpression)}
allErrs = append(allErrs, field.NotSupported(fldPath.Child("pathType"), *path.Type, pathTypes))
}
return allErrs
Expand Down
16 changes: 8 additions & 8 deletions apis/v1alpha2/validation/httproute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand All @@ -69,7 +69,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand All @@ -96,7 +96,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand Down Expand Up @@ -132,7 +132,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand Down Expand Up @@ -248,7 +248,7 @@ func TestValidateHTTPRoute(t *testing.T) {
Matches: []gatewayv1a2.HTTPRouteMatch{
{
Path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
},
Expand Down Expand Up @@ -400,7 +400,7 @@ func TestValidateHTTPPathMatch(t *testing.T) {
{
name: "invalid httpRoute prefix",
path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/."),
},
errCount: 1,
Expand All @@ -416,7 +416,7 @@ func TestValidateHTTPPathMatch(t *testing.T) {
{
name: "invalid httpRoute prefix",
path: &gatewayv1a2.HTTPPathMatch{
Type: pkgutils.PathMatchTypePtr("Prefix"),
Type: pkgutils.PathMatchTypePtr("PathPrefix"),
Value: utilpointer.String("/"),
},
errCount: 0,
Expand Down
14 changes: 8 additions & 6 deletions apis/v1alpha2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions config/crd/v1alpha2/gateway.networking.k8s.io_httproutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions config/crd/v1alpha2/gateway.networking.k8s.io_referencepolicies.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions examples/v1alpha2/basic-http.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ spec:
rules:
- matches:
- path:
type: Prefix
type: PathPrefix
value: /bar
backendRefs:
- name: my-service1
Expand All @@ -47,7 +47,7 @@ spec:
name: great
value: example
path:
type: Prefix
type: PathPrefix
value: /some/thing
method: GET
backendRefs:
Expand Down
2 changes: 1 addition & 1 deletion examples/v1alpha2/default-match-http.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ spec:
port: 80
---
# This HTTPRoute demonstrates patch match defaulting. If no path match is
# specified, CRD defaults adds a default prefix match on the path "/". This
# specified, CRD defaults adds a default PathPrefix match on the path "/". This
# matches every HTTP request and ensures that route rules always have at
# least one valid match.
apiVersion: gateway.networking.k8s.io/v1alpha2
Expand Down
2 changes: 1 addition & 1 deletion examples/v1alpha2/http-redirect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ spec:
rules:
- matches:
- path:
type: Prefix
type: PathPrefix
value: /
backendRefs:
- name: my-filter-svc1
Expand Down
2 changes: 1 addition & 1 deletion examples/v1alpha2/http-routing/foo-httproute.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ spec:
rules:
- matches:
- path:
type: Prefix
type: PathPrefix
value: /login
backendRefs:
- name: foo-svc
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ func Test_PathMatchTypePtr(t *testing.T) {

{
name: "valid path prefix match",
pathType: "Prefix",
expectedPath: gatewayv1a2.PathMatchPrefix,
pathType: "PathPrefix",
expectedPath: gatewayv1a2.PathMatchPathPrefix,
},
{
name: "valid path regular expression match",
Expand Down
8 changes: 0 additions & 8 deletions site-src/geps/gep-713.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,6 @@ type PolicyTargetReference struct {
// +kubebuilder:validation:MaxLength=253
// +optional
Namespace string `json:"namespace,omitempty"`

// ClassName is the name of the class this policy should apply to. When
// unspecified, the policy will apply to all classes that support it.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +optional
ClassName string `json:"className,omitempty"`
}
```

Expand Down