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

NCC in 2-net-a-simple #2397

Merged
merged 22 commits into from
Jul 25, 2024
Merged

NCC in 2-net-a-simple #2397

merged 22 commits into from
Jul 25, 2024

Conversation

sruffilli
Copy link
Collaborator

@sruffilli sruffilli commented Jun 29, 2024

  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass
  • Updated README.md to reflect the changes

This is a first, basic implementation of NCC in 2-simple.

Caveats:

  1. Landing is not part of the core network anymore, but it simply hosts hybrid connectivity, which itself is connected as a spoke
  2. Because of (1), do we need a "shared services" VPC?
  3. VPC spokes must be authorized manually (i.e. from the Cloud Console). At this stage the terraform provider (nor the API?) does not allow for ncc hub projects allowlisting.

Copy link
Member

@LucaPrete LucaPrete left a comment

Choose a reason for hiding this comment

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

Minor comments.
For ncc we shouldn't need to be whitelisted, given it just went public preview. Anyway, the provider is not there yet.
Also, readme should be updated before merge.

fast/stages/2-networking-a-simple/main.tf Show resolved Hide resolved
fast/stages/2-networking-a-simple/spoke-ncc.tf Outdated Show resolved Hide resolved
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

really linear and simple to read :)

@sruffilli
Copy link
Collaborator Author

Minor comments. For ncc we shouldn't need to be whitelisted, given it just went public preview. Anyway, the provider is not there yet.

This PR is tested and it works :)
The provider didn't really require any change, as the new feature allows for the NCC-Hub to have BOTH VPC spokes and Hybrid connectivity spokes.

Also, readme should be updated before merge.

This is why the PR is in draft. I'm updating the README as/if we reach consensus this is a valid PR.

Copy link
Member

@LucaPrete LucaPrete left a comment

Choose a reason for hiding this comment

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

lgtm now. I guess we can finalize this updating tests and readme and then merge.

@LucaPrete LucaPrete requested review from ludoo and LucaPrete July 6, 2024 07:21
@sruffilli
Copy link
Collaborator Author

Given there seems to be consensus, I'll go ahead and write docs/tests.

@sruffilli sruffilli marked this pull request as ready for review July 24, 2024 13:52
@sruffilli sruffilli enabled auto-merge (squash) July 24, 2024 15:25
@sruffilli sruffilli disabled auto-merge July 24, 2024 15:25
@sruffilli sruffilli merged commit 27bb48d into master Jul 25, 2024
17 checks passed
@sruffilli sruffilli deleted the sruffilli/ncc-simple branch July 25, 2024 16:03
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.

4 participants