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

feat: expose idle timeout loadbalancer input #569

Merged
merged 8 commits into from
Oct 25, 2023
Merged

Conversation

vburckhardt
Copy link
Member

Description

Expose https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/resources/is_lb_listener#idle_connection_timeout in the lb inputs.

Part of terraform-ibm-modules/terraform-ibm-landing-zone#609

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@vburckhardt vburckhardt changed the title Idle timeout lb feat: expose idle timeout loadbalancer input Oct 24, 2023
@vburckhardt vburckhardt requested review from rajatagarwal-ibm and ocofaigh and removed request for rajatagarwal-ibm October 24, 2023 10:38
@vburckhardt
Copy link
Member Author

/run pipeline

@vburckhardt
Copy link
Member Author

/run pipeline

argeiger
argeiger previously approved these changes Oct 24, 2023
Copy link
Contributor

@argeiger argeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -298,6 +299,19 @@ variable "load_balancers" {
)) == 0
}

validation {
error_message = "Load balancer idle_connection_timeout must be between 50 and 7200."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to mention that "idle_connection_timeout" is used only by application load balancer.

@rajatagarwal-ibm
Copy link
Member

rajatagarwal-ibm commented Oct 24, 2023

Also, the unit test failed because of the auth policy conflict. Probably need to delete existing policy manually before re-running the pipeline.

Separate to this task - we probably need to investigate ways to check the auth policy prior to creating it. This will avoid "auth policy conflict" issues. If you agree @vburckhardt I can create a task to look into this.

@ocofaigh
Copy link
Member

The upgrade test failed with this, which seems expected, so I suggest skipping it:

        	Messages:   	Resource(s) identified to be updated Name: listener Address: module.slz_vsi.ibm_is_lb_listener.listener["slz-vsi-com-upg-sjfifs-lb"] Actions: [update]
        	            	DIFF:
        	            	Before: {"idle_connection_timeout":50}
        	            	After: {"idle_connection_timeout":100}

@ocofaigh
Copy link
Member

@rajatagarwal-ibm The issue with clashing auth policies is a known issue. It happens because the block storage auth policy cannot be scoped to a specific resource group, and because we use a permanent HPCS instance, so it means such a policy may already exist in our account from a parallel test, or an old test that did not get cleaned up.
The only way to avoid it would be to stop running the fscloud example (which requires HPCS) and instead create an example that uses a Key Protect instance that we create as part of the tests so the auth policy would always be unique for the new instance

@vburckhardt
Copy link
Member Author

/run pipeline

tests/pr_test.go Outdated
ResourceGroup: resourceGroup,
Region: region,
TerraformVars: map[string]interface{}{
"skip_iam_authorization_policy": true, // The test account already has got a s2s policy setup that would clash
Copy link
Member

@ocofaigh ocofaigh Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time this auth policy will not exist actually, and this test will fail. The nuke nukes all global auth policies. We could add a feature to the nuke maybe to pass an ignore list of auth policies? Since policies don't have a name, we can't use the geretain prefix to tell nuke to ignore them - we would have to add a new way to ignore policies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think this is a good feature to have with the nuke, so I have created an internal issue for it

Copy link
Member Author

@vburckhardt vburckhardt Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to really review our testing approach here, I am spending 10x the time to get that pipeline to pass on unrelated aspects, than actually coding the change. We have a problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll comment it out, I think we need to invest in fresh account created for tests that touch account level resources like s2s policies. Once we have an enterprise account, we can create subaccounts for it for each new test run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we have the enterprise account already, so we should deep dive on next steps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's prioritize this in next sprint.

@vburckhardt
Copy link
Member Author

/run pipeline

@vburckhardt vburckhardt merged commit 7e6c34d into main Oct 25, 2023
2 checks passed
@vburckhardt vburckhardt deleted the idle-timeout-lb branch October 25, 2023 11:58
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants