squid
squid copied to clipboard
squid-cache
Improve TrafficMode API
TrafficMode users often had to use complex expressions with low-level port flags. Those expressions were often duplicated because the same (implicitly expressed) high-level logic was needed in many places. Source code comments were often necessary to explain those low-level manipulations, but some were missing or stale. Post-configuration code succumbed to the temptation to modify the port configuration (as an optimization) because configuration flags were easily writable. Adding a new flag essentially required checking all those low-level expressions, resulting in an explosion of developer work and costly reviews.
The new API addresses these problems by encapsulating complex conditions in reusable high-level TrafficMode methods while also providing methods to enumerate the primary port modes and to configure raw flags.
TrafficMode now also supports configurations where Squid expects PROXY protocol headers sent to an https_port configured with SslBump support. Code changes that actually enable/allow and use that configuration will be merged separately. This "early" addition reduces review overheads by providing a "complete" TrafficMode API for a single review.
Also eliminated some duplicated flags (such as RequestFlags::intercepted and COMM_INTERCEPTION) and limited COMM_TRANSPARENT scope to spoofing needs only.
Also explicitly prohibit unsupported 'accel' for ftp_port.
Also fixed a bug with mgr:config reporting where every http/https port configuration was printed twice.
Status update: I have closed the discussion threads where I am satisfied. Still have to do a re-review of the new code changes, and merge the listening port PRs for the items I said I would pick up.
Reviewed the API changes in src/anyp/TrafficMode.h and remaining requests are still relevant. Please resolve those name/definition issues before we continue on to adjust their callers in light of the finalized API.
@yadij, during the next review round, I urge you to focus on truth tables rather than method names or descriptions. Once you have no blocking issues with how the compiled code works, we will do review rounds dedicated to polishing names and descriptions to your liking.
@yadij, during the next review round, I urge you to focus on truth tables rather than method names or descriptions. Once you have no blocking issues with how the compiled code works, we will do review rounds dedicated to polishing names and descriptions to your liking.
This request does not make sense to me. The truth table correctness (or not) depends on the name and description matching up with the values in its table.
This request does not make sense to me. The truth table correctness (or not) depends on the name and description matching up with the values in its table.
To the extent possible, think of the compiled code: From that point of view, whether foo() usage is correct in a given context depends on foo()'s truth table and nothing else. Needless to say, good names and accurate descriptions are important and help us a lot. I am not rejecting or even discounting them! However, we are currently wasting too much time on arguing about misunderstandings and falsehoods surrounding them. In this particular case, where the PR author says that the mainline code continues to do what it was doing before (except for a few identified bug fixes), I think it would speed things a lot if we could, to the extent possible, conclude that the code indeed works as before and, in the vast majority of cases, as desired (or fix it) and only then focus on polishing names/descriptions to match the (correct) truth tables.
The same basic idea can be expressed in another way: Look at how the mainline code is using the function, and then try to find an interpretation/variation of the function name and description that would more-or-less match that actual usage. You may not like those particular interpretations/variations, but if they do exist (and in the vast majority of cases they ought to exist!), then the problem becomes much simpler and the path to consensus much shorter.
IMO, it is much easier to agree on a reasonable description of a correct truth table than to navigate comments mixing "foo() should not be using x", "foo() should not mean y", "foo() should not be used in context z", and "foo() should not be named foo" statements!
Said that, if you find my request unreasonable or impractical to satisfy, we will carry on the usual way, of course. No reason for this optimization suggestion to become a showstopper!
@yadij, please move this review-1 PR forward. It has been stuck waiting for your review for more than a month. Thank you.
@yadij, please move this review-1 PR forward. It has been stuck waiting for your review for more than two months. Thank you.
@yadij, please move this review-1 PR forward. It (and the work depending on it) has been stuck waiting for your review for more than three months. Thank you.
@yadij, please move this review-1 PR forward. It (and the work depending on it) has been stuck waiting for your review for more than four months.
If you do not want or unable to review this PR, please dismiss your blocking review and remove the corresponding re-review request. AFAICT, there are no known serious issues in the proposed code. We can polish and merge this code without your help, leaving the remaining arguments about wrappers necessity and names for the time when you can participate in the discussion without unreasonable delays.
FTR; I have re-read and reviewed this discussion on a weekly or so basis and my thinking has not been significantly changed by the responses given to my change requests.
Since we have not reached an agreement on the specific implementation details in a short reasonable timeframe. Please consider this a hard "No" on this PR merging.
Some of the reasons given for doing this change are important, but can be ignored (for now) or resolved in other ways as outlined below:
TrafficMode users often had to use complex expressions with low-level port flags. Those expressions were often duplicated because the same (implicitly expressed) high-level logic was needed in many places.
This is the nature of complex code such as Squid. I agree that perfect code would avoid it, which is why I entertained this PR for so long. As implied by @rousskov the attempt has now taken up a lot of discussion time and actual feature PRs relying on it mean we can no longer afford to entertain this particular attempt at perfect code.
Solution: ignore. Continue with status quo / current code.
Source code comments were often necessary to explain those low-level manipulations, but some were missing or stale.
Non-logic patches correcting stale or wrong documentation to fix this situation are always acceptible.
Solution: documentation updates.
- performing text corrections, or
- adding TODOs for non-serious issues, or
- adding XXX with bug report for serious problems that cannot be fixed (yet).
Post-configuration code succumbed to the temptation to modify the port configuration (as an optimization) because configuration flags were easily writable.
This correctness of such change is debatable and in absence of an agreed overall solution (eg this PR) need to be considered a bug specific to each location doing the change.
Solution: Please open a bug report with specific details about each code area (eg. component) identified doing this and we can go through them case by case to resolve the issued caused. At minimum PRs adding TODOs for future attention can be submitted instead as part of the documentation updates mentioned above.
Adding a new flag essentially required checking all those low-level expressions, resulting in an explosion of developer work and costly reviews.
What is being omitted from this statement is that the same review and design work/time needs to be done on every new flag regardless of this PR change. Any meaning that flag has may impact code paths doing "mode" dependent action/decisions for Squid.
Solution: ignore. We do not agree on this PR being better than status-quo. Sorry.
The new API addresses these problems by encapsulating complex conditions in reusable high-level TrafficMode methods while also providing methods to enumerate the primary port modes and to configure raw flags.
Even so a new flag may require adding a new API function to the wrapper class, and still touching all the code lines that called the old wrapper API. We simply do not know until a new flag is needed.
FYI; A lot of effort is being put in elsewhere by the protocol design people to minimize the need for these flags to make decisions. That had some payoffs for future protocols additions to make this change increasingly less relevant. Even though I do not foresee the mode flags going away completely.
Solution: ignore. We do not agree on this PR being better than status-quo. Sorry.
TrafficMode now also supports configurations where Squid expects PROXY protocol headers sent to an https_port configured with SslBump support. Code changes that actually enable/allow and use that configuration will be merged separately. This "early" addition reduces review overheads by providing a "complete" TrafficMode API for a single review.
The PROXY protocol integration being (discussed elsewhere) can also be done easily using the existing flags object for if-conditions at the appropriate decision points.
Solution: ignore. "nice to have", not a requirement.
Also eliminated some duplicated flags (such as RequestFlags::intercepted and COMM_INTERCEPTION) and limited COMM_TRANSPARENT scope to spoofing needs only.
This we agreed on earlier, is "nice to have" additional extra. I am also inclined favourably towards changes replacing COMM_INTERCEPTION with a test of the existing flags object.
Solution: optionally implement another way.
Also explicitly prohibit unsupported 'accel' for ftp_port.
Another nice discovery. Also a "nice to have" additional extra.
Solution: optionally dedicated PR fixing this config bug using the existing flags object. NP: If it comes with a bugzilla ID documenting issues it can also backport.
Closed all open conversations per request to do a clean re-review. That does not imply the issues discussed there are agreed to. Anything still relevant will be brought up in the new review.
This PR was waiting for sub-PRs #993, #994, and #1006. All those are merged now. There is at least one unaddressed change request that may require another sub-PR -- https://github.com/squid-cache/squid/pull/835#discussion_r810447983 -- so I have labeled this PR as S-waiting-for-PR. I also have a couple of simple changes and comments on my side; I hope to complete that work soon.