From ef441e87047930e04d24646e0d89183b07552747 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 15 May 2020 15:45:49 -0700 Subject: [PATCH] Tye should validate that service names are valid DNS names (#480) --- .gitignore | 2 +- docs/reference/schema.md | 7 ++- samples/apps-with-ingress/tye.yaml | 3 +- .../ConfigModel/ConfigFactory.cs | 8 ++- .../ConfigModel/ConfigService.cs | 8 +++ .../Serialization/ConfigIngressParser.cs | 4 +- .../Serialization/ConfigServiceParser.cs | 2 +- test/E2ETest/TyeGenerateTests.cs | 14 ++--- test/E2ETest/TyeInitTests.cs | 15 +++++ test/E2ETest/TyeRunTests.cs | 8 +-- .../generate/apps-with-ingress.yaml | 60 +++++++++---------- test/E2ETest/testassets/generate/dapr.yaml | 22 +++---- .../init/console-normalization-svc-name.yaml | 10 ++++ .../Console.Normalization.svc.Name.csproj | 8 +++ .../Console.Normalization.svc.Name/Program.cs | 12 ++++ .../projects/apps-with-ingress/tye.yaml | 12 ++-- .../E2ETest/testassets/projects/dapr/tye.yaml | 2 +- test/Test.Infrastructure/TyeAssert.cs | 5 +- .../TyeDeserializationValidationTests.cs | 37 +++++++++++- 19 files changed, 168 insertions(+), 71 deletions(-) create mode 100644 test/E2ETest/testassets/init/console-normalization-svc-name.yaml create mode 100644 test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Console.Normalization.svc.Name.csproj create mode 100644 test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Program.cs diff --git a/.gitignore b/.gitignore index 10fd5d99..5931d2ed 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,7 @@ .dotnet/ artifacts/ .tye/ - +**/.vscode/ ## Ignore Visual Studio temporary files, build results, and ## files generated by popular Visual Studio add-ons. diff --git a/docs/reference/schema.md b/docs/reference/schema.md index a6f8e1fb..ba832733 100644 --- a/docs/reference/schema.md +++ b/docs/reference/schema.md @@ -133,7 +133,12 @@ services: #### `name` (string) *required* -The service name. Each service must have a name, and it must be a legal DNS name: (`a-z` + `_`). +The service name. Each service must have a name, and it must be a legal DNS name: (`a-z` + `-`). Specifically, the service name must: + +- Contain at most 63 characters +- Contain only alphanumeric characters or ‘-’ +- Start with an alphanumeric character +- End with an alphanumeric character #### `project` (string) diff --git a/samples/apps-with-ingress/tye.yaml b/samples/apps-with-ingress/tye.yaml index c6aa8cca..4fbd7178 100644 --- a/samples/apps-with-ingress/tye.yaml +++ b/samples/apps-with-ingress/tye.yaml @@ -17,8 +17,7 @@ ingress: - host: a.example.com service: app-a - host: b.example.com - service: app-b - + service: app-b services: - name: app-a project: ApplicationA/ApplicationA.csproj diff --git a/src/Microsoft.Tye.Core/ConfigModel/ConfigFactory.cs b/src/Microsoft.Tye.Core/ConfigModel/ConfigFactory.cs index 8f4342ee..8ba8f65c 100644 --- a/src/Microsoft.Tye.Core/ConfigModel/ConfigFactory.cs +++ b/src/Microsoft.Tye.Core/ConfigModel/ConfigFactory.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Text.RegularExpressions; using Microsoft.Tye.Serialization; using Tye.Serialization; @@ -42,7 +43,7 @@ namespace Microsoft.Tye.ConfigModel var service = new ConfigService() { - Name = Path.GetFileNameWithoutExtension(file.Name).ToLowerInvariant(), + Name = NormalizeServiceName(Path.GetFileNameWithoutExtension(file.Name)), Project = file.FullName.Replace('\\', '/'), }; @@ -73,7 +74,7 @@ namespace Microsoft.Tye.ConfigModel { var service = new ConfigService() { - Name = Path.GetFileNameWithoutExtension(projectFile.Name).ToLowerInvariant(), + Name = NormalizeServiceName(Path.GetFileNameWithoutExtension(projectFile.Name)), Project = projectFile.FullName.Replace('\\', '/'), }; @@ -97,5 +98,8 @@ namespace Microsoft.Tye.ConfigModel using var parser = new YamlParser(file); return parser.ParseConfigApplication(); } + + private static string NormalizeServiceName(string name) + => Regex.Replace(name.ToLowerInvariant(), "[^0-9A-Za-z-]+", "-"); } } diff --git a/src/Microsoft.Tye.Core/ConfigModel/ConfigService.cs b/src/Microsoft.Tye.Core/ConfigModel/ConfigService.cs index 3a22896d..c9972f40 100644 --- a/src/Microsoft.Tye.Core/ConfigModel/ConfigService.cs +++ b/src/Microsoft.Tye.Core/ConfigModel/ConfigService.cs @@ -4,13 +4,21 @@ using System.Collections.Generic; using System.ComponentModel.DataAnnotations; +using Tye; using YamlDotNet.Serialization; namespace Microsoft.Tye.ConfigModel { public class ConfigService { + const string ErrorMessage = "A service name must consist of lower case alphanumeric characters or '-'," + + " start with an alphabetic character, and end with an alphanumeric character" + + " (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')."; + const string MaxLengthErrorMessage = "Name cannot be more that 63 characters long."; + [Required] + [RegularExpression("[a-z]([-a-z0-9]*[a-z0-9])?", ErrorMessage = ErrorMessage)] + [MaxLength(63, ErrorMessage = MaxLengthErrorMessage)] public string Name { get; set; } = default!; public bool External { get; set; } public string? Image { get; set; } diff --git a/src/Microsoft.Tye.Core/Serialization/ConfigIngressParser.cs b/src/Microsoft.Tye.Core/Serialization/ConfigIngressParser.cs index 6ccc391c..da40d87d 100644 --- a/src/Microsoft.Tye.Core/Serialization/ConfigIngressParser.cs +++ b/src/Microsoft.Tye.Core/Serialization/ConfigIngressParser.cs @@ -30,7 +30,7 @@ namespace Tye.Serialization switch (key) { case "name": - configIngress.Name = YamlParser.GetScalarValue(key, child.Value); + configIngress.Name = YamlParser.GetScalarValue(key, child.Value).ToLowerInvariant(); break; case "replicas": if (!int.TryParse(YamlParser.GetScalarValue(key, child.Value), out var replicas)) @@ -91,7 +91,7 @@ namespace Tye.Serialization rule.Path = YamlParser.GetScalarValue(key, child.Value); break; case "service": - rule.Service = YamlParser.GetScalarValue(key, child.Value); + rule.Service = YamlParser.GetScalarValue(key, child.Value).ToLowerInvariant(); break; default: throw new TyeYamlException(child.Key.Start, CoreStrings.FormatUnrecognizedKey(key)); diff --git a/src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs b/src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs index d5e332b7..f9c84534 100644 --- a/src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs +++ b/src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs @@ -30,7 +30,7 @@ namespace Tye.Serialization switch (key) { case "name": - service.Name = YamlParser.GetScalarValue(key, child.Value); + service.Name = YamlParser.GetScalarValue(key, child.Value).ToLowerInvariant(); break; case "external": if (!bool.TryParse(YamlParser.GetScalarValue(key, child.Value), out var external)) diff --git a/test/E2ETest/TyeGenerateTests.cs b/test/E2ETest/TyeGenerateTests.cs index 03973904..6f6f1b96 100644 --- a/test/E2ETest/TyeGenerateTests.cs +++ b/test/E2ETest/TyeGenerateTests.cs @@ -183,7 +183,7 @@ namespace E2ETest public async Task Generate_DaprApplication() { var applicationName = "dapr_test_application"; - var projectName = "dapr_test_project"; + var projectName = "dapr-test-project"; var environment = "production"; await DockerAssert.DeleteDockerImagesAsync(output, projectName); @@ -363,8 +363,8 @@ namespace E2ETest var applicationName = "apps-with-ingress"; var environment = "production"; - await DockerAssert.DeleteDockerImagesAsync(output, "app-a"); - await DockerAssert.DeleteDockerImagesAsync(output, "app-b"); + await DockerAssert.DeleteDockerImagesAsync(output, "appa"); + await DockerAssert.DeleteDockerImagesAsync(output, "appa"); using var projectDirectory = TestHelpers.CopyTestProjectDirectory(applicationName); @@ -383,13 +383,13 @@ namespace E2ETest YamlAssert.Equals(expectedContent, content, output); - await DockerAssert.AssertImageExistsAsync(output, "app-a"); - await DockerAssert.AssertImageExistsAsync(output, "app-b"); + await DockerAssert.AssertImageExistsAsync(output, "appa"); + await DockerAssert.AssertImageExistsAsync(output, "appb"); } finally { - await DockerAssert.DeleteDockerImagesAsync(output, "app-a"); - await DockerAssert.DeleteDockerImagesAsync(output, "app-b"); + await DockerAssert.DeleteDockerImagesAsync(output, "appa"); + await DockerAssert.DeleteDockerImagesAsync(output, "appb"); } } } diff --git a/test/E2ETest/TyeInitTests.cs b/test/E2ETest/TyeInitTests.cs index 7b111f80..bb592c9e 100644 --- a/test/E2ETest/TyeInitTests.cs +++ b/test/E2ETest/TyeInitTests.cs @@ -74,5 +74,20 @@ namespace E2ETest YamlAssert.Equals(expectedContent, content); } + + + [Fact] + public void Console_Normalization_Service_Name() + { + using var projectDirectory = CopyTestProjectDirectory("Console.Normalization.svc.Name"); + + var projectFile = new FileInfo(Path.Combine(projectDirectory.DirectoryPath, "Console.Normalization.svc.Name.csproj")); + + var (content, _) = InitHost.CreateTyeFileContent(projectFile, force: false); + + var expectedContent = File.ReadAllText("testassets/init/console-normalization-svc-name.yaml"); + + YamlAssert.Equals(expectedContent, content); + } } } diff --git a/test/E2ETest/TyeRunTests.cs b/test/E2ETest/TyeRunTests.cs index 0614c2be..a7dd8a8d 100644 --- a/test/E2ETest/TyeRunTests.cs +++ b/test/E2ETest/TyeRunTests.cs @@ -489,8 +489,8 @@ namespace E2ETest await RunHostingApplication(application, new HostOptions(), async (app, uri) => { var ingressUri = await GetServiceUrl(client, uri, "ingress"); - var appAUri = await GetServiceUrl(client, uri, "app-a"); - var appBUri = await GetServiceUrl(client, uri, "app-b"); + var appAUri = await GetServiceUrl(client, uri, "appa"); + var appBUri = await GetServiceUrl(client, uri, "appb"); var appAResponse = await client.GetAsync(appAUri); var appBResponse = await client.GetAsync(appBUri); @@ -538,8 +538,8 @@ namespace E2ETest await RunHostingApplication(application, new HostOptions(), async (app, uri) => { var nginxUri = await GetServiceUrl(client, uri, "nginx"); - var appAUri = await GetServiceUrl(client, uri, "appA"); - var appBUri = await GetServiceUrl(client, uri, "appB"); + var appAUri = await GetServiceUrl(client, uri, "appa"); + var appBUri = await GetServiceUrl(client, uri, "appb"); var nginxResponse = await client.GetAsync(nginxUri); var appAResponse = await client.GetAsync(appAUri); diff --git a/test/E2ETest/testassets/generate/apps-with-ingress.yaml b/test/E2ETest/testassets/generate/apps-with-ingress.yaml index 0c707c35..c01452ba 100644 --- a/test/E2ETest/testassets/generate/apps-with-ingress.yaml +++ b/test/E2ETest/testassets/generate/apps-with-ingress.yaml @@ -1,36 +1,36 @@ kind: Deployment apiVersion: apps/v1 metadata: - name: app-a + name: appa labels: - app.kubernetes.io/name: 'app-a' + app.kubernetes.io/name: 'appa' app.kubernetes.io/part-of: 'apps-with-ingress' spec: replicas: 2 selector: matchLabels: - app.kubernetes.io/name: app-a + app.kubernetes.io/name: appa template: metadata: labels: - app.kubernetes.io/name: 'app-a' + app.kubernetes.io/name: 'appa' app.kubernetes.io/part-of: 'apps-with-ingress' spec: containers: - - name: app-a - image: app-a:1.0.0 + - name: appa + image: appa:1.0.0 imagePullPolicy: Always env: - name: ASPNETCORE_URLS value: 'http://*' - name: PORT value: '80' - - name: SERVICE__APP-B__PROTOCOL + - name: SERVICE__APPB__PROTOCOL value: 'http' - - name: SERVICE__APP-B__PORT + - name: SERVICE__APPB__PORT value: '80' - - name: SERVICE__APP-B__HOST - value: 'app-b' + - name: SERVICE__APPB__HOST + value: 'appb' ports: - containerPort: 80 ... @@ -38,13 +38,13 @@ spec: kind: Service apiVersion: v1 metadata: - name: app-a + name: appa labels: - app.kubernetes.io/name: 'app-a' + app.kubernetes.io/name: 'appa' app.kubernetes.io/part-of: 'apps-with-ingress' spec: selector: - app.kubernetes.io/name: app-a + app.kubernetes.io/name: appa type: ClusterIP ports: - name: http @@ -56,36 +56,36 @@ spec: kind: Deployment apiVersion: apps/v1 metadata: - name: app-b + name: appb labels: - app.kubernetes.io/name: 'app-b' + app.kubernetes.io/name: 'appb' app.kubernetes.io/part-of: 'apps-with-ingress' spec: replicas: 2 selector: matchLabels: - app.kubernetes.io/name: app-b + app.kubernetes.io/name: appb template: metadata: labels: - app.kubernetes.io/name: 'app-b' + app.kubernetes.io/name: 'appb' app.kubernetes.io/part-of: 'apps-with-ingress' spec: containers: - - name: app-b - image: app-b:1.0.0 + - name: appb + image: appb:1.0.0 imagePullPolicy: Always env: - name: ASPNETCORE_URLS value: 'http://*' - name: PORT value: '80' - - name: SERVICE__APP-A__PROTOCOL + - name: SERVICE__APPA__PROTOCOL value: 'http' - - name: SERVICE__APP-A__PORT + - name: SERVICE__APPA__PORT value: '80' - - name: SERVICE__APP-A__HOST - value: 'app-a' + - name: SERVICE__APPA__HOST + value: 'appa' ports: - containerPort: 80 ... @@ -93,13 +93,13 @@ spec: kind: Service apiVersion: v1 metadata: - name: app-b + name: appb labels: - app.kubernetes.io/name: 'app-b' + app.kubernetes.io/name: 'appb' app.kubernetes.io/part-of: 'apps-with-ingress' spec: selector: - app.kubernetes.io/name: app-b + app.kubernetes.io/name: appb type: ClusterIP ports: - name: http @@ -122,25 +122,25 @@ spec: - http: paths: - backend: - serviceName: app-a + serviceName: appa servicePort: 80 path: /A(/|$)(.*) - backend: - serviceName: app-b + serviceName: appb servicePort: 80 path: /B(/|$)(.*) - host: a.example.com http: paths: - backend: - serviceName: app-a + serviceName: appa servicePort: 80 path: /()(.*) - host: b.example.com http: paths: - backend: - serviceName: app-b + serviceName: appb servicePort: 80 path: /()(.*) ... \ No newline at end of file diff --git a/test/E2ETest/testassets/generate/dapr.yaml b/test/E2ETest/testassets/generate/dapr.yaml index 2b1936ef..bf175b4a 100644 --- a/test/E2ETest/testassets/generate/dapr.yaml +++ b/test/E2ETest/testassets/generate/dapr.yaml @@ -1,36 +1,36 @@ kind: Deployment apiVersion: apps/v1 metadata: - name: dapr_test_project + name: dapr-test-project annotations: dapr.io/enabled: 'true' - dapr.io/id: 'dapr_test_project' + dapr.io/id: 'dapr-test-project' dapr.io/port: '80' dapr.io/config: 'tracing' dapr.io/log-level: 'debug' labels: - app.kubernetes.io/name: 'dapr_test_project' + app.kubernetes.io/name: 'dapr-test-project' app.kubernetes.io/part-of: 'dapr_test_application' spec: replicas: 1 selector: matchLabels: - app.kubernetes.io/name: dapr_test_project + app.kubernetes.io/name: dapr-test-project template: metadata: annotations: dapr.io/enabled: 'true' - dapr.io/id: 'dapr_test_project' + dapr.io/id: 'dapr-test-project' dapr.io/port: '80' dapr.io/config: 'tracing' dapr.io/log-level: 'debug' labels: - app.kubernetes.io/name: 'dapr_test_project' + app.kubernetes.io/name: 'dapr-test-project' app.kubernetes.io/part-of: 'dapr_test_application' spec: containers: - - name: dapr_test_project - image: dapr_test_project:1.0.0 + - name: dapr-test-project + image: dapr-test-project:1.0.0 imagePullPolicy: Always env: - name: ASPNETCORE_URLS @@ -44,13 +44,13 @@ spec: kind: Service apiVersion: v1 metadata: - name: dapr_test_project + name: dapr-test-project labels: - app.kubernetes.io/name: 'dapr_test_project' + app.kubernetes.io/name: 'dapr-test-project' app.kubernetes.io/part-of: 'dapr_test_application' spec: selector: - app.kubernetes.io/name: dapr_test_project + app.kubernetes.io/name: dapr-test-project type: ClusterIP ports: - name: http diff --git a/test/E2ETest/testassets/init/console-normalization-svc-name.yaml b/test/E2ETest/testassets/init/console-normalization-svc-name.yaml new file mode 100644 index 00000000..23e7bf8e --- /dev/null +++ b/test/E2ETest/testassets/init/console-normalization-svc-name.yaml @@ -0,0 +1,10 @@ +# tye application configuration file +# read all about it at https://github.com/dotnet/tye +# +# when you've given us a try, we'd love to know what you think: +# https://aka.ms/AA7q20u +# +name: console.normalization.svc.name +services: +- name: console-normalization-svc-name + project: Console.Normalization.svc.Name.csproj diff --git a/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Console.Normalization.svc.Name.csproj b/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Console.Normalization.svc.Name.csproj new file mode 100644 index 00000000..c73e0d16 --- /dev/null +++ b/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Console.Normalization.svc.Name.csproj @@ -0,0 +1,8 @@ + + + + Exe + netcoreapp3.1 + + + diff --git a/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Program.cs b/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Program.cs new file mode 100644 index 00000000..92c4b591 --- /dev/null +++ b/test/E2ETest/testassets/projects/Console.Normalization.svc.Name/Program.cs @@ -0,0 +1,12 @@ +using System; + +namespace Console.Normalization.svc.Name +{ + class Program + { + static void Main(string[] args) + { + Console.WriteLine("Hello World!"); + } + } +} diff --git a/test/E2ETest/testassets/projects/apps-with-ingress/tye.yaml b/test/E2ETest/testassets/projects/apps-with-ingress/tye.yaml index c6aa8cca..304a2177 100644 --- a/test/E2ETest/testassets/projects/apps-with-ingress/tye.yaml +++ b/test/E2ETest/testassets/projects/apps-with-ingress/tye.yaml @@ -11,18 +11,18 @@ ingress: - port: 8080 rules: - path: /A - service: app-a + service: appA - path: /B - service: app-b + service: appB - host: a.example.com - service: app-a + service: appA - host: b.example.com - service: app-b + service: appB services: -- name: app-a +- name: appA project: ApplicationA/ApplicationA.csproj replicas: 2 -- name: app-b +- name: appB project: ApplicationB/ApplicationB.csproj replicas: 2 diff --git a/test/E2ETest/testassets/projects/dapr/tye.yaml b/test/E2ETest/testassets/projects/dapr/tye.yaml index c292e8c1..a940411e 100644 --- a/test/E2ETest/testassets/projects/dapr/tye.yaml +++ b/test/E2ETest/testassets/projects/dapr/tye.yaml @@ -4,5 +4,5 @@ extensions: config: tracing log-level: debug services: -- name: dapr_test_project +- name: dapr-test-project project: dapr.csproj \ No newline at end of file diff --git a/test/Test.Infrastructure/TyeAssert.cs b/test/Test.Infrastructure/TyeAssert.cs index e9ca4470..36c4644b 100644 --- a/test/Test.Infrastructure/TyeAssert.cs +++ b/test/Test.Infrastructure/TyeAssert.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Linq; using Microsoft.Tye.ConfigModel; using Xunit; @@ -29,7 +30,7 @@ namespace Test.Infrastructure { var otherRule = otherIngress .Rules - .Where(o => o.Path == rule.Path && o.Host == rule.Host && o.Service == rule.Service) + .Where(o => o.Path == rule.Path && o.Host == rule.Host && o.Service?.Equals(rule.Service, StringComparison.OrdinalIgnoreCase) == true) .Single(); Assert.NotNull(otherRule); } @@ -49,7 +50,7 @@ namespace Test.Infrastructure { var otherService = expected .Services - .Where(o => o.Name == service.Name) + .Where(o => o.Name.Equals(service.Name, StringComparison.OrdinalIgnoreCase)) .Single(); Assert.NotNull(otherService); Assert.Equal(otherService.Args, service.Args); diff --git a/test/UnitTests/TyeDeserializationValidationTests.cs b/test/UnitTests/TyeDeserializationValidationTests.cs index 58cae2f5..e74f5fa5 100644 --- a/test/UnitTests/TyeDeserializationValidationTests.cs +++ b/test/UnitTests/TyeDeserializationValidationTests.cs @@ -201,10 +201,45 @@ services: - port: 8080 protocol: http"; + using var parser = new YamlParser(input); + var app = parser.ParseConfigApplication(); + Assert.Throws(() => app.Validate()); + } + + [Fact] + public void ValidateServiceNameThrowsException() + { + var input = @" +services: + - name: app_ + bindings: + - protocol: http + name: a + - protocol: https + name: b"; + var errorMessage = "A service name must consist of lower case alphanumeric"; using var parser = new YamlParser(input); var app = parser.ParseConfigApplication(); var exception = Assert.Throws(() => app.Validate()); - Assert.Contains(CoreStrings.FormatProjectImageExecutableExclusive(a, b), exception.Message); + Assert.Contains(errorMessage, exception.Message); + } + + [Fact] + public void ValidateServiceNameThrowsExceptionForMaxLength() + { + var input = @" +services: + - name: appavalidateservicenamethrowsexceptionformaxlengthvalidateservicen + bindings: + - protocol: http + name: a + - protocol: https + name: b"; + var errorMessage = "Name cannot be more that 63 characters long."; + using var parser = new YamlParser(input); + var app = parser.ParseConfigApplication(); + var exception = Assert.Throws(() => app.Validate()); + Assert.Contains(errorMessage, exception.Message); } } }