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

azurerm_kubernetes_cluster - support for the kube_proxy property #19567

Merged
merged 11 commits into from
Jan 15, 2023

Conversation

ms-henglu
Copy link
Contributor

=== RUN   TestAccKubernetesCluster_kubeProxy
=== PAUSE TestAccKubernetesCluster_kubeProxy
=== CONT  TestAccKubernetesCluster_kubeProxy
--- PASS: TestAccKubernetesCluster_kubeProxy (809.84s)
PASS

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @ms-henglu, i've left several questions and comments in-line, once those are resolved we can take another look through.

Comment on lines 3453 to 3457
if input[0] == nil {
return &managedclusters.ContainerServiceNetworkProfileKubeProxyConfig{
Enabled: utils.Bool(true),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we enabling it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this if block and made the mode a required field.

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@NissesSenap
Copy link

Great to see that this feature is coming.
I might misunderstand the code but It feels like a feature is missing.

In https://learn.microsoft.com/en-us/azure/aks/configure-kube-proxy it's an option to disable kube-proxy all together.

{
  "enabled": true,
  "mode": "IPVS",
  "ipvsConfig": {
    "scheduler": "LeastConnection",
    "TCPTimeoutSeconds": 900,
    "TCPFINTimeoutSeconds": 120,
    "UDPTimeoutSeconds": 300
  }
}

This is something that needs to be supported, especially together with BYOCNI mode.

@stephybun
Copy link
Member

@NissesSenap, the omission of the block in the config should disable kube_proxy for the cluster. We have a section in the contributor docs explaining how to map features that are toggled with an Enabled field and how blocks should behave in the provider. Hopefully that helps in clarifying any confusion!

@NissesSenap
Copy link

Thanks for the quick reply @stephybun.
From reading the docs my understanding is that it should contain optional_feature_enabled, for example open_service_mesh_enabled.

This feature is also optional, so I think we are missing the kube_proxy_enabled from this PR.
But I could of course misunderstand it.

@stephybun
Copy link
Member

stephybun commented Dec 8, 2022

@NissesSenap, if we were flattening the kube_proxy block, i.e. adding all the kube proxy fields as top level properties then yes your understanding is correct in that we would need to have a property like kube_proxy_enabled to toggle this feature on and off.

Since we're keeping it as a block, enabling it is done in the usual fashion by adding it into the config, i.e.

resource "azurerm_kubernetes_cluster" "example" {
  name                = "myakscluster"

...

  network_profile {
    network_plugin = "azure"
    kube_proxy {
      mode = "IPVS"
      ipvs_config {
        scheduler                  = "LeastConnection"
        tcp_fin_timeout_in_seconds = 1000
        tcp_timeout_in_seconds     = 1000
        udp_timeout_in_seconds     = 1000
      }
    }
  }

...

}

Disabling it is done purely by removing the block from the resource config.

resource "azurerm_kubernetes_cluster" "example" {
  name                = "myakscluster"

...

  network_profile {
    network_plugin = "azure"
  }

...

}

If you're familiar with or have used some of the AKS add-ons they behave in the same way e.g. oms_agent or ingress_application_gateway.

Also very happy for any feedback on the docs that would help better clarify this pattern/behaviour within the provider.

@NissesSenap
Copy link

@stephybun once again I might be wrong but I'm rather sure that what you describe would be a breaking change.

The kube-proxy config supports three big configuration areas according to my understanding of https://learn.microsoft.com/en-us/azure/aks/configure-kube-proxy.

Configure kube-proxy to use IPVS

A new feature in AKS that makes it possible to use IPVS in kube-proxy

{
  "enabled": true,
  "mode": "IPVS",
  "ipvsConfig": {
    "scheduler": "LeastConnection",
    "TCPTimeoutSeconds": 900,
    "TCPFINTimeoutSeconds": 120,
    "UDPTimeoutSeconds": 300
  }
}

Configure kube-proxy to use IPAM

This is the default setting and currently used by all AKS clusters (don't know the exact config but something like this).

{
  "enabled": true,
  "mode": "IPAM",
  "ipvsConfig": {
    "scheduler": "LeastConnection",
    "TCPTimeoutSeconds": 900,
    "TCPFINTimeoutSeconds": 120,
    "UDPTimeoutSeconds": 300
  }
}

Disable kube-proxy

If you want to disable kube-proxy from running in your cluster all together.

{
  "enabled": false,
}

What you describe in your last comment

resource "azurerm_kubernetes_cluster" "example" {
  name                = "myakscluster"

...

  network_profile {
    network_plugin = "azure"
  }

...

}

Would disable kube-proxy on all clusters that have defined the network_profile.

This is not what you want.

Real example

This is my current setting in my cluster when using BYOCNI in AKS.
According to your example this config would be kube_proxy = disabled on the cluster.

resource "azurerm_kubernetes_cluster" "example" {
  name                = "myakscluster"

...
  network_profile {
    network_plugin    = "none"
    load_balancer_sku = "standard"
    load_balancer_profile {
      outbound_ip_prefix_ids = [
        var.aks_public_ip_prefix_id
      ]
    }
  }
...

Summary

All I want to do is to disable kube-proxy from running on my cluster all together.
If I still have misunderstood you I'm sorry about wasting your time and all I can say is that I'm looking forward to the new feature.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

@ms-henglu could you please resolve the merge conflict? This should be good to go then.

@ms-henglu
Copy link
Contributor Author

Hi @stephybun , Thanks! I've resolved the conflicts.

@stephybun
Copy link
Member

@NissesSenap, apologies for the late response and thank you for outlining your concerns and current scenario.

After looking through the PR again and making further changes I believe this should no longer be a breaking change.

The API returns kube proxy information in the response only once it's been modified. With the current implementation the provider would only send a value for kube_proxy once it's been modified. This means the current kube proxy settings for the cluster (whether it's enabled or disabled) should be respected and Terraform shouldn't flag a diff.

I hope this resolves any confusion about this feature. If there are any issues please do let us know and thanks again for your input!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @ms-henglu, LGTM 🦘

@stephybun stephybun merged commit f212e5b into hashicorp:main Jan 15, 2023
@github-actions github-actions bot added this to the v3.40.0 milestone Jan 15, 2023
stephybun added a commit that referenced this pull request Jan 15, 2023
@stephybun stephybun mentioned this pull request Jan 16, 2023
1 task
@EppO
Copy link
Contributor

EppO commented Jan 16, 2023

The API returns kube proxy information in the response only once it's been modified. With the current implementation the provider would only send a value for kube_proxy once it's been modified. This means the current kube proxy settings for the cluster (whether it's enabled or disabled) should be respected and Terraform shouldn't flag a diff.

How do you explicitly disable it then? It looks like to me that keeping the default kube-proxy config or disabling it altogether are both using the absence of the block, am I wrong?

@mkilchhofer
Copy link
Contributor

Yes, I was also thinking about this. Does the absence of the whole block results in a cluster with missing kube-proxy?

@stephybun
Copy link
Member

Thank you all for your input and questions - it's much appreciated and you've all raised very valid points that I unfortunately overlooked in my reviews.

As it stands I do not think we can support this feature in its current state due to the behaviour of the API and the fact that Kube Proxy is a functionality that is enabled on clusters by default. I've raised an issue over on Azure/azure-rest-api-specs#22208 detailing the behaviour and hope the AKS team will take a look and can resolve them.

Presently there is no straight forward way to expose this feature without shipping either a breaking change, or a roundabout way of explicitly disabling Kube Proxy.

To answer your question @EppO @mkilchhofer, the only way to explicitly disable Kube Proxy (with the way that it has been shipped) would be to add the block, apply, and then to remove the block. This is because the API does not supply the kube proxy configuration in the response unless it's been modified, even once the preview feature for it has been enabled. I concede that this is less than ideal.

The initial implementation where we always send "enabled": false when the block is absent is problematic since it is a breaking change as @NissesSenap pointed out, and will also result in an error from the API if the preview feature for it isn't enabled on the subscription.

As a result I'm going to revert this PR for the moment. Apologies for jumping the gun on this one.

@EppO
Copy link
Contributor

EppO commented Jan 17, 2023

Glad we helped you sort this one out.
Can't we have a default block config passed to the API when user doesn't provide one instead of sending "enabled": false?

EDIT: scratch that, I just read Azure/azure-rest-api-specs#22208 and I get it's an API issue now.

@mkilchhofer
Copy link
Contributor

mkilchhofer commented Jan 18, 2023

👍
So I think we should also reopen the enhancement request over here: #19300

@github-actions
Copy link

This functionality has been released in v3.40.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants