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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws/provider: add validation function to the tagsSchema #12127

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@netjunki
Contributor

netjunki commented Feb 21, 2017

Validate tag values. Ran into the following error today:

* aws_s3_bucket.bucket: InvalidTag: The TagValue you have provided is invalid
status code: 400, request id: C7CA2ED60DCE4678

I requested a PR to add the name of the bucket which generated the tag error, but felt it would also be valuable to have the tag actually validated so one would get errors about problem tags during plan.

So with this terraform template:

resource "aws_instance" "test" {
  tags {
    abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz = "()abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"
  }
   instance_type = "t2.micro"
  ami = "ami-1c221e76"
}

I can now generate the following error output during terraform plan:

3 error(s) occurred:

* aws_instance.test: tag "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" name cannot be longer than '127' characters
* aws_instance.test: tag "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" value "()abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" cannot be longer than '255' characters
* aws_instance.test: tag "tags" value "()abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" contains invalid characters (^[0-9A-Za-z+-=._:\@]+$)

@netjunki netjunki changed the title from add validation function to the tagsSchema to aws/provider: add validation function to the tagsSchema Feb 21, 2017

@pessimism

This comment has been minimized.

Show comment
Hide comment
@pessimism

pessimism Mar 7, 2017

This validation would have saved me some time this morning catching a merge issue

pessimism commented Mar 7, 2017

This validation would have saved me some time this morning catching a merge issue

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Mar 7, 2017

Contributor

Encountered a new error related to this today:

* aws_alb_target_group.sdp: Error Modifying Tags on ALB Target Group: ValidationError: 1 validation error detected: Value 'smi-sdp}-tgt' at 'tags.1.member.value' failed to satisfy constraint: Member must satisfy regular expression pattern: ^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$
status code: 400, request id: 14f24721-033d-11e7-bc8f-950c9583e793

This pointed to a better regex to use so updated the code accordingly.

Contributor

netjunki commented Mar 7, 2017

Encountered a new error related to this today:

* aws_alb_target_group.sdp: Error Modifying Tags on ALB Target Group: ValidationError: 1 validation error detected: Value 'smi-sdp}-tgt' at 'tags.1.member.value' failed to satisfy constraint: Member must satisfy regular expression pattern: ^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$
status code: 400, request id: 14f24721-033d-11e7-bc8f-950c9583e793

This pointed to a better regex to use so updated the code accordingly.

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Mar 16, 2017

Member

Hi @netjunki
thanks for the contribution.

I'm afraid that the validation rules aren't that simple however, because we use this generic tagsSchema() in multiple resources whose tags impose different limitations.

For example

grep -R 'tagsSchema()' ./*
./ec2_filters.go:// "tags" which conforms to the schema returned by the tagsSchema() function.
./resource_aws_alb.go:			"tags": tagsSchema(),
./resource_aws_alb_target_group.go:			"tags": tagsSchema(),
./resource_aws_ami.go:		"tags": tagsSchema(),
./resource_aws_cloudfront_distribution.go:			"tags": tagsSchema(),
./resource_aws_cloudtrail.go:			"tags": tagsSchema(),
./resource_aws_cloudwatch_log_group.go:			"tags": tagsSchema(),
./resource_aws_codebuild_project.go:			"tags": tagsSchema(),
./resource_aws_customer_gateway.go:			"tags": tagsSchema(),
./resource_aws_db_event_subscription.go:			"tags": tagsSchema(),
./resource_aws_db_instance.go:			"tags": tagsSchema(),
./resource_aws_db_option_group.go:			"tags": tagsSchema(),
./resource_aws_db_parameter_group.go:			"tags": tagsSchema(),
./resource_aws_db_security_group.go:			"tags": tagsSchema(),
./resource_aws_db_subnet_group.go:			"tags": tagsSchema(),
./resource_aws_default_network_acl.go:			"tags": tagsSchema(),
./resource_aws_default_route_table.go:			"tags": tagsSchema(),
./resource_aws_dynamodb_table.go:			"tags": tagsSchema(),
./resource_aws_ebs_volume.go:			"tags": tagsSchema(),
./resource_aws_efs_file_system.go:			"tags": tagsSchema(),
./resource_aws_elastic_beanstalk_environment.go:			"tags": tagsSchema(),
./resource_aws_elasticache_cluster.go:		"tags": tagsSchema(),
./resource_aws_elasticsearch_domain.go:			"tags": tagsSchema(),
./resource_aws_elb.go:			"tags": tagsSchema(),
./resource_aws_emr_cluster.go:			"tags": tagsSchema(),
./resource_aws_glacier_vault.go:			"tags": tagsSchema(),
./resource_aws_instance.go:			"tags": tagsSchema(),
./resource_aws_internet_gateway.go:			"tags": tagsSchema(),
./resource_aws_kinesis_stream.go:			"tags": tagsSchema(),
./resource_aws_network_acl.go:			"tags": tagsSchema(),
./resource_aws_network_interface.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster_instance.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster_parameter_group.go:			"tags": tagsSchema(),
./resource_aws_redshift_cluster.go:			"tags": tagsSchema(),
./resource_aws_redshift_subnet_group.go:			"tags": tagsSchema(),
./resource_aws_route53_health_check.go:			"tags": tagsSchema(),
./resource_aws_route53_zone.go:			"tags": tagsSchema(),
./resource_aws_route_table.go:			"tags": tagsSchema(),
./resource_aws_s3_bucket.go:			"tags": tagsSchema(),
./resource_aws_s3_bucket_object.go:			"tags": tagsSchema(),
./resource_aws_security_group.go:			"tags": tagsSchema(),
./resource_aws_subnet.go:			"tags": tagsSchema(),
./resource_aws_vpc.go:			"tags": tagsSchema(),
./resource_aws_vpc_peering_connection.go:			"tags":      tagsSchema(),
./resource_aws_vpc_peering_connection_accepter.go:			"tags":      tagsSchema(),
./resource_aws_vpn_connection.go:			"tags": tagsSchema(),
./resource_aws_vpn_gateway.go:			"tags": tagsSchema(),

I believe there's value in such validation, but we'd probably need to come up with some service-specific validation functions and maybe pass those into something like tagsSchemaWithValidation(validateFunc) to keep the benefits of reusable schema, what do you think?

Member

radeksimko commented Mar 16, 2017

Hi @netjunki
thanks for the contribution.

I'm afraid that the validation rules aren't that simple however, because we use this generic tagsSchema() in multiple resources whose tags impose different limitations.

For example

grep -R 'tagsSchema()' ./*
./ec2_filters.go:// "tags" which conforms to the schema returned by the tagsSchema() function.
./resource_aws_alb.go:			"tags": tagsSchema(),
./resource_aws_alb_target_group.go:			"tags": tagsSchema(),
./resource_aws_ami.go:		"tags": tagsSchema(),
./resource_aws_cloudfront_distribution.go:			"tags": tagsSchema(),
./resource_aws_cloudtrail.go:			"tags": tagsSchema(),
./resource_aws_cloudwatch_log_group.go:			"tags": tagsSchema(),
./resource_aws_codebuild_project.go:			"tags": tagsSchema(),
./resource_aws_customer_gateway.go:			"tags": tagsSchema(),
./resource_aws_db_event_subscription.go:			"tags": tagsSchema(),
./resource_aws_db_instance.go:			"tags": tagsSchema(),
./resource_aws_db_option_group.go:			"tags": tagsSchema(),
./resource_aws_db_parameter_group.go:			"tags": tagsSchema(),
./resource_aws_db_security_group.go:			"tags": tagsSchema(),
./resource_aws_db_subnet_group.go:			"tags": tagsSchema(),
./resource_aws_default_network_acl.go:			"tags": tagsSchema(),
./resource_aws_default_route_table.go:			"tags": tagsSchema(),
./resource_aws_dynamodb_table.go:			"tags": tagsSchema(),
./resource_aws_ebs_volume.go:			"tags": tagsSchema(),
./resource_aws_efs_file_system.go:			"tags": tagsSchema(),
./resource_aws_elastic_beanstalk_environment.go:			"tags": tagsSchema(),
./resource_aws_elasticache_cluster.go:		"tags": tagsSchema(),
./resource_aws_elasticsearch_domain.go:			"tags": tagsSchema(),
./resource_aws_elb.go:			"tags": tagsSchema(),
./resource_aws_emr_cluster.go:			"tags": tagsSchema(),
./resource_aws_glacier_vault.go:			"tags": tagsSchema(),
./resource_aws_instance.go:			"tags": tagsSchema(),
./resource_aws_internet_gateway.go:			"tags": tagsSchema(),
./resource_aws_kinesis_stream.go:			"tags": tagsSchema(),
./resource_aws_network_acl.go:			"tags": tagsSchema(),
./resource_aws_network_interface.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster_instance.go:			"tags": tagsSchema(),
./resource_aws_rds_cluster_parameter_group.go:			"tags": tagsSchema(),
./resource_aws_redshift_cluster.go:			"tags": tagsSchema(),
./resource_aws_redshift_subnet_group.go:			"tags": tagsSchema(),
./resource_aws_route53_health_check.go:			"tags": tagsSchema(),
./resource_aws_route53_zone.go:			"tags": tagsSchema(),
./resource_aws_route_table.go:			"tags": tagsSchema(),
./resource_aws_s3_bucket.go:			"tags": tagsSchema(),
./resource_aws_s3_bucket_object.go:			"tags": tagsSchema(),
./resource_aws_security_group.go:			"tags": tagsSchema(),
./resource_aws_subnet.go:			"tags": tagsSchema(),
./resource_aws_vpc.go:			"tags": tagsSchema(),
./resource_aws_vpc_peering_connection.go:			"tags":      tagsSchema(),
./resource_aws_vpc_peering_connection_accepter.go:			"tags":      tagsSchema(),
./resource_aws_vpn_connection.go:			"tags": tagsSchema(),
./resource_aws_vpn_gateway.go:			"tags": tagsSchema(),

I believe there's value in such validation, but we'd probably need to come up with some service-specific validation functions and maybe pass those into something like tagsSchemaWithValidation(validateFunc) to keep the benefits of reusable schema, what do you think?

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Mar 16, 2017

Contributor

@radeksimko,

The tagging standards being different everywhere bothered me... so I decided to do some research instead. I think there's some more to do here (do the same analysis on all of the tagable resource types) but figured I'd share the results I have since I'm waiting for some efs resources to initialize.

Service Key Length Value Length Validation
route53 128 256 Not specified
ec2 127 255 letters, spaces, and numbers representable in UTF-8, plus the following special characters: + - = . _ : / @
rds 128 256 the string may contain only the set of Unicode letters, digits, white-space, '_', '.', '/', '=', '+', '-'

Those all look suspiciously similar to me... so similar that I'm skeptical that the restrictions are actually different. So I went and tested them experimentally. :-)

Take rds... their documentation kind of says they support the same set of restrictions as ec2... the only difference is the @ and : symbols seems to be missing:

Restricted chars from specifications:

Service + - = . _ : / @
ec2 X X X X X X X X
rds X X X X X X
route53 ? ? ? ? ? ? ? ?

But testing rds experimentally it supports having both : and @ in the tags... And actually the fact that it allows : makes sense since amazon uses aws: and rds: as prefixes that they specifically mention you can't use as a user... though apparently that's just a recommendation... the console UI let me set those as well.

I tested route53 as well and it seems to support the same restricted set as the other two services.

Restricted chars from experimentation (differences highlighted in bold):

Service + - = . _ : / @
ec2 X X X X X X X X
rds X X X X X X X X
route53 X X X X X X X X

For the length restrictions I let's compare the actuals to the documented values.

Service Key Len Spec Key Len Actual Val Len Spec Val Len Actual
route53 128 128 256 256
rds 128 128 256 255
ec2 127 127 255 255

So on the length side... we found out that the docs actually have a mistake in them and the max length for the rds value is actually 255 not 256. We're talking about a single character here... I don't think I'd want to introduce the complexity of having separate validations for all the different services.

I think we may want to experimentally validate all of the tag limits for all tagable resources though just to make sure we're not doing anything crazy.

Thoughts?

Contributor

netjunki commented Mar 16, 2017

@radeksimko,

The tagging standards being different everywhere bothered me... so I decided to do some research instead. I think there's some more to do here (do the same analysis on all of the tagable resource types) but figured I'd share the results I have since I'm waiting for some efs resources to initialize.

Service Key Length Value Length Validation
route53 128 256 Not specified
ec2 127 255 letters, spaces, and numbers representable in UTF-8, plus the following special characters: + - = . _ : / @
rds 128 256 the string may contain only the set of Unicode letters, digits, white-space, '_', '.', '/', '=', '+', '-'

Those all look suspiciously similar to me... so similar that I'm skeptical that the restrictions are actually different. So I went and tested them experimentally. :-)

Take rds... their documentation kind of says they support the same set of restrictions as ec2... the only difference is the @ and : symbols seems to be missing:

Restricted chars from specifications:

Service + - = . _ : / @
ec2 X X X X X X X X
rds X X X X X X
route53 ? ? ? ? ? ? ? ?

But testing rds experimentally it supports having both : and @ in the tags... And actually the fact that it allows : makes sense since amazon uses aws: and rds: as prefixes that they specifically mention you can't use as a user... though apparently that's just a recommendation... the console UI let me set those as well.

I tested route53 as well and it seems to support the same restricted set as the other two services.

Restricted chars from experimentation (differences highlighted in bold):

Service + - = . _ : / @
ec2 X X X X X X X X
rds X X X X X X X X
route53 X X X X X X X X

For the length restrictions I let's compare the actuals to the documented values.

Service Key Len Spec Key Len Actual Val Len Spec Val Len Actual
route53 128 128 256 256
rds 128 128 256 255
ec2 127 127 255 255

So on the length side... we found out that the docs actually have a mistake in them and the max length for the rds value is actually 255 not 256. We're talking about a single character here... I don't think I'd want to introduce the complexity of having separate validations for all the different services.

I think we may want to experimentally validate all of the tag limits for all tagable resources though just to make sure we're not doing anything crazy.

Thoughts?

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Mar 24, 2017

Contributor

Continuing on with this effort (will just update this comment while I do this)...

Supported Characters

Service + - = . _ : / @
efs X X X X X X X X
s3 X X X X X X X X
redshift X X X X X X X X
elasticache X X X X X X X X
acm X X X X X X X X
kinesis X X X X X X X X
vpc X X X X X X X X

Length

Service Key Len Spec Key Len Actual Val Len Spec Val Len Actual
efs ? 128 ? 256
s3 128 128 256 256
redshift 128 128 256 256
elasticache 128 128 256 256
acm 127 128 255 256
kinesis 128 128 255 256
vpc ? 127 ? 255
Contributor

netjunki commented Mar 24, 2017

Continuing on with this effort (will just update this comment while I do this)...

Supported Characters

Service + - = . _ : / @
efs X X X X X X X X
s3 X X X X X X X X
redshift X X X X X X X X
elasticache X X X X X X X X
acm X X X X X X X X
kinesis X X X X X X X X
vpc X X X X X X X X

Length

Service Key Len Spec Key Len Actual Val Len Spec Val Len Actual
efs ? 128 ? 256
s3 128 128 256 256
redshift 128 128 256 256
elasticache 128 128 256 256
acm 127 128 255 256
kinesis 128 128 255 256
vpc ? 127 ? 255
@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Mar 24, 2017

Contributor

@radeksimko so this list isn't anywhere near comprehensive yet, but this does account for all the services I currently use and those are a good cross section of the top 30 amazon services (if this article is to be trusted) that are tagable.

It seems like there are generally 2 different modes for the functioning of the tags. One where there are 128 chars for labels and 256 for the value or 127 chars for the labels and 255 chars for the values. Supported special characters regardless of reporting in the docs seem to be standard across all services.

So going back to my original PR... I think we'd be better off having a simple consistent rule here (127 label len, 255 val len) if we can actually support that. I'd be curious about stats on the length of labels and values folks are using in practice. but that's probably something Amazon could answer better. It really would just be nicer if this were consistent across all services though. ;-)

Contributor

netjunki commented Mar 24, 2017

@radeksimko so this list isn't anywhere near comprehensive yet, but this does account for all the services I currently use and those are a good cross section of the top 30 amazon services (if this article is to be trusted) that are tagable.

It seems like there are generally 2 different modes for the functioning of the tags. One where there are 128 chars for labels and 256 for the value or 127 chars for the labels and 255 chars for the values. Supported special characters regardless of reporting in the docs seem to be standard across all services.

So going back to my original PR... I think we'd be better off having a simple consistent rule here (127 label len, 255 val len) if we can actually support that. I'd be curious about stats on the length of labels and values folks are using in practice. but that's probably something Amazon could answer better. It really would just be nicer if this were consistent across all services though. ;-)

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Mar 31, 2017

Contributor

@radeksimko encountered an error today that would have been caught by this and just spent several hours trying to track down the source of the issue. Today I got the following error:

18:00:02 * aws_elasticache_replication_group.elasticache: Error creating Elasticache Replication Group: InvalidParameterValue: Tagging not available right now. Please try your query without tags.
18:00:02 	status code: 400, request id: bf34b580-1665-11e7-9a51-a55f3efe8520

Based on that I assumed I was encountering some kind of issue with the aws tagging service. So after poking around I finally got in touch with amazon support. After being on with them and describing the problem and going through a pile of different debugging efforts I finally was pasting them some of the terraform output and noticed:

tags.team:                      "{var.rli_team_name}"

One of our tags didn't interpolate it's value correctly and the error above seems to be due to the fact that { and } are invalid characters in tags keys and values.

Anyway... another justification for getting something like this going in terraform. :-)

Contributor

netjunki commented Mar 31, 2017

@radeksimko encountered an error today that would have been caught by this and just spent several hours trying to track down the source of the issue. Today I got the following error:

18:00:02 * aws_elasticache_replication_group.elasticache: Error creating Elasticache Replication Group: InvalidParameterValue: Tagging not available right now. Please try your query without tags.
18:00:02 	status code: 400, request id: bf34b580-1665-11e7-9a51-a55f3efe8520

Based on that I assumed I was encountering some kind of issue with the aws tagging service. So after poking around I finally got in touch with amazon support. After being on with them and describing the problem and going through a pile of different debugging efforts I finally was pasting them some of the terraform output and noticed:

tags.team:                      "{var.rli_team_name}"

One of our tags didn't interpolate it's value correctly and the error above seems to be due to the fact that { and } are invalid characters in tags keys and values.

Anyway... another justification for getting something like this going in terraform. :-)

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart Apr 1, 2017

Contributor

Thanks for all that great research, @netjunki! I will let Radek continue working with you on this but I just wanted to say that this is awesome info and seems worth capturing somewhere more permanent than this PR...

Contributor

apparentlymart commented Apr 1, 2017

Thanks for all that great research, @netjunki! I will let Radek continue working with you on this but I just wanted to say that this is awesome info and seems worth capturing somewhere more permanent than this PR...

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Apr 1, 2017

Contributor

Maybe I'll dust off my blog. I haven't posted there in a while. :-) Updated: Lightly dusted.... The theme I'm using doesn't work with tables well... so if I want to make this work right I need to track down the original sources to my blog (they're someplace), pick a new theme or fix the one I'm working with, and regenerate the site.

Contributor

netjunki commented Apr 1, 2017

Maybe I'll dust off my blog. I haven't posted there in a while. :-) Updated: Lightly dusted.... The theme I'm using doesn't work with tables well... so if I want to make this work right I need to track down the original sources to my blog (they're someplace), pick a new theme or fix the one I'm working with, and regenerate the site.

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Apr 6, 2017

Contributor

@radeksimko I still think having just a single validation is the right way to go here. Should we try to add tests for the limits into all of the acceptance tests someplace (at least for all the taggable services)? I'm not against it, but I'd like some guidance about where to take this next.

Contributor

netjunki commented Apr 6, 2017

@radeksimko I still think having just a single validation is the right way to go here. Should we try to add tests for the limits into all of the acceptance tests someplace (at least for all the taggable services)? I'm not against it, but I'd like some guidance about where to take this next.

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Apr 6, 2017

Member

👋 @netjunki
I don't mind having a single validation function which is configurable in a way that puts the right restrictions in the right places - i.e. not too tight or not too relaxed. If you can figure out the common differences and how can we decouple these as arguments, then I'm 👌 with that.

On the length side it should be fairly easy, on the side of regexp validation we often prefer to present users friendly and helpful errors, i.e. not field_xyz doesn't match regexp ^[.-_a-z0-9]$ but rather field_xyz must contain alphanumeric characters, dots, hyphens or underscores. In case you find it too dificult - i.e. the restrictions are way too drifted from each other, then I guess we'll have to fall back to unfriendly messages 😞 .

User experience is important and can go above optimizations - i.e. we'd rather validate smaller/upper-case first to produce field_xyz must only contain lowercase characters rather than less helpful long regexp message. This however doesn't mean we produce only a single error message - the validation function can and should return all violations ([]error) if possible.

Tests are certainly welcomed and encouraged, although acceptance tests for all affected resources would be probably overkill. Links or explanations in comments to how/where did we find the restrictions are much more helpful, if not necessary.

I hope this didn't discourage you from working on this & I hope it's understandable. 🙈 😅

Member

radeksimko commented Apr 6, 2017

👋 @netjunki
I don't mind having a single validation function which is configurable in a way that puts the right restrictions in the right places - i.e. not too tight or not too relaxed. If you can figure out the common differences and how can we decouple these as arguments, then I'm 👌 with that.

On the length side it should be fairly easy, on the side of regexp validation we often prefer to present users friendly and helpful errors, i.e. not field_xyz doesn't match regexp ^[.-_a-z0-9]$ but rather field_xyz must contain alphanumeric characters, dots, hyphens or underscores. In case you find it too dificult - i.e. the restrictions are way too drifted from each other, then I guess we'll have to fall back to unfriendly messages 😞 .

User experience is important and can go above optimizations - i.e. we'd rather validate smaller/upper-case first to produce field_xyz must only contain lowercase characters rather than less helpful long regexp message. This however doesn't mean we produce only a single error message - the validation function can and should return all violations ([]error) if possible.

Tests are certainly welcomed and encouraged, although acceptance tests for all affected resources would be probably overkill. Links or explanations in comments to how/where did we find the restrictions are much more helpful, if not necessary.

I hope this didn't discourage you from working on this & I hope it's understandable. 🙈 😅

@netjunki

This comment has been minimized.

Show comment
Hide comment
@netjunki

netjunki Apr 6, 2017

Contributor

@radeksimko Nope no discouragement! Just was trying to figure out next steps for this. It really looks like the supported extra characters are limited to just those ones which are supported by that regex. That actually seems to be consistent regardless of what the docs say. The only part which varies seems to be the lengths (127 or 128 chars for the key, 255 or 256 chars for the value) but again only by one character and the docs aren't always correct on this either.

Guess I need to play around with the string length functions in go as well and confirm that the fields match up if you use unicode characters in your string. Because I'm now having this suspicion that the length might not be 255/256 unicode characters but 255/256 bytes... I'll test it out and verify.

Agree about changing the error message as well and I'll see what I can do about giving better detailed feedback. There's gotta be a way to find everything in a string which doesn't conform to the regex and then we could do some processing on that to give details or at least point the user in the right direction.

Cheers,
Ben

Contributor

netjunki commented Apr 6, 2017

@radeksimko Nope no discouragement! Just was trying to figure out next steps for this. It really looks like the supported extra characters are limited to just those ones which are supported by that regex. That actually seems to be consistent regardless of what the docs say. The only part which varies seems to be the lengths (127 or 128 chars for the key, 255 or 256 chars for the value) but again only by one character and the docs aren't always correct on this either.

Guess I need to play around with the string length functions in go as well and confirm that the fields match up if you use unicode characters in your string. Because I'm now having this suspicion that the length might not be 255/256 unicode characters but 255/256 bytes... I'll test it out and verify.

Agree about changing the error message as well and I'll see what I can do about giving better detailed feedback. There's gotta be a way to find everything in a string which doesn't conform to the regex and then we could do some processing on that to give details or at least point the user in the right direction.

Cheers,
Ben

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Apr 6, 2017

Member

There's gotta be a way to find everything in a string which doesn't conform to the regex and then we could do some processing on that to give details or at least point the user in the right direction.

It doesn't have to be too clever in the sense of figuring out what exactly is the extra/wrong/missing character - just separating out the common reasons - e.g. smaller/uppercase, static regexp with static err message and that's probably it... it's more about the message wording, i.e. not saying "you failed to comply with this complex regular expression" but explaining the regexp in plain simple english.

Member

radeksimko commented Apr 6, 2017

There's gotta be a way to find everything in a string which doesn't conform to the regex and then we could do some processing on that to give details or at least point the user in the right direction.

It doesn't have to be too clever in the sense of figuring out what exactly is the extra/wrong/missing character - just separating out the common reasons - e.g. smaller/uppercase, static regexp with static err message and that's probably it... it's more about the message wording, i.e. not saying "you failed to comply with this complex regular expression" but explaining the regexp in plain simple english.

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart Oct 31, 2017

Contributor

Hello @netjunki, and thanks for working on this!

As part of the the Terraform 0.10 release earlier this year, all of the Terraform providers were moved to their own repositories in the terraform-providers GitHub organization, and removed from the Terraform Core repository.

Unfortunately due to the fact that new issues and pull requests are being opened constantly, it was not possible for the various provider maintainers to merge all outstanding pull requests before this split, and there is no automatic way to migrate a pull request to a new repository.

As a result, this pull request can sadly no longer be applied as-is, and so I'm going to close it.

If you or someone else has the time and motivation to apply same changes (modulo the subsequent discussion) to the aws provider repository and open a new PR there, the maintainers of that provider should be able to review and merge it.

Thanks again for working on this, and sorry it was not able to be merged before the provider repository changes.

Contributor

apparentlymart commented Oct 31, 2017

Hello @netjunki, and thanks for working on this!

As part of the the Terraform 0.10 release earlier this year, all of the Terraform providers were moved to their own repositories in the terraform-providers GitHub organization, and removed from the Terraform Core repository.

Unfortunately due to the fact that new issues and pull requests are being opened constantly, it was not possible for the various provider maintainers to merge all outstanding pull requests before this split, and there is no automatic way to migrate a pull request to a new repository.

As a result, this pull request can sadly no longer be applied as-is, and so I'm going to close it.

If you or someone else has the time and motivation to apply same changes (modulo the subsequent discussion) to the aws provider repository and open a new PR there, the maintainers of that provider should be able to review and merge it.

Thanks again for working on this, and sorry it was not able to be merged before the provider repository changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment