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 public ips with service tags for LBs during cluster creation #2821

Closed
wants to merge 54 commits into from

Conversation

shubham-pathak-03
Copy link

@shubham-pathak-03 shubham-pathak-03 commented Jul 1, 2024

Reason for Change:

As per the new security requirements, all traffic for our services must be under a service tag. Cluster creation under ACN repo creates load balancers and the associated ips use default service tags provided by Microsoft. This change creates new public ips and attaches it with LBs during cluster creation phase.

Requirements:

Notes:

hack/aks/Makefile Outdated Show resolved Hide resolved
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Jul 27, 2024
Comment on lines 302 to 309
$(AZCLI) network public-ip create --name $(PUBLIC_IP) \
--resource-group $(GROUP) \
--allocation-method Static \
--ip-tags $(IP_TAG) \
--location $(REGION) \
--sku Standard \
--tier Regional \
--version IPv4
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of copy/pasting these 8 lines in to every cluster target, creating the public IP should be it's own target that can be reused like the set-kubeconf target

public-ip: rg-up
	$(AZCLI) network public-ip create --name $(PUBLIC_IP) \
		--resource-group $(GROUP) \
		--allocation-method Static \
		--ip-tags $(IP_TAG) \
		--location $(REGION) \
		--sku Standard \
		--tier Regional \
		--version IPv4

...
...

swiftv2-multitenancy-cluster-up: rg-up
	@$(MAKE) public-ip // <- calls the new public ip target
	$(AZCLI) aks create -n $(CLUSTER) -g $(GROUP) -l $(REGION) \
		--network-plugin azure \
		--network-plugin-mode overlay \
		--kubernetes-version 1.28 \
		--nodepool-name "mtapool" \
		--node-vm-size $(VM_SIZE) \
		--node-count 2 \
		--load-balancer-outbound-ips /subscriptions/$(SUB)/resourceGroups/$(GROUP)/providers/Microsoft.Network/publicIPAddresses/$(PUBLIC_IP) \
		--nodepool-tags fastpathenabled=true \
		--no-ssh-key \
		--yes
	@$(MAKE) set-kubeconf

...

Choose a reason for hiding this comment

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

updated the PR to reflect this

@github-actions github-actions bot removed the stale Stale due to inactivity. label Jul 31, 2024
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Aug 14, 2024
Copy link

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Aug 22, 2024
@github-actions github-actions bot deleted the spathak/add-service-tag branch August 22, 2024 00:01
@k-routhu
Copy link

opening PR

@k-routhu k-routhu reopened this Dec 13, 2024
@k-routhu
Copy link

location: westcentralus
allowed ip range: 172.208.176.64 - 172.208.176.95
ip address: 172.208.176.65
Cilium Nightly Pipeline run
image

@github-actions github-actions bot removed the stale Stale due to inactivity. label Dec 14, 2024
@k-routhu
Copy link

k-routhu commented Dec 14, 2024

General Notes:

  • We had a couple of PRs out by Esteban and Shubham to update service tags. However, all of the PRs targeted hack/aks/Makefile, so I went ahead and made all of the updates in this branch.
  • Also added ipv6 public ips to load balancer outbound ips for dualstack clusters to resolve errors

Testing:
Tested changes in a couple of Cilium pipelines:
CNI Nightly Pipeline - similar as above, but with all of the final changes
location: westcentralus
allowed ip range: 172.208.176.64 - 172.208.176.95
ip address: 172.208.176.65

CNI Release Test
The cilium-overlay test fails at the scale test job, but this happens on the master branch for the same exact reason, so I believe that this is safe to merge, considering the other pipelines that have been run
All of the public ips created have service tags attached to them, and the ips fall within the allowed ranges for the locations they were created

Cilium Private Test on v1.14 as suggested by John Payne

Cilium Private Test on v1.16 as suggested by John Payne

ACN PR Pipeline
The failing test also fails in master, so I believe this is safe to merge considering the other pipelines that have been run

@rbtr
Copy link
Contributor

rbtr commented Dec 14, 2024

@k-routhu This will not be merged since the author of the PR has not agreed to the CLA (and likely won't). You will need to fork off of this branch and open a new PR.

@rbtr rbtr closed this Dec 14, 2024
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.

5 participants