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

Updating GEP-2648 to support multiple target refs #2966

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Updating GEP-2648 to support multiple target refs #2966

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
6 changes: 3 additions & 3 deletions apis/applyconfiguration/apis/v1alpha2/backendtlspolicyspec.go
View file
Open in desktop

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

61 changes: 61 additions & 0 deletions apis/applyconfiguration/apis/v1alpha2/localpolicytargetreference.go
View file
Open in desktop

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

68 changes: 68 additions & 0 deletions apis/applyconfiguration/apis/v1alpha2/localpolicytargetreferencewithsectionname.go
View file
Open in desktop

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

41 changes: 19 additions & 22 deletions apis/applyconfiguration/internal/internal.go
View file
Open in desktop

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

8 changes: 4 additions & 4 deletions apis/applyconfiguration/utils.go
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion apis/v1alpha2/backendtlspolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ type BackendTLSPolicySpec struct {
//
// Support: Implementation-specific for any other resource
//
TargetRef PolicyTargetReferenceWithSectionName `json:"targetRef"`
TargetRef LocalPolicyTargetReferenceWithSectionName `json:"targetRef"`

// TLS contains backend TLS policy configuration.
TLS BackendTLSPolicyConfig `json:"tls"`
Expand Down
39 changes: 28 additions & 11 deletions apis/v1alpha2/policy_types.go
View file
Open in desktop
Copy link
Contributor

@candita candita Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are there few enough people that have implemented Policy that we wouldn't want to make this change by deleting the v1a2 Policy and instead migrate it to v1a3 with the changes? For the same reasons as mentioned in #2955 (comment)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm good suggestion, that might be the best way to handle this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question - I think this is different since we're not directly generating CRDs from these types, so versioning doesn't seem to carry the same weight. It might actually be better to move these types to an internal/unversioned package instead since these primarily exist as a reference point + for use in our relatively new Policy CRDs (BackendTLSPolicy and BackendLBPolicy).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 There may be other CRDs that don't require specific versioning. It would be great to have a policy in place, no pun intended.

Copy link
Contributor

@mikemorris mikemorris Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These feel similar to the types in https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/shared_types.go - moving them to internal/unversioned could be reasonable if we have some safeguards in place to protect against breaking changes that may propagate up to a published CRD that is versioned (like for instance if we just removed a namespace field without considering the implications for whether any published CRD embedding this type may need a revision bump).

Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,29 @@ const (
PolicyLabelKey = "gateway.networking.k8s.io/policy"
)

// PolicyTargetReference identifies an API object to apply a direct or
// LocalPolicyTargetReference identifies an API object to apply a direct or
// inherited policy to. This should be used as part of Policy resources
// that can target Gateway API resources. For more information on how this
// policy attachment model works, and a sample Policy resource, refer to
// the policy attachment documentation for Gateway API.
type PolicyTargetReference struct {
type LocalPolicyTargetReference struct {
// Group is the group of the target resource.
Group Group `json:"group"`

// Kind is kind of the target resource.
Kind Kind `json:"kind"`

// Name is the name of the target resource.
Name ObjectName `json:"name"`
}

// NamespacedPolicyTargetReference identifies an API object to apply a direct or
// inherited policy to, potentially in a different namespace. This should only
// be used as part of Policy resources that need to be able to target resources
// in different namespaces. For more information on how this policy attachment
// model works, and a sample Policy resource, refer to the policy attachment
// documentation for Gateway API.
type NamespacedPolicyTargetReference struct {
Copy link
Contributor

@mikemorris mikemorris Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if it's needed, but it looks like this type may be missing from some places where PolicyTargetReference has been swapped to LocalPolicyTargetReference like apis/applyconfiguration/internal/internal.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch - that was an intentional omission for now, basically leaving out NamespacedPolicyTargetReferenceWithSectionName until we're very sure we need it. It should be easy for us to add in the future or for someone else to recreate on their own if needed, just generally think that namespaced targets in policies should be a rare exception. Happy to change though if you'd prefer I add all the types up front.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, agreed keeping this simpler unless/until we have an actual need is best!

// Group is the group of the target resource.
Group Group `json:"group"`

Expand All @@ -55,17 +72,17 @@ type PolicyTargetReference struct {
Namespace *Namespace `json:"namespace,omitempty"`
}

// PolicyTargetReferenceWithSectionName identifies an API object to apply a direct
// policy to. This should be used as part of Policy resources that can target
// single resources. For more information on how this policy attachment mode
// works, and a sample Policy resource, refer to the policy attachment documentation
// for Gateway API.
// LocalPolicyTargetReferenceWithSectionName identifies an API object to apply a
// direct policy to. This should be used as part of Policy resources that can
// target single resources. For more information on how this policy attachment
// mode works, and a sample Policy resource, refer to the policy attachment
// documentation for Gateway API.
//
// Note: This should only be used for direct policy attachment when references
// to SectionName are actually needed. In all other cases, PolicyTargetReference
// should be used.
type PolicyTargetReferenceWithSectionName struct {
PolicyTargetReference `json:",inline"`
// to SectionName are actually needed. In all other cases,
// LocalPolicyTargetReference should be used.
type LocalPolicyTargetReferenceWithSectionName struct {
LocalPolicyTargetReference `json:",inline"`

// SectionName is the name of a section within the target resource. When
// unspecified, this targetRef targets the entire resource. In the following
Expand Down
Loading