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

Add dataplane-v2 provisioning support #753

Conversation

ryan-atkins
Copy link
Contributor

Fixes #656

The datapath_provider was released in terraform-provider-google-beta v3.41.0

  • adds datapath_provider var for beta clusters, defaults to "DATAPATH_PROVIDER_UNSPECIFIED"
  • as-is, would need to set network_policy = false whenever desiring to use the "ADVANCED_DATAPATH" provider due to module currently setting CALICO as the default network_policy_provider.

Could use suggestions on how best to handle introducing this and better working with the network_policy config.

@comment-bot-dev
Copy link

comment-bot-dev commented Dec 2, 2020

Thanks for the PR! 🚀
✅ Lint checks have passed.

Copy link
Member

@bharathkkb bharathkkb 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 the PR @ryan-atkins

as-is, would need to set network_policy = false whenever desiring to use the "ADVANCED_DATAPATH" provider due to module currently setting CALICO as the default network_policy_provider

It makes sense to automatically set that for the user if they are explicitly enabling dataplanev2. We should also document this in the variable description. For implementation we would need to check if ADVANCED_DATAPATH is set and use that to control the network_policy behavior in places below.

disabled = ! var.network_policy

cluster_network_policy = var.network_policy ? [{

examples/simple_regional_beta/main.tf Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 1, 2021

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Feb 1, 2021
@github-actions github-actions bot closed this Feb 9, 2021
@bharathkkb bharathkkb reopened this Feb 9, 2021
@bharathkkb bharathkkb removed the Stale label Feb 9, 2021
@xaocon
Copy link

xaocon commented Feb 18, 2021

@bharathkkb, are you working on this already? If not would you feel like maybe doing some mentoring around it? I'd like to see it implemented but I'm still pretty new with Terraform and don't know much about this module. It looks like it's pretty close already but I may need some guidance around network policy.

@bharathkkb
Copy link
Member

Hi @xaocon
Feel free to pick this up and happy to help support. I had two comments above, one was just hard coding the example. The other was to automatically set network_policy = false depending on if datapath_provider is ADVANCED_DATAPATH

@keskiju
Copy link

keskiju commented Feb 20, 2021

I'm not quite sure if network_policy should be automatically disabled if datapath_provider is set to ADVANCED_DATAPATH. I guess the following configuration is perfectly valid for the google_container_cluster:

  datapath_provider = "ADVANCED_DATAPATH"

  network_policy {
    enabled = true
    provider = "PROVIDER_UNSPECIFIED"
  }

I would assume that with these settings it would automatically choose CILIUM as the network policy provider. You can even leave the network_policy.provider unset since PROVIDER_UNSPECIFIED is the default value for it there. I wonder why this module uses CALICO as the default network_policy.provider instead of PROVIDER_UNSPECIFIED. There would be no problem if PROVIDER_UNSPECIFIED was the default.

@sharkymcdongles
Copy link

sharkymcdongles commented Feb 22, 2021

I'm not quite sure if network_policy should be automatically disabled if datapath_provider is set to ADVANCED_DATAPATH. I guess the following configuration is perfectly valid for the google_container_cluster:

  datapath_provider = "ADVANCED_DATAPATH"

  network_policy {
    enabled = true
    provider = "PROVIDER_UNSPECIFIED"
  }

I would assume that with these settings it would automatically choose CILIUM as the network policy provider. You can even leave the network_policy.provider unset since PROVIDER_UNSPECIFIED is the default value for it there. I wonder why this module uses CALICO as the default network_policy.provider instead of PROVIDER_UNSPECIFIED. There would be no problem if PROVIDER_UNSPECIFIED was the default.

I think you might be incorrect as when I try to deploy a cluster using ADVANCED_DATAPATH and network policy true I get:

Error: googleapi: Error 400: Enabling NetworkPolicy for clusters with DatapathProvider=ADVANCED_DATAPATH is not allowed., badRequest

Also in the documentation here: https://cloud.google.com/kubernetes-engine/docs/how-to/dataplane-v2?hl=it

Warning: You cannot enable the NetworkPolicy feature in the same request that creates a cluster.

So perhaps it is possible to enable it after the fact given the warning here, but it seems it cannot be done on first deploy.

@ryan-atkins
Copy link
Contributor Author

Sorry ignored this for awhile!
With #809 being merged, I believe that resolves needing to touch the network_policy here as the default now is false.
Updated the example and reran integration tests 👍

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks @ryan-atkins

@bharathkkb bharathkkb merged commit d1fbef4 into terraform-google-modules:master Mar 5, 2021
@gw0
Copy link

gw0 commented Jul 14, 2021

Why is this so complicated? Wouldn't it make much more sense to extend network_policy.provider by adding dataplane-v2 (or cilium)?

@morgante
Copy link
Contributor

@gw0 This was the simplest non-breaking fix, but we could probably re-evaluate interaction with network_policy_provider.

CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
…#753)

* add datapath_provider var for beta clusters

* incorporate datapath_provider option

* update tests for datapath_provider beta cluster

* example README var updates

* remove network_policy var usage

* finish docs updates

Co-authored-by: ryan-atkins <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GKE Dataplane v2 provisioning support
8 participants