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.

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

Separate out annotation assignment logic #3889

Merged

Conversation

alexkursell
Copy link
Contributor

@alexkursell alexkursell commented Mar 12, 2019

What this PR does / why we need it: In controller.go, annotation values were being applied to the created Locations in 4 separate places. This is long, pointless, and causes the annotations supported by the different kinds of Locations to drift apart as new annotations are added and people forget to add the assignment to one of the 4 places.

2 of the more recent examples of added annotations, HTTP2PushPreload and Satisfy only apply the annotations to 1 and 2, respectively, out of the 4.

Here are the annotations that this change will cause to be applied that previously weren't:

Catch-all location:

  • HTTP2PushPreload
  • XForwardedPrefix
  • UsePortInRedirects
  • Connection
  • Logs
  • DefaultBackend
  • CustomHTTPErrors
  • Satisfy

Default location

  • HTTP2PushPreload
  • Satisfy

New Location (not really sure what this one does that the one above it doesn't)

  • HTTP2PushPreload

If any of these annotations actually shouldn't be applied to that type of location, please let me know so I can fix it.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2019
@ElvinEfendi
Copy link
Member

LGTM, I don't know why we would not wanna apply all annotations.

I'll let @aledbf review this as well

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019
@aledbf
Copy link
Member

aledbf commented Mar 14, 2019

@alexkursell @ElvinEfendi we had something like this in the past but that triggered some issues with the default backend when the service has no endpoints. That said, this is passing all the e2e test so I am not sure how to check this is ok or not.

@alexkursell
Copy link
Contributor Author

@aledbf Do you know if there was a specific annotation causing the issue? The only change this should make to the controller behaviour is by adding the annotations I listed above.

@aledbf
Copy link
Member

aledbf commented Mar 14, 2019

Merging. This cleanup makes sense. If we found issues we can review the change and narrow the scope of the changes

@aledbf
Copy link
Member

aledbf commented Mar 14, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, alexkursell, ElvinEfendi

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 merged commit 189e2db into kubernetes:master Mar 14, 2019
@alexkursell alexkursell deleted the simplify-controller-annotations branch March 14, 2019 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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
Development

Successfully merging this pull request may close these issues.

4 participants