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.

Sign up for GitHub

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

Jump to bottom

Auto-detect Azure cloud name via IMDS #16515

Merged
merged 2 commits into from
Jul 13, 2021
Merged

Auto-detect Azure cloud name via IMDS #16515

merged 2 commits into from
Jul 13, 2021

Conversation

ungureanuvladvictor
Copy link
Member

Via the instance metadata service we can query in which Azure cloud the
operator is running.

Signed-off-by: Vlad Ungureanu vladu@palantir.com

Auto-detect Azure cloud name via IMDS

@ungureanuvladvictor ungureanuvladvictor added area/operator Impacts the cilium-operator component area/azure Impacts Azure based IPAM. labels Jun 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 12, 2021
@ungureanuvladvictor ungureanuvladvictor added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 12, 2021
@ungureanuvladvictor
Copy link
Member Author

@ti-mo -- we chatted about this in #16043 (comment) + #16043 (comment).

I'd be curious how to execute the flag migration. At the moment there are the following cases:

  • users who do not pass azure-cloud-name so this will be transparent for them
  • users who use azure-cloud-name and pass a custom cloud and for those the new cilium build will break because I am removing the flag altogether. This is a bit weird because given Use of Azure IPAM in non-default Azure clouds #16003 I don't think they can run in a separate cloud (unless running the latest Cilium 1.10.x [the PR was backported]).

We could mark it as deprecated but given it has a default I don't know of a way to check in code if it was passed or not. If it would be a way to check that this was not passed then we could invoke the new codepath. Interested to hear your opinions on this. @aanm -- I know we talked in the past about flag migrations, if you have any thoughts they are welcomed!

@ungureanuvladvictor ungureanuvladvictor force-pushed the vladu/auto-detect-azure-cloud branch 2 times, most recently from 4149473 to 0ac5dd6 Compare June 12, 2021 06:40
@ungureanuvladvictor
Copy link
Member Author

test-me-please

@ti-mo ti-mo requested review from a team and errordeveloper and removed request for a team June 14, 2021 12:59
@ti-mo ti-mo requested review from a team and ti-mo and removed request for errordeveloper and a team June 14, 2021 13:05
@ti-mo
Copy link
Contributor

ti-mo commented Jun 14, 2021

Thanks for the patch! Unfortunately, I don't have an answer to the flag migration. Since this only recently started working, and it's not configurable through Helm (yet?), I don't think this would be used in the wild.

Personally, I wouldn't worry too much about breaking this, but I don't know what our usual policy is. I would guess we can't ever break backports, so this might need to go into 1.11.

@ungureanuvladvictor ungureanuvladvictor marked this pull request as ready for review June 14, 2021 15:54
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner June 14, 2021 15:54
@ungureanuvladvictor ungureanuvladvictor requested a review from a team June 14, 2021 15:54
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner June 14, 2021 15:54
@ungureanuvladvictor
Copy link
Member Author

@ti-mo -- in this case I marked the PR as ready for review.

errordeveloper
errordeveloper reviewed Jun 21, 2021
View reviewed changes
pkg/ipam/allocator/azure/azure.go Outdated
@@ -48,6 +48,11 @@ func (*AllocatorAzure) Start(getterUpdater ipam.CiliumNodeGetterUpdater) (alloca

log.Info("Starting Azure IP allocator...")

azureCloudName, err := azureAPI.GetAzureCloudName(context.TODO())
Copy link
Contributor

@errordeveloper errordeveloper Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can see there is at least one other instance of context.TODO() in this function, do we have a plan to address these todos?

Copy link
Member Author

Choose a reason for hiding this comment

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

@errordeveloper -- I changed the signature of the Allocator interface to pass a context to the Start.

@pchaigno
Copy link
Member

Marking as draft while reviews are addressed to remove from others' review queue.

@pchaigno pchaigno marked this pull request as draft June 29, 2021 13:05
@twpayne
Copy link
Contributor

twpayne commented Jun 29, 2021

@ungureanuvladvictor would you be able to dust off this PR?

@ungureanuvladvictor
Copy link
Member Author

hey folks -- sorry for the silence here, work schedule got busy. I'll get to it by EoW.

@ungureanuvladvictor
Add context.Context as param to the Start method of the AllocatorProv…
26b5dec 
…ider

Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Auto-detect Azure cloud name via IMDS
8a1ac80 
Via the instance metadata service we can query in which Azure cloud the
operator is running.

Marked the "azure-cloud-name" flag as deprecated and to be removed in
1.11.

Signed-off-by: Vlad Ungureanu <vladu@palantir.com>
@ungureanuvladvictor
Copy link
Member Author

test-me-please

@ungureanuvladvictor
Copy link
Member Author

ci-aks

@ungureanuvladvictor ungureanuvladvictor marked this pull request as ready for review July 3, 2021 16:26
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner July 3, 2021 16:26
@ungureanuvladvictor
Copy link
Member Author

ci-eks

@ungureanuvladvictor
Copy link
Member Author

ci-gke

@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented Jul 3, 2021 •
edited
Loading

test-1.21-4.9 (https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.9/965/testReport/junit/Suite-k8s-1/21/K8sPolicyTest_Basic_Test_Traffic_redirections_to_proxy_Tests_proxy_visibility_interactions_with_policy_lifecycle_operations/)

@ungureanuvladvictor
Copy link
Member Author

test-1.21-4.9

ti-mo
ti-mo approved these changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks Vlad! Left 2 small nits.

pkg/ipam/allocator/azure/azure.go
@@ -19,6 +19,7 @@ import (
"fmt"

operatorMetrics "github.com/cilium/cilium/operator/metrics"
"github.com/cilium/cilium/operator/option"
Copy link
Contributor

Choose a reason for hiding this comment

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

This package is already imported on the subsequent line.

pkg/ipam/allocator/azure/azure.go
@@ -48,10 +50,22 @@ func (*AllocatorAzure) Start(getterUpdater ipam.CiliumNodeGetterUpdater) (alloca

log.Info("Starting Azure IP allocator...")

var azureCloudName string
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the following might be nicer:

azureCloudName := operatorOption.Config.AzureCloudName
if !viper.IsSet(operatorOption.AzureCloudName) {
    // Call IMDS...
}

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 12, 2021
@kkourt kkourt merged commit b29389d into cilium:master Jul 13, 2021
@ungureanuvladvictor ungureanuvladvictor mentioned this pull request Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@errordeveloper errordeveloper errordeveloper left review comments

@twpayne twpayne twpayne approved these changes

@tgraf tgraf tgraf approved these changes

@ti-mo ti-mo ti-mo approved these changes

@aanm aanm aanm approved these changes

Assignees

@errordeveloper errordeveloper

Labels
area/azure Impacts Azure based IPAM. area/operator Impacts the cilium-operator component ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ungureanuvladvictor @ti-mo @pchaigno @twpayne @errordeveloper @tgraf @aanm @kkourt