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(TF>=1.1)!: Configure ASM management mode #1702

Merged
merged 18 commits into from Nov 10, 2023
Merged

feat(TF>=1.1)!: Configure ASM management mode #1702

merged 18 commits into from Nov 10, 2023

Conversation

ferrarimarco
Copy link
Contributor

This PR implements the ability of configuring ASM management mode.

Without this, users have to manually configure the feature membership.

@ferrarimarco ferrarimarco requested review from Jberlinsky, ericyz and a team as code owners August 4, 2023 11:22
@ferrarimarco ferrarimarco changed the title Configure ASM management mode feat: Configure ASM management mode Aug 4, 2023
@ferrarimarco
Copy link
Contributor Author

Ah! But we configure the CPR already, so this may not actually be needed...

@ericyz
Copy link
Collaborator

ericyz commented Aug 15, 2023

/gcbrun

@ferrarimarco
Copy link
Contributor Author

Also, it looks like that the module.cpr and kubernetes_config_map.asm_options aren't necessary anymore because the fleet API takes care of that, and actually conflict with the Managed ASM control plane installation. Worth removing in this PR, or a separate one?

@ferrarimarco
Copy link
Contributor Author

PS: the build failure doesn't seem to be related to this PR.

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Thanks for the pull request @ferrarimarco

From the linter

Checking submodule's files generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=.git' /workspace/modules/asm/README.md /tmp/tmp.eM1y9hxVzy/workspace/modules/asm/README.md
62c62
< | enable\_vpc\_sc | Determines whether to enable VPC-SC for this ASM installation. For more information read <https://cloud.google.com/service-mesh/docs/managed/vpc-sc> | `bool` | `false` | no |
---
> | enable\_vpc\_sc | Determines whether to enable VPC-SC for this ASM installation. For more information read https://cloud.google.com/service-mesh/docs/managed/vpc-sc | `bool` | `false` | no |
65c65
< | mesh\_management | ASM management mode. For more information, see the [gke_hub_feature_membership resource documentation](https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/gke_hub_feature_membership#nested_mesh) | `string` | `""` | no |
---
> | mesh\_management | ASM Management mode. | `string` | `""` | no |
Error: submodule's files generation has not been run, please run the
'make build' command and commit changes

Checking for documentation generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=autogen' '--exclude=*.tfvars' '--exclude=*metadata.yaml' /workspace/modules/asm/README.md /tmp/tmp.3rgofwWMtd/generate_docs/workspace/modules/asm/README.md
62c62
< | enable\_vpc\_sc | Determines whether to enable VPC-SC for this ASM installation. For more information read <https://cloud.google.com/service-mesh/docs/managed/vpc-sc> | `bool` | `false` | no |
---
> | enable\_vpc\_sc | Determines whether to enable VPC-SC for this ASM installation. For more information read https://cloud.google.com/service-mesh/docs/managed/vpc-sc | `bool` | `false` | no |
65c65
< | mesh\_management | ASM management mode. For more information, see the [gke_hub_feature_membership resource documentation](https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/gke_hub_feature_membership#nested_mesh) | `string` | `""` | no |
---
> | mesh\_management | ASM Management mode. | `string` | `""` | no |
Error: Documentation generation has not been run, please run the
'make docker_generate_docs' command and commit the above changes.

@ferrarimarco
Copy link
Contributor Author

@apeabody thanks for the update. I'll look into that.

@ferrarimarco
Copy link
Contributor Author

@apeabody hopefully fixed!

@apeabody
Copy link
Contributor

@apeabody hopefully fixed!

Thanks @ferrarimarco - With the addition of count to the module, you will also need to add the index for the output value:

│ Error: Unsupported attribute
│ 
│   on ../../../modules/asm/outputs.tf line 23, in output "wait":
│   23:   value       = module.cpr.wait
│     ├────────────────
│     │ module.cpr is a list of object
│ 
│ Can't access attributes on a list of objects. Did you mean to access
│ attribute "wait" for a specific element of the list, or across all elements
│ of the list?

@ferrarimarco
Copy link
Contributor Author

@apeabody thanks for your review.

I've modified the wait output variable initialization to fix the issue reported in #1702 (comment).

Also, the module now accordingly sets its value considering both MANAGEMENT_AUTOMATIC and MANAGEMENT_MANUAL (or unset management mode).

@apeabody
Copy link
Contributor

apeabody commented Oct 3, 2023

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Oct 4, 2023

/gcbrun

@apeabody apeabody changed the title feat: Configure ASM management mode feat(TF>=1.1)!: Configure ASM management mode Oct 4, 2023
@ferrarimarco
Copy link
Contributor Author

Build failures seem related to quota issues.

@apeabody
Copy link
Contributor

apeabody commented Oct 6, 2023

/gcbrun

@ferrarimarco
Copy link
Contributor Author

ferrarimarco commented Oct 9, 2023

This failure seems to be related to an unsupported GKE version. I don't think it's related to this PR :)

@apeabody
Copy link
Contributor

apeabody commented Oct 9, 2023

Also, it looks like that the module.cpr and kubernetes_config_map.asm_options aren't necessary anymore because the fleet API takes care of that, and actually conflict with the Managed ASM control plane installation. Worth removing in this PR, or a separate one?

Hi @ferrarimarco - Is there a recommend migration path? While reviewing this PR its clear that supporting both adds extra complexity. Ideally we would want to minimze wrapping imperative commands like in module.cpr.

@ferrarimarco
Copy link
Contributor Author

ferrarimarco commented Oct 10, 2023

Currently, Anthos Service Mesh supports the following installation methods:

In all cases, there's no explicit creation of ControlPlaneRevision resources. Either the Fleet API does it, or asmcli takes care of creating it.

My opinion is that we should:

  • Remove the module.cpr resource because the Fleet API and asmcli take care of creating it.
  • Remove the kubernetes_namespace.system resource because the Fleet API and asmcli take care of creating it.
  • Clarify that we install the "managed ASM control plane" version using the Fleet API.
  • Refactor optional features according to Enable optional features on managed ASM. This probably requires dropping unsupported features, such as var.enable_cni. See the update.

Installation methods that involve using asmcli will likely result in having to wrap calls to asmcli using null_resources, which are hard to maintain and to implement with the right triggers. That's why I suggest sticking with provisioning and configuring managed ASM using the Fleet API only.

I'm happy to talk about this over Meet if you prefer :)

/cc @jbrook

UPDATE: I think we should also drop the dependency from the Kubernetes provider because we wouldn't need to connect to individual clusters in the fleet and avoid configuring optional features at all Enable optional features on managed ASM. As pointed out by folks I aligned with, it may be unlikely that we want users to manage individual objects inside clusters with Terraform. We might want to defer optional features configuration to a later PR, provided that we see demand.

UPDATE 2: By dropping the dependency from the Kubernetes provider, we'd need to find an alternative way to check if the ControlPlaneRevision is there, which we use as a proxy to check if ASM mutating web hooks are in place.

@apeabody
Copy link
Contributor

Currently, Anthos Service Mesh supports the following installation methods:

In all cases, there's no explicit creation of ControlPlaneRevision resources. Either the Fleet API does it, or asmcli takes care of creating it.

My opinion is that we should:

  • Remove the module.cpr resource because the Fleet API and asmcli take care of creating it.
  • Remove the kubernetes_namespace.system resource because the Fleet API and asmcli take care of creating it.
  • Clarify that we install the "managed ASM control plane" version using the Fleet API.
  • Refactor optional features according to Enable optional features on managed ASM. This probably requires dropping unsupported features, such as var.enable_cni.

Installation methods that involve using asmcli will likely result in having to wrap calls to asmcli using null_resources, which are hard to maintain and to implement with the right triggers. That's why I suggest sticking with provisioning and configuring managed ASM using the Fleet API only.

I'm happy to talk about this over Meet if you prefer :)

/cc @jbrook

Thanks @ferrarimarco - Given the scale of the change, let me ping you directly.

@apeabody apeabody requested review from g-awmalik and removed request for Jberlinsky October 18, 2023 16:37
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Currently, Anthos Service Mesh supports the following installation methods:

In all cases, there's no explicit creation of ControlPlaneRevision resources. Either the Fleet API does it, or asmcli takes care of creating it.
My opinion is that we should:

  • Remove the module.cpr resource because the Fleet API and asmcli take care of creating it.
  • Remove the kubernetes_namespace.system resource because the Fleet API and asmcli take care of creating it.
  • Clarify that we install the "managed ASM control plane" version using the Fleet API.
  • Refactor optional features according to Enable optional features on managed ASM. This probably requires dropping unsupported features, such as var.enable_cni.

Installation methods that involve using asmcli will likely result in having to wrap calls to asmcli using null_resources, which are hard to maintain and to implement with the right triggers. That's why I suggest sticking with provisioning and configuring managed ASM using the Fleet API only.
I'm happy to talk about this over Meet if you prefer :)
/cc @jbrook

Thanks @ferrarimarco - Given the scale of the change, let me ping you directly.

Hi @ferrarimarco - Did we want to move forward with this change for now? Cheers!

@apeabody
Copy link
Contributor

/gcbrun

@ferrarimarco
Copy link
Contributor Author

ferrarimarco commented Oct 30, 2023

Hi Andrew! I'll align internally and get back to you ASAP. Thanks for following up!

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks @ferrarimarco!

modules/asm/hub.tf Show resolved Hide resolved
modules/asm/main.tf Show resolved Hide resolved
modules/asm/main.tf Show resolved Hide resolved
@apeabody apeabody merged commit a9de2d7 into terraform-google-modules:master Nov 10, 2023
4 checks passed
@ferrarimarco ferrarimarco deleted the mesh-feature-configuration branch November 13, 2023 09:44
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.

None yet

4 participants