Handle Hive ProvisionFailed=True conditions within RP and pass errors in API response #3116

tsatam · 2023-08-23T17:58:19Z

Which issue this PR addresses:

What this PR does / why we need it:

Implements a pattern for handling Hive install failures in the RP, and handles one specific case (Azure RequestDisallowedByPolicy) by passing the policy errors along to the requester.

This PR also handles the generic Azure "InvalidTemplateDeployment" error, as this error contains many potential underlying errors including the above RequestDisallowedByPolicy error. The intent here is to cascade these generic errors to customers, but also alert SREs that a more specific error case ought to be implemented and handled as well. The ordering of patterns in the Hive ConfigMap ensures that more specific errors will take precedence over this generic one.

Test plan for issue:

With this code running on e.g. a local RP setup that installs via Hive, with the included additional-install-log-regexes ConfigMap applied in the Hive cluster:

Create a policy that blocks e.g. creation of the bootstrap/master VMs created by the installer
Attempt a cluster creation

Unit tests have been added for the specific Hive install failure cases implemented within the PR.

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

Hive Cluster Provision Pod Not Ready SOP (todo)

Additional Notes

This PR is soft-dependent on a PR to our Hive deployment pipeline in order to add the additional-install-log-regexes ConfigMap to its deployment artifacts. An RP with this code can be deployed without that, but the functionality present here will not go into effect as Hive will report all failed installs as UnknownError until the ConfigMap is present.

gvanderpotte

Looks good to me.
This is great fundations work for better handling of installation errors happening during hive controlled ocp installation, and will help with customer experience for a set of well identified scenarios

SudoBrendan

Great PR! Very exciting, can't wait to see a merge on this. I had a few questions/nits. Thanks for your hard work!

SudoBrendan · 2023-08-29T17:31:53Z

hack/genhiveconfig/genhiveconfig.go

+	if path != "" {
+		return os.WriteFile(path, configmapRaw, 0666)
+	} else {


nit: if we can always genhiveconfig.go | oc apply -f - is there a reason we'd need to write to FS, or can we remove this logic for simplicity?

I haven't plumbed this file through to our actual deployment pipelines. As of right now it's manually copied from this repository into the pipelines and applied statically alongside other static Hive resources.

SudoBrendan · 2023-08-29T17:36:56Z

hack/hive-config/hive-additional-install-log-regexes.yaml

+      name: AzureInvalidTemplateDeployment
+      searchRegexStrings:
+      - '"code":\w?"InvalidTemplateDeployment"'
+kind: ConfigMap


Hmm, ok now I'm wondering why we need genhiveconfig.go at all? What's wrong with us keeping a static resource in source control somewhere we apply via GitOps/etc? I guess I don't quite see the value of that script given this resource's existence since it's 99% of the way there, though I'm sure there's a reason.

The primary benefit here is to get unit tests for these regex patterns, as well as to tie our RP-side implementation to the same strings used in the ConfigMap.

We could just maintain the CM directly, just like Hive itself does, though they also maintain unit tests that send that ConfigMap through their implementation and assert that the regexes do as they're expected.

Where this is this going to be deployed?

another point to consider - when we want to add a regex (perhaps for 423s on storage account api calls) we need to do a full RP release vs a config+hive release.

SudoBrendan · 2023-08-29T17:42:39Z

pkg/hive/failure/handler.go

@@ -0,0 +1,96 @@
+package failure


super-nit: I'm not sure I like the noun handler for this, since my assumption would be that this pkg is meant to implement golang's standard lib's net/http.Handler, but it might just be me :p

I agree, open to better name ideas for this "ThingThatTakesHiveProvisionFailed=TrueConditionsAndGeneratesAUsefulUserErrorFromThem"er (transformer? enricher?)

SudoBrendan · 2023-08-29T17:47:57Z

pkg/hive/failure/handler.go

+	case AzureInvalidTemplateDeployment.Reason:
+		armError, err := parseDeploymentFailedJson(*installLog)
+		if err != nil {
+			return err
+		}
+
+		return wrapArmError(
+			AzureInvalidTemplateDeployment.Message,
+			*armError,
+		)


Can we update the PR description to include this too? It looks like we're processing two cases.

Added this extra case and reasoning. Open to suggestions on changing how we handle this case as well (e.g. below comment)

SudoBrendan · 2023-08-29T17:53:14Z

pkg/hive/failure/handler.go

+}
+
+func parseDeploymentFailedJson(installLog string) (*mgmtfeatures.ErrorResponse, error) {
+	regex := regexp.MustCompile(`level=error msg=400: DeploymentFailed: : Deployment failed. Details: : : (\{.*\})`)


Trying to run my head through this... Is there ever a case where we might leak something like our credentials through an error like this to a customer? Should we perform sanitation of data like service principal IDs/tokens/passwords to be safe?

My understanding was that the previous RP implementation (before Hive) would directly return this object to the customer, but I can validate exactly what was done then and ensure we do at least as thorough a job as the previous implementation did.

I'm also open to implementing specific transformations for specific sub-errors here, and not returning the generic error at all, and I think if we want to pursue that path, we should align other parts of the RP that handle RequestDisallowedByPolicy errors (e.g.

ARO-RP/pkg/cluster/deploybaseresources.go

Lines 252 to 273 in 3b370ae

if detailedErr, ok := err.(autorest.DetailedError); ok {

if strings.Contains(detailedErr.Original.Error(), "RequestDisallowedByPolicy") {

return &api.CloudError{

StatusCode: http.StatusBadRequest,

CloudErrorBody: &api.CloudErrorBody{

Code: api.CloudErrorCodeRequestDisallowedByPolicy,

Message: fmt.Sprintf("Resource %s was disallowed by policy.",

subnetId[strings.LastIndex(subnetId, "/")+1:],

),

Details: []api.CloudErrorBody{

{

Code: api.CloudErrorCodeRequestDisallowedByPolicy,

Message: fmt.Sprintf("Policy definition : %s\nPolicy Assignment : %s",

regexp.MustCompile(`policyDefinitionName":"([^"]+)"`).FindStringSubmatch(detailedErr.Original.Error())[1],

regexp.MustCompile(`policyAssignmentName":"([^"]+)"`).FindStringSubmatch(detailedErr.Original.Error())[1],

),

},

},

},

}

}

}

) to return errors the same way.

pkg/hive/manager.go

…finition

…/ Azure policy errors

cadenmarchese

Thanks for your work on this, returning some pre-defined common errors to customers will be a big upgrade. I just have a few comments going through this, but mostly this LGTM!

cadenmarchese · 2023-10-03T15:14:47Z

pkg/hive/failure/reasons.go

+	// Order within this array determines precedence. Earlier entries will take
+	// priority over later ones.


I'm not sure I understand this comment. Isn't this just establishing a list of errors that we range over? If not, how is priority decided?

cadenmarchese · 2023-10-03T16:55:42Z

pkg/hive/failure/handler.go

+	"github.com/Azure/ARO-RP/pkg/api"
+)
+
+var genericErr = &api.CloudError{


It might be worth exporting this / other err definitions and use them in your tests. I see we redeclare this in manager_test.go. Thoughts?

cadenmarchese · 2023-10-03T17:17:49Z

pkg/hive/manager_test.go

+					Reason: failure.AzureRequestDisallowedByPolicy.Reason,
+				},
+			),
+			cp: makeClusterProvision(`level=info msg=running in local development mode


Do we need the whole cluster provision log, or can we just specify the error that matches your regex?

s-amann · 2023-10-25T12:31:05Z

pkg/hive/failure/reasons.go

@@ -0,0 +1,38 @@
+package failure


do we have to have this file? it is essentially a duplicate of our configuration. I understand the benefit of testing the regex, but we are testing a copy of it by duplicating the yaml into a go file.

hack/hive-config/hive-additional-install-log-regexes.yaml

edit: I suppose since the file going to production isn't the one above either, this is less of an issue but the same regex are now in triplicate.

The YAML is a generated artifact, which make generate will keep up-to-date based on the source-of-truth in this file.

As is, we would need to manually copy this YAML into our deployment pipeline repository, but I believe it would be possible to automate that as well.

s-amann

lgtm, I think we need to figure out where to store the regex so we can test, deploy, and update one copy, but this is a great step towards the goal of providing better errors.

petrkotas

Well I think this is a good start. LGTM

petrkotas

Well I think this is a good start. LGTM

tsatam marked this pull request as ready for review August 23, 2023 18:03

tsatam requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, facchettos, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, Shivkumar13, yjst2012, anshulvermapatel and hlipsig as code owners August 23, 2023 18:03

tsatam force-pushed the ARO-3220/rp-handles-hive-provisionfailed branch 3 times, most recently from b8a7ec7 to 5e44eeb Compare August 23, 2023 19:01

tsatam added ready-for-review loki labels Aug 24, 2023

gvanderpotte reviewed Aug 25, 2023

View reviewed changes

SudoBrendan reviewed Aug 29, 2023

View reviewed changes

tsatam mentioned this pull request Aug 29, 2023

Add hack script to build hive install-log-regexes from ARO definition #3098

Closed

tsatam force-pushed the ARO-3220/rp-handles-hive-provisionfailed branch from a4c3380 to b7f819b Compare September 7, 2023 20:05

tsatam force-pushed the ARO-3220/rp-handles-hive-provisionfailed branch from b7f819b to bd7e1a6 Compare September 15, 2023 18:44

tsatam added 3 commits September 20, 2023 12:51

Add hack script to generate hive install-log-regexes.yaml from ARO de…

438411b

…finition

Implement generic handling for Hive ProvisionFailed=True conditions w…

73f3b03

…/ Azure policy errors

Rename installLogsForDeployment to indicate latest deployment

5a5cd93

tsatam force-pushed the ARO-3220/rp-handles-hive-provisionfailed branch from bd7e1a6 to 5a5cd93 Compare September 20, 2023 16:51

cadenmarchese reviewed Oct 3, 2023

View reviewed changes

cadenmarchese added the next-release To be included in the next RP release rollout label Oct 19, 2023

s-amann reviewed Oct 25, 2023

View reviewed changes

s-amann approved these changes Oct 25, 2023

View reviewed changes

cadenmarchese removed the next-release To be included in the next RP release rollout label Oct 25, 2023

petrkotas approved these changes Oct 25, 2023

View reviewed changes

cadenmarchese merged commit f421bac into Azure:master Nov 17, 2023
18 checks passed

This was referenced Jul 12, 2024

CIF: OSProvisioningTimedOut #3677

Open

CIF - ServerError: ZonalAllocationFailed #3691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Hive ProvisionFailed=True conditions within RP and pass errors in API response #3116

Handle Hive ProvisionFailed=True conditions within RP and pass errors in API response #3116

tsatam commented Aug 23, 2023 •

edited

Loading

gvanderpotte left a comment

SudoBrendan left a comment

SudoBrendan Aug 29, 2023

tsatam Aug 29, 2023

SudoBrendan Aug 29, 2023 •

edited

Loading

tsatam Aug 29, 2023

petrkotas Oct 25, 2023

s-amann Oct 25, 2023

SudoBrendan Aug 29, 2023 •

edited

Loading

tsatam Aug 29, 2023

SudoBrendan Aug 29, 2023 •

edited

Loading

tsatam Aug 29, 2023

SudoBrendan Aug 29, 2023

tsatam Aug 29, 2023 •

edited

Loading

cadenmarchese left a comment

cadenmarchese Oct 3, 2023

cadenmarchese Oct 3, 2023

cadenmarchese Oct 3, 2023

s-amann Oct 25, 2023 •

edited

Loading

tsatam Oct 25, 2023

s-amann left a comment

petrkotas left a comment

petrkotas left a comment

	if detailedErr, ok := err.(autorest.DetailedError); ok {
	if strings.Contains(detailedErr.Original.Error(), "RequestDisallowedByPolicy") {
	return &api.CloudError{
	StatusCode: http.StatusBadRequest,
	CloudErrorBody: &api.CloudErrorBody{
	Code: api.CloudErrorCodeRequestDisallowedByPolicy,
	Message: fmt.Sprintf("Resource %s was disallowed by policy.",
	subnetId[strings.LastIndex(subnetId, "/")+1:],
	),
	Details: []api.CloudErrorBody{
	{
	Code: api.CloudErrorCodeRequestDisallowedByPolicy,
	Message: fmt.Sprintf("Policy definition : %s\nPolicy Assignment : %s",
	regexp.MustCompile(`policyDefinitionName":"([^"]+)"`).FindStringSubmatch(detailedErr.Original.Error())[1],
	regexp.MustCompile(`policyAssignmentName":"([^"]+)"`).FindStringSubmatch(detailedErr.Original.Error())[1],
	),
	},
	},
	},
	}
	}
	}

		// Order within this array determines precedence. Earlier entries will take
		// priority over later ones.

Handle Hive ProvisionFailed=True conditions within RP and pass errors in API response #3116

Handle Hive ProvisionFailed=True conditions within RP and pass errors in API response #3116

Conversation

tsatam commented Aug 23, 2023 • 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?

Additional Notes

gvanderpotte left a comment

Choose a reason for hiding this comment

SudoBrendan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBrendan Aug 29, 2023 • 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

SudoBrendan Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SudoBrendan Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsatam Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

cadenmarchese left a 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

s-amann Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-amann left a comment

Choose a reason for hiding this comment

petrkotas left a comment

Choose a reason for hiding this comment

petrkotas left a comment

Choose a reason for hiding this comment

tsatam commented Aug 23, 2023 •

edited

Loading

SudoBrendan Aug 29, 2023 •

edited

Loading

SudoBrendan Aug 29, 2023 •

edited

Loading

SudoBrendan Aug 29, 2023 •

edited

Loading

tsatam Aug 29, 2023 •

edited

Loading

s-amann Oct 25, 2023 •

edited

Loading