Skip to content

Commit

Permalink
Support args on project.v0 and dockerfile.v0 resources (#3720)
Browse files Browse the repository at this point in the history
We allow binding expressions to refer to properties of other
resources, but we fail if any of these values are secrets (since ACA
has no way to treat these as ACA or KeyVault secrets and reference
them). If we detect this case, we fail and encourage the user to pass
these values via environment variaibles.

Fixes #3678
  • Loading branch information
ellismg authored Apr 23, 2024
1 parent 16b73bf commit c836afc
Show file tree
Hide file tree
Showing 13 changed files with 315 additions and 153 deletions.
96 changes: 74 additions & 22 deletions cli/azd/pkg/apphost/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ func Dockerfiles(manifest *Manifest) map[string]genDockerfile {
Env: comp.Env,
Bindings: comp.Bindings,
BuildArgs: comp.BuildArgs,
Args: comp.Args,
}
}
}
Expand Down Expand Up @@ -451,7 +452,7 @@ func (b *infraGenerator) LoadManifest(m *Manifest) error {
case "azure.appinsights.v0":
b.addAppInsights(name)
case "project.v0":
b.addProject(name, *comp.Path, comp.Env, comp.Bindings)
b.addProject(name, *comp.Path, comp.Env, comp.Bindings, comp.Args)
case "container.v0":
b.addContainer(name, *comp.Image, comp.Env, comp.Bindings, comp.Inputs, comp.Volumes)
case "dapr.v0":
Expand All @@ -465,7 +466,7 @@ func (b *infraGenerator) LoadManifest(m *Manifest) error {
return err
}
case "dockerfile.v0":
b.addDockerfile(name, *comp.Path, *comp.Context, comp.Env, comp.Bindings, comp.BuildArgs)
b.addDockerfile(name, *comp.Path, *comp.Context, comp.Env, comp.Bindings, comp.BuildArgs, comp.Args)
case "redis.v0":
b.addContainerAppService(name, RedisContainerAppService)
case "azure.keyvault.v0":
Expand Down Expand Up @@ -755,7 +756,7 @@ func (b *infraGenerator) addSqlDatabase(sqlAccount, dbName string) {
}

func (b *infraGenerator) addProject(
name string, path string, env map[string]string, bindings custommaps.WithOrder[Binding],
name string, path string, env map[string]string, bindings custommaps.WithOrder[Binding], args []string,
) {
b.requireCluster()
b.requireContainerRegistry()
Expand All @@ -764,6 +765,7 @@ func (b *infraGenerator) addProject(
Path: path,
Env: env,
Bindings: bindings,
Args: args,
}
}

Expand Down Expand Up @@ -932,6 +934,7 @@ func (b *infraGenerator) addDaprStateStoreComponent(name string) {
func (b *infraGenerator) addDockerfile(
name string, path string, context string, env map[string]string,
bindings custommaps.WithOrder[Binding], buildArgs map[string]string,
args []string,
) {
b.requireCluster()
b.requireContainerRegistry()
Expand All @@ -942,6 +945,7 @@ func (b *infraGenerator) addDockerfile(
Env: env,
Bindings: bindings,
BuildArgs: buildArgs,
Args: args,
}
}

Expand Down Expand Up @@ -1063,6 +1067,10 @@ func (b *infraGenerator) Compile() error {
return fmt.Errorf("configuring environment for resource %s: %w", resourceName, err)
}

if err := b.buildArgsBlock(docker.Args, &projectTemplateCtx); err != nil {
return err
}

b.containerAppTemplateContexts[resourceName] = projectTemplateCtx
}

Expand Down Expand Up @@ -1101,6 +1109,10 @@ func (b *infraGenerator) Compile() error {
return err
}

if err := b.buildArgsBlock(project.Args, &projectTemplateCtx); err != nil {
return err
}

b.containerAppTemplateContexts[resourceName] = projectTemplateCtx
}

Expand Down Expand Up @@ -1471,6 +1483,64 @@ func urlPortFromTargetPort(binding *Binding, bindingMappedToMainIngress bool) (s
return acaTemplatedTargetPort, nil
}

// asYamlString converts a string to the YAML representation of the string, ensuring that it is quoted and escaped as needed.
func asYamlString(s string) (string, error) {
// We want to ensure that we render these values in the YAML as strings. If `res` was the string "true"
// (without the quotes), we would naturally create a value directive in yaml that looks like this:
//
// - name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES
// value: true
//
// And YAML rules would treat the above as the value being a boolean instead of a string, which the container
// app service expects.
//
// YAML marshalling the string value will give us something like `"true"` (with the quotes, and any escaping
// that needs to be done), which is what we want here.
// Do not use JSON marshall as it would escape the quotes within the string, breaking the meaning of the value.
// yaml marshall will use 'some text "quoted" more text' as a valid yaml string.
yamlString, err := yaml.Marshal(s)
if err != nil {
return "", fmt.Errorf("marshalling env value: %w", err)
}

// remove the trailing newline. yaml marshall will add a newline at the end of the string, as the new line is
// expected at the end of the yaml document. But we are getting a single value with valid yaml here, so we don't
// need the newline
return string(yamlString[0 : len(yamlString)-1]), nil
}

func (b *infraGenerator) buildArgsBlock(args []string, manifestCtx *genContainerAppManifestTemplateContext) error {
for argN, arg := range args {
resolvedArg, err := EvalString(arg, func(s string) (string, error) { return b.evalBindingRef(s, inputEmitTypeYaml) })
if err != nil {
return fmt.Errorf("evaluating value for argument %d: %w", argN, err)
}

// Unlike environment variables, ACA doesn't provide a way to pass secret values without baking them into the args
// array directly. We don't want folks to accidentally bake the plaintext value of these secrets into the container
// definition, so for now, we block this.
//
// This logic is similar to what we do in buildEnvBlock to detect when we need to take values and treat them as ACA
// secrets.
if strings.Contains(arg, ".connectionString}") ||
strings.Contains(resolvedArg, "{{ connectionString") ||
strings.Contains(resolvedArg, "{{ securedParameter ") ||
strings.Contains(resolvedArg, "{{ secretOutput ") {

return fmt.Errorf("argument %d cannot contain connection strings, secured parameters, or secret outputs. Use "+
"environment variables instead", argN)
}

yamlString, err := asYamlString(resolvedArg)
if err != nil {
return fmt.Errorf("marshalling arg value: %w", err)
}
manifestCtx.Args = append(manifestCtx.Args, yamlString)
}

return nil
}

// buildEnvBlock creates the environment map in the template context. It does this by copying the values from the given map,
// evaluating any binding expressions that are present. It writes the result of the evaluation after calling json.Marshal
// so the values may be emitted into YAML as is without worrying about escaping.
Expand All @@ -1481,29 +1551,11 @@ func (b *infraGenerator) buildEnvBlock(env map[string]string, manifestCtx *genCo
return fmt.Errorf("evaluating value for %s: %w", k, err)
}

// We want to ensure that we render these values in the YAML as strings. If `res` was the string "true"
// (without the quotes), we would naturally create a value directive in yaml that looks like this:
//
// - name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES
// value: true
//
// And YAML rules would treat the above as the value being a boolean instead of a string, which the container
// app service expects.
//
// YAML marshalling the string value will give us something like `"true"` (with the quotes, and any escaping
// that needs to be done), which is what we want here.
// Do not use JSON marshall as it would escape the quotes within the string, breaking the meaning of the value.
// yaml marshall will use 'some text "quoted" more text' as a valid yaml string.
yamlString, err := yaml.Marshal(res)
resolvedValue, err := asYamlString(res)
if err != nil {
return fmt.Errorf("marshalling env value: %w", err)
}

// remove the trailing newline. yaml marshall will add a newline at the end of the string, as the new line is
// expected at the end of the yaml document. But we are getting a single value with valid yaml here, so we don't
// need the newline
resolvedValue := string(yamlString[0 : len(yamlString)-1])

// connectionString detection, either of:
// a) explicit connection string key for env, like "ConnectionStrings__resource": "XXXXX"
// b) a connection string field references in the value, like "FOO": "{resource.connectionString}"
Expand Down
22 changes: 22 additions & 0 deletions cli/azd/pkg/apphost/generate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ var aspireDockerManifest []byte
//go:embed testdata/aspire-storage.json
var aspireStorageManifest []byte

//go:embed testdata/aspire-args.json
var aspireArgsManifest []byte

//go:embed testdata/aspire-bicep.json
var aspireBicepManifest []byte

Expand Down Expand Up @@ -212,6 +215,25 @@ func TestAspireDockerGeneration(t *testing.T) {
require.NoError(t, err)
}

func TestAspireArgsGeneration(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Skipping due to EOL issues on Windows with the baselines")
}

ctx := context.Background()
mockCtx := mocks.NewMockContext(ctx)
mockPublishManifest(mockCtx, aspireArgsManifest, nil)
mockCli := dotnet.NewDotNetCli(mockCtx.CommandRunner)

m, err := ManifestFromAppHost(ctx, filepath.Join("testdata", "AspireArgs.AppHost.csproj"), mockCli, "")
require.NoError(t, err)

manifest, err := ContainerAppManifestTemplateForProject(m, "apiservice", false)
require.NoError(t, err)

snapshot.SnapshotT(t, manifest)
}

func TestAspireContainerGeneration(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Skipping due to EOL issues on Windows with the baselines")
Expand Down
3 changes: 3 additions & 0 deletions cli/azd/pkg/apphost/generate_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ type genDockerfile struct {
Env map[string]string
Bindings custommaps.WithOrder[Binding]
BuildArgs map[string]string
Args []string
}

type genProject struct {
Path string
Env map[string]string
Args []string
Bindings custommaps.WithOrder[Binding]
}

Expand Down Expand Up @@ -154,6 +156,7 @@ type genContainerAppManifestTemplateContext struct {
KeyVaultSecrets map[string]string
Dapr *genContainerAppManifestTemplateContextDapr
AutoConfigureDataProtection bool
Args []string
}

type genProjectFileContext struct {
Expand Down
3 changes: 3 additions & 0 deletions cli/azd/pkg/apphost/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ type Resource struct {
// BuildArgs is present on a dockerfile.v0 resource and is the --build-arg for building the docker image.
BuildArgs map[string]string `json:"buildArgs,omitempty"`

// Args is optionally present on project.v0 and dockerfile.v0 resources and are the arguments to pass to the container.
Args []string `json:"args,omitempty"`

// Parent is present on a resource which is a child of another. It is the name of the parent resource. For example, a
// postgres.database.v0 is a child of a postgres.server.v0, and so it would have a parent of which is the name of
// the server resource.
Expand Down
44 changes: 44 additions & 0 deletions cli/azd/pkg/apphost/testdata/TestAspireArgsGeneration.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
location: {{ .Env.AZURE_LOCATION }}
identity:
type: UserAssigned
userAssignedIdentities:
? "{{ .Env.AZURE_CONTAINER_REGISTRY_MANAGED_IDENTITY_ID }}"
: {}
properties:
environmentId: {{ .Env.AZURE_CONTAINER_APPS_ENVIRONMENT_ID }}
configuration:
activeRevisionsMode: single
ingress:
external: false
targetPort: {{ targetPortOrDefault 8080 }}
transport: http
allowInsecure: true
registries:
- server: {{ .Env.AZURE_CONTAINER_REGISTRY_ENDPOINT }}
identity: {{ .Env.AZURE_CONTAINER_REGISTRY_MANAGED_IDENTITY_ID }}
template:
containers:
- image: {{ .Image }}
name: apiservice
args:
- --supports-args
- "true"
- --port
- "12345"
env:
- name: AZURE_CLIENT_ID
value: {{ .Env.MANAGED_IDENTITY_CLIENT_ID }}
- name: ASPNETCORE_FORWARDEDHEADERS_ENABLED
value: "true"
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES
value: "true"
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES
value: "true"
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_RETRY
value: in_memory
scale:
minReplicas: 1
tags:
azd-service-name: apiservice
aspire-resource-name: apiservice

Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ properties:
transport: http
allowInsecure: true
registries:
- server: {{ .Env.AZURE_CONTAINER_REGISTRY_ENDPOINT }}
identity: {{ .Env.AZURE_CONTAINER_REGISTRY_MANAGED_IDENTITY_ID }}
- server: {{ .Env.AZURE_CONTAINER_REGISTRY_ENDPOINT }}
identity: {{ .Env.AZURE_CONTAINER_REGISTRY_MANAGED_IDENTITY_ID }}
secrets:
- name: applicationinsights-connection-string
value: '{{ .Env.AI_APPINSIGHTSCONNECTIONSTRING }}'
Expand All @@ -27,27 +27,27 @@ properties:
value: '{{ .Env.S_B_SERVICEBUSENDPOINT }}'
template:
containers:
- image: {{ .Image }}
name: frontend
env:
- name: AZURE_CLIENT_ID
value: {{ .Env.MANAGED_IDENTITY_CLIENT_ID }}
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES
value: "true"
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES
value: "true"
- name: bicepValue0
value: '{{ .Env.TEST_VAL0 }}'
- name: bicepValue_test
value: '{{ .Env.TEST_TEST }}'
- name: APPLICATIONINSIGHTS_CONNECTION_STRING
secretRef: applicationinsights-connection-string
- name: ConnectionStrings__db
secretRef: connectionstrings--db
- name: ConnectionStrings__db2
secretRef: connectionstrings--db2
- name: ConnectionStrings__s-b
secretRef: connectionstrings--s-b
- image: {{ .Image }}
name: frontend
env:
- name: AZURE_CLIENT_ID
value: {{ .Env.MANAGED_IDENTITY_CLIENT_ID }}
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES
value: "true"
- name: OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES
value: "true"
- name: bicepValue0
value: '{{ .Env.TEST_VAL0 }}'
- name: bicepValue_test
value: '{{ .Env.TEST_TEST }}'
- name: APPLICATIONINSIGHTS_CONNECTION_STRING
secretRef: applicationinsights-connection-string
- name: ConnectionStrings__db
secretRef: connectionstrings--db
- name: ConnectionStrings__db2
secretRef: connectionstrings--db2
- name: ConnectionStrings__s-b
secretRef: connectionstrings--s-b
scale:
minReplicas: 1
tags:
Expand Down
Loading

0 comments on commit c836afc

Please sign in to comment.