fix(terraform): update less-than/less-than/equals version constraints

[bryan-bar]

Issue:
When using the terraform dependency constraints less-than or less-than/equals to set a max version, only the largest semvar version number is updated. This allows the provider version to be set to a non-existent version.

example:
Latest provider version: 0.7.0
Provider version suggested changed: 0.6.1 -> 0.7.1

fixes #8959

---

Fix:
Update each version index to tighten the constraints:

• less-than/equals allows provider versions up to the latest version or latest requirement
• less-than allows for a provider version up the latest minus 1 against the smallest defined semvar number.

[bryan-bar]

Hello @caspermeijn @bdragon @sachin-sandhu,

If I could get a reviewer set on this PR or comments on how to move forward with this fix please.

It is similar to the versioning issue with cargo in #9828

[abdulapopoola]

Thanks @bryan-bar , can you please resolve the conflicts and we can get this merged. Thanks for fixing this.

[caspermeijn]

I believe the original implementation of update_greatest_version was correct for "<" requirements, but "<=" requirements need to be handles differently.

This is what I did for cargo:

dependabot-core/cargo/lib/dependabot/cargo/update_checker/requirements_updater.rb

Lines 134 to 147 in 4f2cfb5

	def update_range_requirements(string_reqs)
	string_reqs.map do |req|
	next req unless req.match?(/[<>]/)
	ruby_req = Cargo::Requirement.new(req)
	next req if ruby_req.satisfied_by?(target_version)
	raise UnfixableRequirement if req.start_with?(">")
	req.sub(VERSION_REGEX) do |old_version|
	if req.start_with?("<=")
	update_version_string(old_version)
	else
	update_greatest_version(old_version, target_version)

I think you need to focus on update_range for handling "<" and "<=" differently.

[bryan-bar]

@caspermeijn
The less-than operator should still be incremented. I updated the PR to handle both < and <=.

The issue with the function is that < also returned an inconsistent version so I chose to update it.
For example, < 0.3.0 with a latest version of 0.3.7 would return as < 0.4.0
Change the initial version to < 0.3.2 with a latest version of 0.3.7 and the returned version changed to < 0.3.8

It seems index_to_update is setting the wrong index to be incremented and then anything after is set to 0 or skipped over.

Another issue is that terraform does not have wild cards and it also did not expand the version when it has less than 3 segments like it does for some of cargos operators.
For less-than/equals, that would mean incrementing the version and then restricting it (example assuming current version <= 1.6, new version 1.6.5 -> <=1.7, !=1.7) or expanding the version as is ( <= 1.6.5). Another option would be to set a really high version <= 1.6.9999 or converting it to less than < 1.7.

[bryan-bar]

@caspermeijn @abdulapopoola
Also note that Terraform treats non-semantic versions as being expanded out internally. 0.2 == 0.2.0 | 1 == 1.0.0
I added this comment in the code as well.

[Zawadidone]

@bryan-bar are you working on this PR?

[bryan-bar]

@bryan-bar are you working on this PR?

Yes, thanks for the reminder. I just updated the PR.

[caspermeijn]

Great, that you can make this work with a simple code addition. I like the additional tests.

The code looks good to me. I don't have any terraform experience, so I can't confirm this is the behavior that is required for terraform.

[caspermeijn]

I don't fully understand why the old code was not sufficient. Does the old code fail the new tests?

The new code is simpler and seems to do the trick.

[bryan-bar]

Yes, when removing b946d23 the test that fails in relation to the less-than/< operator is this test since it jumps a minor version to 0.4.0 instead of incrementing the patch version when a patch is set to 0, ">= 0.2.1, < 0.3.0, <= 0.3.0":

        context "when not satisfied, 0 patch version" do
          let(:requirement) { ">= 0.2.1, < 0.3.0, <= 0.3.0" }
          let(:latest_version) { "0.3.7" }

          its([:requirement]) { is_expected.to eq(">= 0.2.1, < 0.3.8, <= 0.3.7") }
        end

Error:

  17) Dependabot::Terraform::RequirementsUpdater#updated_requirements when there is a latest version when a =>,<,<= requirement was previously specified when not satisfied, 0 patch version [:requirement] is expected to eq ">= 0.2.1, < 0.3.8, <= 0.3.7"
      Failure/Error: its([:requirement]) { is_expected.to eq(">= 0.2.1, < 0.3.8, <= 0.3.7") }
      
        expected: ">= 0.2.1, < 0.3.8, <= 0.3.7"
             got: ">= 0.2.1, < 0.4.0, <= 0.4.0"
      
        (compared using ==)
      # ./spec/dependabot/terraform/requirements_updater_spec.rb:134:in `block (6 levels) in <top (required)>'
      # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
      # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'

---

This revealed a second issue where a non-zero patch would be incremented as expected, ">= 0.2.1, < 0.3.2, <= 0.3.2", besides the original <= issue, unlike the zero-patch version:

        context "when not satisfied, non-0 patch version" do
          let(:requirement) { ">= 0.2.1, < 0.3.2, <= 0.3.2" }
          let(:latest_version) { "0.3.7" }

          its([:requirement]) { is_expected.to eq(">= 0.2.1, < 0.3.8, <= 0.3.7") }
        end

Error:

  16) Dependabot::Terraform::RequirementsUpdater#updated_requirements when there is a latest version when a =>,<,<= requirement was previously specified when not satisfied, non-0 patch version [:requirement] is expected to eq ">= 0.2.1, < 0.3.8, <= 0.3.7"
      Failure/Error: its([:requirement]) { is_expected.to eq(">= 0.2.1, < 0.3.8, <= 0.3.7") }
      
        expected: ">= 0.2.1, < 0.3.8, <= 0.3.7"
             got: ">= 0.2.1, < 0.3.8, <= 0.3.8"
      
        (compared using ==)
      # ./spec/dependabot/terraform/requirements_updater_spec.rb:141:in `block (6 levels) in <top (required)>'
      # /home/dependabot/common/spec/spec_helper.rb:66:in `block (2 levels) in <top (required)>'
      # /home/dependabot/dependabot-updater/vendor/ruby/3.3.0/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'

[bryan-bar]

Great, that you can make this work with a simple code addition. I like the additional tests.

The code looks good to me. I don't have any terraform experience, so I can't confirm this is the behavior that is required for terraform.

@caspermeijn @abdulapopoola
Who would be the proper person to tag for this PR?

[abdulapopoola]

Thanks @bryan-bar , I'm tagging @randhircs so we can review this week and deploy it if it meets the bar.