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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect and log unsupported region error for fully-private clusters #3186

Merged
merged 4 commits into from Feb 5, 2021

Conversation

cPu1
Copy link
Collaborator

@cPu1 cPu1 commented Feb 1, 2021

Description

As discovered in #3133 , eu-south-1 does not support fully-private clusters. This PR detects this error condition and logs the error.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 馃く

  • Backfilled missing tests for code in same general area 馃帀
  • Refactored something and made the world a better place 馃専

@@ -166,10 +166,16 @@ func buildVPCEndpointServices(ec2API ec2iface.EC2API, region string, endpoints [
})

if err != nil {
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "InvalidServiceName" {
return nil, &api.UnsupportedFeatureError{
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage over errors.Wrapf?

Copy link
Collaborator Author

@cPu1 cPu1 Feb 2, 2021

Choose a reason for hiding this comment

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

It lets the caller identify errors of this type without relying on string comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case we only print the error anyway, right? I.e. why do we need the errors.As check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it's an issue necessarily but is the handling of the error correct here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case we only print the error anyway, right? I.e. why do we need the errors.As check?

We could just log the error here instead of wrapping it in a typed error, but I have avoided doing any I/O here because this code has no external side effects (if the provider implementation is mocked). We don't want logs for this case appearing in our unit tests. Additionally, the error type provides a standard format for unsupported features that the calling code can identify with a single check. The errors.As check is required for the caller to identify errors of this type because this check happens higher up in the chain where this error may already be wrapped by other errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't mean logging it here but if you were to just return errors.Wrap(err, fmt.Sprintf("fully-private clusters are not supported in region %q, please retry with a different region", region)) here or even errors.New(fmt.Sprintf("fully-private clusters are not supported in region %q, please retry with a different region", region), wouldn't the user see the same thing they're seeing with this PR? We call logger.Critical in pkg/ctl/create/cluster.go anyway.
Like:

				ufe := &api.UnsupportedFeatureError{}
				if errors.As(err, &ufe) {
					logger.Critical(ufe.Message)
				}
				logger.Critical("%s\n", err.Error())

results in logger.Critical being called for this UnsupportedFeatureError twice basically

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With errors.Wrap or errors.New, the log message will not start with the "fully-private clusters are not supported" error, the messages from the errors.Wrap call higher up in the chain will appear first, which can hide the user-friendly error message.

Having an error type with a Message field to represent this condition lets us log a more-specific, user-friendly message while still preserving the error context and logging it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah the UnsupportedFeatureError itself is being wrapped again and isn't the highest level error? Now I get it.
Sorry, I somehow was convinced the logging happened directly after getting the UnsupportedFeatureError.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it isn't the highest-level error.

@cPu1 cPu1 force-pushed the private-cluster-error branch 4 times, most recently from a4f052a to 1881884 Compare February 2, 2021 11:48
Base automatically changed from master to main February 2, 2021 15:06
@cPu1 cPu1 merged commit b7e9643 into eksctl-io:main Feb 5, 2021
@cPu1 cPu1 deleted the private-cluster-error branch February 5, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants