Tests run in proxied systems #1230
Conversation
markan
force-pushed
the
CLOUD-294/fix_tests_imp
branch
4 times, most recently
from
6a11445 to
72131d9
Compare
|
@chef/chef-server-maintainers |
markan
force-pushed
the
CLOUD-294/fix_tests_imp
branch
from
72131d9 to
ca48ec8
Compare
In a proxied configuration Pedant needs to be aware of the different between the place the server is listening at (platform.server) and the resource that users actually contact. We frequently construct our expected values based on the first, when the actual API will be returning the second. We add a base_resource_url to the config and platform data to enable us to distinquish that in tests. Signed-off-by: Mark Anderson <[email protected]>
This exposes the base_resource_url in the platform config, and adds a resource_url helper function to assist in constructing URLs. Signed-off-by: Mark Anderson <[email protected]>
markan
force-pushed
the
CLOUD-294/fix_tests_imp
branch
from
ca48ec8 to
cedfb67
Compare
There was a problem hiding this comment.
Code wise, I think this change is fine.
This solves the problem by normalizing every appearance of
platform.serverintoplatform.base_resource_url.
I'm trying to understand the use case better:
when Chef server is proxied, it will be the case that a response JSON object contains references to https://something:8443/organizations/someorg/sth, but the tests don't account for this and expect the string to be https://something/organizations/someorg/sth?
In that case, I wonder if we at all want the :8443 bit to leak out and if it wouldn't be cleaner to have erchef and friends return the "outside address" 🤔
| # Our comparisons are more complex than simple value | ||
| spec.each_with_index do |newspec, x| | ||
| return false unless PedantHashComparator.new(newspec, @mode).matches? value[x] | ||
| return true if spec.all? do |s| |
There was a problem hiding this comment.
[nit] isn't this return true if ... followed by return false equivalent to ending the method with
spec.all? do ...
? 😉
| # | ||
| def normalize(candidate) | ||
| if candidate.is_a?(String) | ||
| candidate.sub(/#{Pedant::Config.pedant_platform.server}/, Pedant::Config.pedant_platform.base_resource_url) |
There was a problem hiding this comment.
Is there an advantage of coercing both the expected and configured value at the time of the comparison versus changing the code that generates the expected URL to always be correct:
https://github.com/chef/chef-server/pull/1230/files#diff-c681a9689ad6f141cb970701a07cbbc3R71
One of my worries is that since we coerce both the expected and actual value, it will be hard to catch cases were we are generating the wrong url.
There was a problem hiding this comment.
You already commented on this in your PR message, apologies for not seeing that. Follow-up comment is below.
|
Whelp, I just finally read this part of your commit message fully:
That is indeed a large change; however, what I don't quite see is why we would need to add the new I'm OK with merging this PR given that if fixes the problem, it just seems to me that we have a number of config items and functions trying to do the same thing in these pedant tests. |
|
I'm a little uncomfortable with this because it adds another level of hidden behaviors that aren't apparent when trying to track down failures - but given the situation, it's a reasonable fix. |
This fixes two issues: First, we were sensitive to order in the array comparator, and weren't totally uniform in our methodology for comparing keys. Second, and more controversially, this normalizes URIs in compared strings. Our pedant tests frequently conflate the address the server is listening at (platform.server) with the URI the client should expect out of the API. This becomes important in cases like Automate/Chef Server all in 1 installs, where the Automate service proxies requests to the Chef Server. This solves the problem by normalizing every appearance of platform.server into platform.base_resource_url. An alternate solution would be to update the pedant tests to distinquish the usages. That would be invasive, changing many tests, and be fragile, every test has to carefully distinguish the two useages, and we don't typically test proxied configuration in regular Chef-Server releases. Normalizing is both elegant and opaque. It is elegant, in that it confines the change to just a few lines of code, and avoids the need to be careful. However it's opaque, in that it introduces yet more abstraction into a codebase which suffers from that already. It also risks confusion, in that any difference report may contain 'differences' that are actually normalized away. Signed-off-by: Mark Anderson <[email protected]> Squash
Signed-off-by: Mark Anderson <[email protected]>
|
I should go a little deeper into the use case to explain why I ended up here. So we get many, many failures where we construct an expected return value using the listen Properly disambiguating that usage requires a large number of changes, and is fragile in that Pedant could be configured to address all API calls in tests to the proxied base url, but Normalizing out the difference is the least fragile change at the cost of some possibly A possible enhancement would be to add 'superstrict' as an option for the comparator, where |
markan
force-pushed
the
CLOUD-294/fix_tests_imp
branch
from
cedfb67 to
92b0d2d
Compare
NOTE: Rebase and merge after CLOUD-294/fix_solr_for_pedant (#1233) lands, as it contains and depends on it.
Passes pedant in the CI environment. (But we have some intermittent test failures on ppc64le Ubuntu 12 testers)
This PR encompasses a series of changes to allow chef-server-ctl test to operate when the main chef-server endpoint is proxied behind another name, such as the automate/chef server all in one install.
When opscode-erchef base_resource_url is set differently from the server address that nginx is listening at (as is often the case when proxying) we construct the expected values in pedant incorrectly, using the server address when we should be using base_resource_url
In some cases :host_headers isn't sufficient, and we need to be flexible on what we accept for input.
I've broken it up into a series of commits for readability. I'd especially like feedback on the approach of modifying PedantHashComparator; the alternate seems to be modifying a large number of tests to disambiguate the URI we use to access the system and the resource we expect to return. See the CLOUD-294/fix_tests_mechanical branch for a sense of what that might look like.