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(http_proxy_config): Add http_proxy_config #435

Closed

Conversation

isantospardo
Copy link
Contributor

Describe your changes

Adds a HTTP(S) proxy configuration within this module like described in the upstream Terraform Kubernetes Cluster resource.

This feature is currently not implemented yet and it would let us configure a HTTP(s) proxy for our AKS nodes with a customer CA as well.

You can have a look on a fork which enabled it, please see here.

We are working on a PR already and it will be linked to this issue asap.

Thanks for your understanding and time.

Issue number

#434

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Fixes: #434

@vmaillot vmaillot mentioned this pull request Aug 29, 2023
1 task
@zioproto
Copy link
Collaborator

@zioproto
Copy link
Collaborator

@isantospardo please run pre-commit and pr-check steps like described in the README file.

https://github.com/Azure/terraform-azurerm-aks#pre-commit--pr-check--test

@isantospardo
Copy link
Contributor Author

@isantospardo please run pre-commit and pr-check steps like described in the README file.

https://github.com/Azure/terraform-azurerm-aks#pre-commit--pr-check--test

Done, thanks for the quick note :)

@isantospardo
Copy link
Contributor Author

@microsoft-github-policy-service agree

@isantospardo
Copy link
Contributor Author

@jiaweitao001 could you have a look at this PR?

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @isantospardo for opening this pr, some review comments, and I'm afraid that we might need to defer this pr to our next major version, but I think it could be merged and released in one or two months.

main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
@isantospardo
Copy link
Contributor Author

Thanks @isantospardo for opening this pr, some review comments, and I'm afraid that we might need to defer this pr to our next major version, but I think it could be merged and released in one or two months.

Thanks a lot for the comments, I just updated the PR, and thanks for the time estimation.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @isantospardo for the update, only one comment.

Btw, I've merged a pr to update the readme file, so your pr contains a conflict with readme file now, would you please update your pr by rebase from the latest main branch and try again? Thanks a lot!

main.tf Outdated Show resolved Hide resolved
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @isantospardo for your update, we have a conflict on the readme file, would you please rebase your branch with the latest main branch and try again? Thanks!

Related to: Azure#434

Signed-off-by: Iago Santos Pardo <[email protected]>
Signed-off-by: Iago Santos Pardo <[email protected]>
Signed-off-by: Iago Santos Pardo <[email protected]>
Signed-off-by: Iago Santos Pardo <[email protected]>
@isantospardo
Copy link
Contributor Author

Thanks @isantospardo for your update, we have a conflict on the readme file, would you please rebase your branch with the latest main branch and try again? Thanks!

Done!

@lonegunmanb
Copy link
Member

Thanks @isantospardo for the update, almost LGTM but this pr should be considered as a breaking change, so I'd like to defer this pr to our next major version upgrade (I hope we could make it happened in October).

@lonegunmanb lonegunmanb added this to the 8.0.0 milestone Sep 14, 2023
default = null
description = <<-EOT
optional(object({
http_proxy = (Optional) The proxy address to be used when communicating over HTTP. Changing this forces a new resource to be created.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Changing this forces a new resource to be created." is not true anymore.

References:
Azure/AKS#3524
MicrosoftDocs/azure-docs@0bb10c7

This will probably change soon upstream in the provider. Hold before merging.

@lonegunmanb
Copy link
Member

I'm closing this pr since I've opened a new one #492 , I've continued @isantospardo's work there. Thanks everyone.

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

Successfully merging this pull request may close these issues.

Support for http proxy settings
3 participants