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

Jump to bottom

Removing BackendPolicy #732

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Removing BackendPolicy #732

merged 1 commit into from
Jul 28, 2021

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind cleanup
/kind api-change

What this PR does / why we need it:
This is part of the GEP #713 implementation.

Does this PR introduce a user-facing change?:

BackendPolicy has been removed from the API for v1alpha2. This is in favor of the new policy attachment guidelines introduced by GEP #713.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 22, 2021
@robscott robscott added this to the v1alpha2 milestone Jul 22, 2021
robscott
robscott commented Jul 22, 2021
View reviewed changes
apis/v1alpha2/shared_types.go
Comment on lines -36 to -49
// AnnotationAppProtocol defines the protocol a Gateway should use for
// communication with a Kubernetes Service. This annotation must be present
// on the BackendPolicy resource and the protocol will apply to all Service
// ports that are selected by BackendPolicy.Spec.BackendRefs. If the
// AppProtocol field is available, this annotation should not be used. The
// AppProtocol field, when populated, takes precedence over this annotation.
// The value of this annotation must be also be a valid value for the
// AppProtocol field.
//
// Examples:
//
// - `gateway.networking.k8s.io/app-protocol: https`
// - `gateway.networking.k8s.io/app-protocol: tls`
AnnotationAppProtocol = "gateway.networking.k8s.io/app-protocol"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a significant removal, but given that Kubernetes 1.18+ is so widely used already, I don't think it's worth including workarounds for older Kubernetes versions anymore. Since this one depended on BackendPolicy, it would be difficult to recreate without some kind of intermediate resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened up #737 to track the versions we support.
I was ambivallent about the change but given where we are and where this API is, reworking this in won't have much ROI.
+1

@robscott robscott mentioned this pull request Jul 26, 2021
Merged
hbagdi
hbagdi reviewed Jul 26, 2021
View reviewed changes
site-src/v1alpha2/guides/tls.md
route.

```yaml
{% include 'upstream-tls.yaml' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, this is just the upstream-tls example right? In that case, it's still used by v1alpha1 docs, and probably will be as long as we keep v1alpha1 docs around.

@hbagdi
Copy link
Contributor

hbagdi commented Jul 26, 2021

/lgtm

/hold because:

  • remove the one file that no longer should be present
  • approvals from other maintainers if you feel the need

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2021
hbagdi
hbagdi reviewed Jul 26, 2021
View reviewed changes
site-src/v1alpha2/guides/tls.md
resource.

Please note that the TLS configuration is related to the Service or backend
resource and not related to a specific route resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep some of this text and inform the user to use ReferencePolicy for these purposes? it could help with some education and getting our users and implementers be more aware about the new feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that the Policy Attachment GEP is likely a better fit here, though it will be easier to reference that when #736 merges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a hold on #736 to deal with this there.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2021
@robscott
Removing BackendPolicy
80e72bd 
This is part of the GEP kubernetes-sigs#713 implementation.
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 27, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Jul 28, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2021
@robscott
Copy link
Member Author

Removing the hold on this one since I think we already have clear consensus on the GEP that proposed this.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit dac9bed into kubernetes-sigs:master Jul 28, 2021
@robscott robscott deleted the remove-backendpolicy branch January 8, 2022 01:05
@robscott robscott mentioned this pull request Aug 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbagdi hbagdi hbagdi left review comments

@danehans danehans Awaiting requested review from danehans

@thockin thockin Awaiting requested review from thockin

Assignees

@hbagdi hbagdi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1alpha2
Development

Successfully merging this pull request may close these issues.
3 participants
@robscott @k8s-ci-robot @hbagdi