NE-1811: Rename the allowlist route annotation - #564
Conversation
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
Bot
removed
the
lifecycle/rotten
Miciah
changed the title
|
@Miciah: This pull request references NE-1811 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
@Miciah: This pull request references NE-1811 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
e80f8a4 to
5d53eea
Compare
openshift-merge-robot
removed
the
needs-rebase
|
https://github.com/openshift/router/compare/e80f8a4e5e43888509848418c66f7d03e028ba29..5d53eea1cc8c991ea36bcb4f518763fa0a83882a rebases to resolve a conflict from #589. |
|
/assign @gcs278 |
Use the new annotation key "haproxy.router.openshift.io/ip_allowlist" in addition to the old "haproxy.router.openshift.io/ip_whitelist" annotation key. Continue to allow the old annotation key for now, but use the new one if it is present. If a route has annotations with both keys, the annotation with the new key is used, and the annotation with the old key is ignored. Also rename some template helper functions. Note that this is technically a breaking change for custom config templates! Also rename internal consts, variables, functions, struct fields, and HAProxy ACLs to change "whitelist" to "allowlist" and "blacklist" to "denylist". * hack/Dockerfile.debug: * images/router/haproxy/Dockerfile: * images/router/haproxy/Dockerfile.ocp: * images/router/haproxy/Dockerfile.rhel: Replace the "whitelists" file name with "allowlists". * images/router/haproxy/conf/haproxy-config.template: Check for both the old annotation and new annotation keys. Rename the associated acl and file name. * pkg/cmd/infra/router/router.go (RouterSelection): Rename BlacklistedDomains to DenylistedDomains and WhitelistedDomains to AllowlistedDomains. (AdmissionCheck, RouteAdmissionFunc): Update for changes in RouterSelection. * pkg/router/router_test.go (TestMain): Use the new HAProxy allowlist file name. (TestConfigTemplate): Update tests to use the new annotation key and to check for the new HAProxy ACL name and file name. Add a test for the old annotation key to ensure we don't break existing routes. * pkg/router/template/configmanager/haproxy/manager.go (modAnnotationsList): Add the new annotation key (but keep the old one as well as it is still recognized in the template). * pkg/router/template/router.go (whitelistDir): Rename... (allowlistDir): ...to this. * pkg/router/template/template_helper.go (validateHAProxyWhiteList): Rename... (validateHAProxyAllowlist): ...to this. Use the new ValidateAllowlist helper. (generateHAProxyWhiteListFile): Rename... (generateHAProxyAllowlistFile): ...to this. Use the new ValidateAllowlist helper and allowlistDir const, and update an error message. (helperFunctions): Rename validateHAProxyWhiteList and generateHAProxyWhiteListFile to validateHAProxyAllowlist and generateHAProxyAllowlistFile, respectively. * pkg/router/template/template_helper_test.go (TestGenerateHAProxyWhiteListFile): Rename... (Test_generateHAProxyAllowlistFile): ...to this. Use the new allowlistDir const. Update some error messages and internal struct field names. * pkg/router/template/util/haproxy/whitelist.go: Rename file... * pkg/router/template/util/haproxy/allowlist.go: ...to this. (HAPROXY_MAX_WHITELIST_LENGTH): Rename const... (HAPROXY_MAX_ALLOWLIST_LENGTH): ...to this. (ValidateWhiteList): Rename... (ValidateAllowlist): ...to this. * pkg/router/template/util/haproxy/whitelist_test.go: Rename file... * pkg/router/template/util/haproxy/allowlist_test.go: ...to this. (TestValidateWhitelist): Rename... (Test_ValidateAllowlist): ...to this.
5d53eea to
242ad71
Compare
|
https://github.com/openshift/router/compare/5d53eea1cc8c991ea36bcb4f518763fa0a83882a..242ad711b57d4440a72cecbb7ea70144319b7b3f updates a couple more places in Dockerfiles and a code comment. |
|
@Miciah: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| "generateHAProxyMap": generateHAProxyMap, //generates a haproxy map content | ||
| "validateHAProxyWhiteList": validateHAProxyWhiteList, //validates a haproxy whitelist (acl) content | ||
| "generateHAProxyWhiteListFile": generateHAProxyWhiteListFile, //generates a haproxy whitelist file for use in an acl | ||
| "validateHAProxyAllowlist": validateHAProxyAllowlist, //validates a haproxy allowlist (acl) content |
There was a problem hiding this comment.
Are you concerned about breaking custom templates at all? I don't think we officially support any form of custom template, so don't have to maintain compatibility?
If it actually is a concern, you could leave "validateHAProxyWhiteList" as an entry in this template.FuncMap to maintain support for custom template. I think its okay to break custom templates though.
There was a problem hiding this comment.
Are you concerned about breaking custom templates at all?
A little bit.
I don't think we officially support any form of custom template, so don't have to maintain compatibility?
We absolutely do not, and never did, support custom templates. That said, it's entirely possible to run an unsupported custom router deployment on OCP, and then of course there's OKD. Support concerns aside, it is unkind to break custom config capriciously, be it customers' or community members'. For what it's worth, when I carelessly broke the dynamic config manager, which isn't enabled in OCP 4, in #51, someone noticed (and then graciously fixed it for us; see #107). However, I do not believe we are being capricious or careless in this case.
If it actually is a concern, you could leave "validateHAProxyWhiteList" as an entry in this
template.FuncMapto maintain support for custom template. I think its okay to break custom templates though.
Yeah, it would be trivial to maintain backwards compatibility. It would be slightly more effort to add a "this helper is deprecated" log message.
Normally I would push back against any kind of breaking change. However, the goal here is specifically to remove the old names, we will document the change in a release note, and it is trivial to update one's custom template once one realizes it is necessary to do so (either from seeing the release note or from testing the custom template in the new .y release before deploying one's unsupported custom template to a production environment).
One more thing to consider is that we cannot really force a migration for unsupported config. At most we could start logging an error message in one release and then remove the helper entirely in a later release. However, the fact that the thing we are talking about here is a custom router deployment means that it isn't being managed by a version of cluster-ingress-operator that we maintain, so our operator cannot necessarily detect the problematic config in order to block an upgrade.
In light of all that, does it seem reasonable just to remove the helpers?
There was a problem hiding this comment.
Yea that sounds convincing enough, I just wanted to be sure. To reiterate:
- This change is different, we are specifically trying to change the function name here
- Custom templates are unsupported
- We'll do a release note
- We don't have a reasonable way to force a migration for these situations anyways
|
It's a pretty straightforward PR. I've tested this PR a cluster just to be sure it works as expected. I have one question, but I don't think it's a concern. @Miciah feel free to remove the hold when you are ready. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
I'm okay with the PR as is, I have no other concerns. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
[ART PR BUILD NOTIFIER] Distgit: ose-haproxy-router-base |
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-haproxy-router |
6 participants
Use the new annotation key "haproxy.router.openshift.io/ip_allowlist" in addition to the old "haproxy.router.openshift.io/ip_whitelist" annotation key. Continue to allow the old annotation key for now, but use the new one if it is present.
If a route has annotations with both keys, the annotation with the new key is used, and the annotation with the old key is ignored.
Also rename some template helper functions. Note that this is technically a breaking change for custom config templates!
Also rename internal consts, variables, functions, struct fields, and HAProxy ACLs to change "whitelist" to "allowlist" and "blacklist" to "denylist".
images/router/haproxy/conf/haproxy-config.template: Check for both the old annotation and new annotation keys. Rename the associated acl and file name.pkg/cmd/infra/router/router.go(RouterSelection): RenameBlacklistedDomainstoDenylistedDomainsandWhitelistedDomainstoAllowlistedDomains.(
AdmissionCheck,RouteAdmissionFunc): Update for changes inRouterSelection.pkg/router/router_test.go(TestMain): Use the new HAProxy allowlist file name.(
TestConfigTemplate): Update tests to use the new annotation key and to check for the new HAProxy ACL name and file name. Add a test for the old annotation key to ensure we don't break existing routes.pkg/router/template/configmanager/haproxy/manager.go(modAnnotationsList): Add the new annotation key (but keep the old one as well as it is still recognized in the template).pkg/router/template/router.go(whitelistDir): Rename...(
allowlistDir): ...to this.pkg/router/template/template_helper.go(validateHAProxyWhiteList): Rename...(
validateHAProxyAllowlist): ...to this. Use the newValidateAllowlisthelper.(
generateHAProxyWhiteListFile): Rename...(
generateHAProxyAllowlistFile): ...to this. Use the newValidateAllowlisthelper andallowlistDirconst, and update an error message.(
helperFunctions): RenamevalidateHAProxyWhiteListandgenerateHAProxyWhiteListFiletovalidateHAProxyAllowlistandgenerateHAProxyAllowlistFile, respectively.pkg/router/template/template_helper_test.go(TestGenerateHAProxyWhiteListFile): Rename...(
Test_generateHAProxyAllowlistFile): ...to this. Use the newallowlistDirconst. Update some error messages and internal struct field names.pkg/router/template/util/haproxy/whitelist.go: Rename file...pkg/router/template/util/haproxy/allowlist.go: ...to this.(
HAPROXY_MAX_WHITELIST_LENGTH): Rename const...(
HAPROXY_MAX_ALLOWLIST_LENGTH): ...to this.(
ValidateWhiteList): Rename...(
ValidateAllowlist): ...to this.pkg/router/template/util/haproxy/whitelist_test.go: Rename file...pkg/router/template/util/haproxy/allowlist_test.go: ...to this.(
TestValidateWhitelist): Rename...(
Test_ValidateAllowlist): ...to this.