Add AppLens Client and Detector Endpoints #2243

zacharyljones · 2022-07-12T20:54:10Z

Which issue this PR addresses:

What this PR does / why we need it:

This PR adds API endpoints that will enable AppLens Detectors in the Diagnose & Solve Problems Blade in the Azure Portal for ARO clusters.

Test plan for issue:

I have tested that I am able to hit the appropriate AppLens endpoints locally and receive a 401: Unauthorized error. There is no way for me to fully test this change in my dev environment because the "public" AppLens endpoints exist within the AME tenant and cross-tenant communication is not allowed. It may be possible to setup a dev RP in the AME tenant, but this is a conversation that I will have with Red Hat/MS offline.

Is there any documentation that needs to be updated for this PR?

No, the AppLens team has generic documentation for performing this integration with any service (necessary api endpoints, AppLens service endpoints, etc.)

zacharyljones · 2022-07-12T20:56:22Z

pkg/util/cluster/cluster.go

@@ -135,36 +135,17 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
 	}

 	c.log.Infof("creating AAD application")
-	// This is for our CFS scenario where we do not have permissions to allow the RP to create it's own AAD application


Changes to this file were specific to a local development scenario and were accidentally committed. All changes to this file have been reverted.

zacharyljones · 2022-07-12T20:57:02Z

pkg/util/cluster/cluster.go

@@ -135,14 +135,32 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
 	}

 	c.log.Infof("creating AAD application")
-	appID, appSecret, err := c.createApplication(ctx, "aro-"+clusterName)


Changes to this file were specific to a local development scenario and were accidentally committed. All changes to this file have been reverted.

zacharyljones · 2022-07-12T21:27:54Z

@weinong @coreyperkins FYSA

zacharyljones · 2022-07-12T21:31:46Z

The AppLens Client was modeled after the Azure CosmosDB Client in the Azure SDK for Go. Unfortunately this new client cannot be added to the Azure SDK for Go, as the AppLens service is technically private and there is no need for end users to have access to the client.

weinong · 2022-07-13T00:15:43Z

pkg/env/env.go

@@ -79,6 +80,7 @@ type Interface interface {
 	Domain() string
 	FeatureIsSet(Feature) bool
 	FPAuthorizer(string, string) (refreshable.Authorizer, error)
+	FPNewClientCertificateCredential(string) (*azidentity.ClientCertificateCredential, error)


we should use azcore.TokenCredential interface

I will take a look - the current FPauthorizer uses cert based authentication to AAD, so I thought that made the most sense at the time. After our conversation yesterday, it sounds like we want to use the FP certificates to create an azcore.TokenCredential and then authenticate to AppLens using that credential. Is that correct?

correct. that being said, azidentity.ClientCertificateCredential is also azcore.TokenCredential

I attempted to use the azcore.TokenCredential interface, but ran into an issue with code gen. The mock env file that gets generated imports github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared instead of just github.com/Azure/azure-sdk-for-go/sdk/azcore. Similarly, all references to azcore.TokenCredential are replaced with shared.TokenCredential. This causes go validation to fail, unit tests to fail, etc.

A bit of research turned up this azure-sdk-for-go issue. Apparently this is a bug/issue with how older versions of the SDK
export symbols. Some aspects of this issue have been fixed but none of those changes are available in the go 1.17 compatible version of the SDK that I am using.

Sticking with azidentity.ClientCertificateCredential for now.

pkg/frontend/adminactions/azureactions.go

weinong · 2022-07-13T00:31:57Z

pkg/frontend/adminactions/azureactions.go

@@ -102,3 +113,20 @@ func (a *azureActions) VMResize(ctx context.Context, vmName string, size string)
 	vm.HardwareProfile.VMSize = mgmtcompute.VirtualMachineSizeTypes(size)
 	return a.virtualMachines.CreateOrUpdateAndWait(ctx, clusterRGName, vmName, vm)
 }
+
+func (a *azureActions) AppLensGetDetector(ctx context.Context, detectorId string) ([]byte, error) {
+	resp, err := a.appLens.GetDetector(ctx, &applens.GetDetectorOptions{ResourceID: a.oc.Properties.ClusterProfile.ResourceGroupID, DetectorID: detectorId})


ResourceGroupID?

Updated to use cluster resource id, and not cluster resource group resource id

weinong · 2022-07-13T00:33:53Z

pkg/util/azureclient/applens/applens.go

+var _ AppLensClient = &appLensClient{}
+
+// NewAppLensClient returns a new AppLensClient
+func NewAppLensClient(env *azureclient.AROEnvironment, clientCertCred *azidentity.ClientCertificateCredential) AppLensClient {


let's also use azcore.TokenCredential

Do you want this in addition to azidentity.ClientCertificateCredential, or do you want me to replace azidentity.ClientCertificateCredential with azcore.TokenCredential?

azidentity.ClientCertificateCredential is the implementation of azcore.TokenCredential interface

Refer to previous comment

weinong · 2022-07-13T00:35:31Z

pkg/util/azureclient/applens/applens_client.go

+		return nil, err
+	}
+
+	return []string{fmt.Sprintf("%s://%s/.default", u.Scheme, u.Hostname())}, nil


i'm pretty sure the endpoint is not the scope.

This was copied over from the Cosmos client - I figured I would resolve any issues related to the scope during testing as I wasn't sure whether it would be needed or not. Do you think the scope should be something else, or removed altogether?

you have to have scope to get AAD token. in ADAL, it's called ResourceID. In MSAL, it's called Scope.
https://msazure.visualstudio.com/CloudNativeCompute/_git/aks-rp?path=/resourceprovider/server/microsoft.com/containerservice/templates/implementation.go&version=GBmaster&line=676&lineEnd=677&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

Understood - with that information I was able to use the appropriate scopes provided by the AppLens team. The code for creating the scope from the target endpoint has been removed.

weinong · 2022-07-13T00:44:10Z

pkg/util/azureclient/applens/applens_client.go

+	ctx context.Context,
+	requestOptions appLensRequestOptions,
+	requestEnricher func(*policy.Request)) (*http.Response, error) {
+	req, err := c.createRequest(ctx, http.MethodGet, requestOptions, requestEnricher)


AppLens api is Post, iirc

Oof - you are correct. I will swap the get method for a post and update relevant unit tests. MTF.

Client updated to use Post. Unit tests also updated.

pkg/util/azureclient/applens/applens_client.go

github-actions · 2022-07-13T04:12:42Z

Please rebase pull request.

weinong · 2022-07-13T16:02:27Z

can you also add user agent please?

weinong · 2022-07-13T16:04:40Z

pkg/frontend/adminactions/azureactions.go

+}
+
+func (a *azureActions) AppLensListDetectors(ctx context.Context) ([]byte, error) {
+	resp, err := a.appLens.ListDetectors(ctx, &applens.ListDetectorsOptions{ResourceID: a.oc.Properties.ClusterProfile.ResourceGroupID})


same here. using resource group ID seems wrong.

Updated to use cluster resource id, and not cluster resource group resource id

zacharyljones · 2022-07-13T20:55:48Z

can you also add user agent please?

@weinong Can you elaborate on this? I see that it is added in AKS, but I do not see it in the AppLens example. What does the user agent afford us?

rogbas · 2022-12-06T20:04:47Z

I'm assuming AppLens relies on an Azure service of same name, correct? Have we checked if it is available in all the Azure regions we support? cc @weinong

weinong · 2022-12-07T05:49:49Z

I'm assuming AppLens relies on an Azure service of same name, correct? Have we checked if it is available in all the Azure regions we support? cc @weinong

yes, it's available in public and fairfax

pkg/util/azureclient/applens/applens_client.go

s-amann · 2022-12-07T12:44:23Z

pkg/util/azureclient/applens/applens_client.go

+		return nil, err
+	}
+
+	successResponse := (response.StatusCode >= 200 && response.StatusCode < 300) || response.StatusCode == 304


Why are we expecting to receive a variety of response codes from applens for a successful call?

Agreed, I think it would be better to have a list of all success responses from applens and and evaluate based on that list rather than have a huge range.

pkg/util/azureclient/applens/mock.go

s-amann · 2022-12-07T13:02:11Z

pkg/util/azureclient/environments.go

@@ -33,6 +40,9 @@ var (
 		ActualCloudName:          "AzureUSGovernment",
 		GenevaMonitoringEndpoint: "https://gcs.monitoring.core.usgovcloudapi.net/",
 		AppSuffix:                "aro.azure.us",
+		AppLensEndpoint:          "https://diag-runtimehost-prod-bn1-001.azurewebsites.us/api/invoke",
+		AppLensScope:             "https://microsoft.onmicrosoft.com/runtimehost",


Why is the scope here a URL?

s-amann · 2022-12-07T20:21:35Z

/azp run ci

azure-pipelines · 2022-12-07T20:21:45Z

Azure Pipelines successfully started running 1 pipeline(s).

cfsmikedrum · 2022-12-09T16:55:21Z

@s-amann, the rebase is complete

pkg/frontend/adminactions/azureactions.go

…cate chain

* Change to using the environment AppLens tenant ID Add the tenant ID values for the clouds * Remove the blank line Co-authored-by: Nont <nthanonchai@microsoft.com>

Also remove empty line from go.mod

cadenmarchese

Some outstanding comments here, but as discussed I'll merge this to unblock AppLens onboarding @s-amann

cadenmarchese · 2022-12-16T17:32:56Z

pkg/util/azureclient/applens/applens_client.go

+		return nil, err
+	}
+
+	successResponse := (response.StatusCode >= 200 && response.StatusCode < 300) || response.StatusCode == 304


Agreed, I think it would be better to have a list of all success responses from applens and and evaluate based on that list rather than have a huge range.

zacharyljones added the hold Hold label Jul 12, 2022

zacharyljones requested review from jewzaam, m1kola, bennerv, hawkowl, rogbas, petrkotas, ross-bryan and darthhexx as code owners July 12, 2022 20:54

zacharyljones commented Jul 12, 2022

View reviewed changes

zacharyljones force-pushed the zjones/app_lens_detectors branch 2 times, most recently from 21a7e79 to 5807b66 Compare July 12, 2022 21:24

weinong reviewed Jul 13, 2022

View reviewed changes

pkg/frontend/adminactions/azureactions.go Show resolved Hide resolved

weinong reviewed Jul 13, 2022

View reviewed changes

pkg/util/azureclient/applens/applens_client.go Show resolved Hide resolved

github-actions bot added the needs-rebase branch needs a rebase label Jul 13, 2022

weinong reviewed Jul 13, 2022

View reviewed changes

zacharyljones force-pushed the zjones/app_lens_detectors branch from 5807b66 to 3b591a4 Compare July 13, 2022 20:38

github-actions bot removed the needs-rebase branch needs a rebase label Jul 13, 2022

zacharyljones force-pushed the zjones/app_lens_detectors branch from 3b591a4 to a726c05 Compare July 13, 2022 20:50

zacharyljones force-pushed the zjones/app_lens_detectors branch 4 times, most recently from 82b2651 to 3707b8d Compare October 18, 2022 15:54

zacharyljones force-pushed the zjones/app_lens_detectors branch 2 times, most recently from d0e2b40 to 1aaa832 Compare October 21, 2022 16:46

zacharyljones requested a review from facchettos as a code owner October 21, 2022 16:46

zacharyljones force-pushed the zjones/app_lens_detectors branch from 1aaa832 to 756a28e Compare November 9, 2022 20:58

zacharyljones requested a review from cadenmarchese as a code owner November 9, 2022 20:58

s-amann reviewed Dec 7, 2022

View reviewed changes

pkg/util/azureclient/applens/applens_client.go Show resolved Hide resolved

s-amann reviewed Dec 7, 2022

View reviewed changes

pkg/util/azureclient/applens/mock.go Show resolved Hide resolved

s-amann reviewed Dec 7, 2022

View reviewed changes

cfsmikedrum force-pushed the zjones/app_lens_detectors branch from fd26c8d to 7478b1b Compare December 9, 2022 16:25

weinong reviewed Dec 10, 2022

View reviewed changes

pkg/frontend/adminactions/azureactions.go Outdated Show resolved Hide resolved

Zach Jones and others added 6 commits December 15, 2022 15:05

feat: add azidentity vendor files

3bc385d

feat: addition of applens client and detector endpoints

ea219d5

fix: updated client ceritifcate credential options to include certifi…

df20180

…cate chain

feat: updated applens public endpoint

2d41074

Fix the TenantID for AppLens (#9)

43eb79d

* Change to using the environment AppLens tenant ID Add the tenant ID values for the clouds * Remove the blank line Co-authored-by: Nont <nthanonchai@microsoft.com>

Update go.sum

2bd4c35

Also remove empty line from go.mod

cfsmikedrum force-pushed the zjones/app_lens_detectors branch from 950750e to 2bd4c35 Compare December 15, 2022 20:12

Add empty line to go.mod

160ca82

cadenmarchese approved these changes Dec 16, 2022

View reviewed changes

cadenmarchese merged commit fe9017d into Azure:master Dec 16, 2022

Add AppLens Client and Detector Endpoints #2243

Add AppLens Client and Detector Endpoints #2243

Conversation

zacharyljones commented Jul 12, 2022 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharyljones commented Jul 12, 2022 • edited Loading

zacharyljones commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharyljones Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2022

weinong commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharyljones commented Jul 13, 2022

rogbas commented Dec 6, 2022

weinong commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-amann commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

cfsmikedrum commented Dec 9, 2022

cadenmarchese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharyljones commented Jul 12, 2022 •

edited

Loading

zacharyljones commented Jul 12, 2022 •

edited

Loading

zacharyljones Jul 13, 2022 •

edited

Loading