async publicips

[CecileRobertMichon]

No description provided.

[karuppiah7890]

I'm assuming no one's working on this, and also assuming that #1721 work is mostly over with maybe some discussion and final touches pending

I'll try to give this one a shot!

/assign

[karuppiah7890]

Question: For getting tags of Public IP to check if Public IP is managed or not over here

cluster-api-provider-azure/azure/services/publicips/publicips.go

Lines 141 to 150 in e532729

	// isIPManaged returns true if the IP has an owned tag with the cluster name as value,
	// meaning that the IP's lifecycle is managed.
	func (s *Service) isIPManaged(ctx context.Context, ipName string) (bool, error) {
	ip, err := s.Client.Get(ctx, s.Scope.ResourceGroup(), ipName)
	if err != nil {
	return false, err
	}
	tags := converters.MapToTags(ip.Tags)
	return tags.HasOwned(s.Scope.ClusterName()), nil
	}

Can we replace the GET Public IP API call with GET Tags At Scope API call with Public IP as scope? I think that would make the GET API call more light weight. What do you think @CecileRobertMichon ?

[karuppiah7890]

Regarding Delete, looks like the DELETE Public IP API Call never returns 404 Not Found according to https://docs.microsoft.com/en-us/rest/api/virtualnetwork/public-ip-addresses/delete#response . It may return some cloud errors though. If that's true then I guess in the below code, the if condition on error scenario is never executed

cluster-api-provider-azure/azure/services/publicips/publicips.go

Lines 127 to 131 in e532729

	err = s.Client.Delete(ctx, s.Scope.ResourceGroup(), ip.Name)
	if err != nil && azure.ResourceNotFound(err) {
	// already deleted
	continue
	}

As azure.ResourceNotFound(err) will always be false according to the API docs

[CecileRobertMichon]

@karuppiah7890 I don't think the API docs are accurate (or at least not descriptive enough), a delete can definitely return a resource not found 404 error. Here's an example:

urllib3.connectionpool: Starting new HTTPS connection (1): management.azure.com:443
urllib3.connectionpool: https://management.azure.com:443 "DELETE /subscriptions/redacted/resourceGroups/group/providers/Microsoft.Network/virtualNetworks/foo?api-version=2021-02-01 HTTP/1.1" 404 97
cli.azure.cli.core.sdk.policies: Response status: 404

[CecileRobertMichon]

Can we replace the GET Public IP API call with GET Tags At Scope API call with Public IP as scope? I think that would make the GET API call more light weight.

+1

[karuppiah7890]

@karuppiah7890 I don't think the API docs are accurate (or at least not descriptive enough), a delete can definitely return a resource not found 404 error. Here's an example:

urllib3.connectionpool: Starting new HTTPS connection (1): management.azure.com:443
urllib3.connectionpool: https://management.azure.com:443 "DELETE /subscriptions/redacted/resourceGroups/group/providers/Microsoft.Network/virtualNetworks/foo?api-version=2021-02-01 HTTP/1.1" 404 97
cli.azure.cli.core.sdk.policies: Response status: 404

That example is for virtual networks though, and yeah in it's API also 404 is not mentioned. I also tried the two DELETE APIs - both public IPs and virtual networks and noticed that they give 204 when trying to delete a non existent resource in a existing resource group. They do give 404 when the resource group itself does not exist. I'm wondering how in the above example 404 came up - maybe it was because of non existent resource group? Could you verify?

Also, I did take care of resource not found in the PR https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/1745/files#diff-fe943e74189aa531d871d7b774c371755043f5bd1b7e8ab14ad3d2b5fc023301R100-R103 similar to existing code, but yeah, seems like that can occur only when resource group itself is not found (probably deleted) which in turn means public IP is also not found / deleted

[karuppiah7890]

I tried to replace GET Public IP API call with GET Tags At Scope API call with Public IP as scope, but for that I had to get the tags client from the tags service

cluster-api-provider-azure/azure/services/tags/client.go

Lines 42 to 46 in e532729

	// newClient creates a new tags client from subscription ID.
	func newClient(auth azure.Authorizer) *azureClient {
	c := newTagsClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
	return &azureClient{c}
	}

which I exported to make it available in publicips package but then I noticed import errors. I think it's due to a cyclic dependency. I see tags service package depending on azure package and azure package depends on publicips service package, so publicips package depending on tags service package would make a full circle causing a cyclic import / cyclic dependency issue. Gotta see how we can resolve it, it's a bit of a tricky thing. I also have to export a few other things, like the azure client returned by the newClient function

cluster-api-provider-azure/azure/services/tags/client.go

Lines 35 to 38 in e532729

	// azureClient contains the Azure go-sdk Client.
	type azureClient struct {
	tags resources.TagsClient
	}

Any thoughts on this @CecileRobertMichon ? Also, who can I request to review #1745 ?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[CecileRobertMichon]

/reopen
/unassign @karuppiah7890

If someone decides to work on this one, please see #1745 for prior work

[k8s-ci-robot]

@CecileRobertMichon: Reopened this issue.

In response to this:

/reopen
/unassign @karuppiah7890

If someone decides to work on this one, please see #1745 for prior work

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[CecileRobertMichon]

/remove-lifecycle rotten

[Jont828]

/assign Jont828