Add profile configuration to bundle endpoint #4476
Conversation
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
MarcosDY
requested review from
evan2645,
amartinezfayo,
azdagron and
rturner3
as code owners
| return nil, errors.New("bundle endpoint 'acme' or 'profile' can be set") | ||
|
|
||
| case config.ACME != nil: | ||
| return configToACMEConfig(config.ACME, dataDir), nil |
There was a problem hiding this comment.
We should log a warning when we do this because config will need to be updated before 1.9, similar to profile nil case. We should probably mention something about ACME configuration without defining it as part of the https_web profile is deprecated
cmd/spire-server/cli/run/run.go
Outdated
| return configToACMEConfig(config.ACME, dataDir), nil | ||
|
|
||
| case config.Profile == nil: | ||
| log.Warn("bundle endpoint has no profile set, using https_spiffe as default, please choose a profile.") |
There was a problem hiding this comment.
Suggest sentence case, plus warning that this will be an error condition in the future
| log.Warn("bundle endpoint has no profile set, using https_spiffe as default, please choose a profile.") | |
| log.Warn("Bundle endpoint is configured but has no profile set, using https_spiffe as default; please configure a profile explicitly. This will be fatal in a future release.") |
| if acme != nil { | ||
| acmeConfig = configToACMEConfig(acme, dataDir) | ||
| } | ||
| return acmeConfig, nil |
There was a problem hiding this comment.
this will be a nil pointer if https_web is set but acme block isn't declared. I think we need a check that it is set? I think it's mandatory for https_web profile
cmd/spire-server/cli/run/run.go
Outdated
| return nil, nil | ||
|
|
||
| default: | ||
| return nil, errors.New(`no bundle endpoint profile defined; current supported profiles are "https_spiffe" and 'https_web"`) |
There was a problem hiding this comment.
I think this would be a bit clearer if we talk about configuration explicitly. If we're hitting this, then a profile is set but it's unknown? It would be nice if we could also say what the value of the unknown profile was so the user can more easily find it. I don't think we should block on that though
| return nil, errors.New(`no bundle endpoint profile defined; current supported profiles are "https_spiffe" and 'https_web"`) | |
| return nil, errors.New(`unknown bundle endpoint profile configured; current supported profiles are "https_spiffe" and 'https_web"`) |
cmd/spire-server/cli/run/run.go
Outdated
| @@ -695,6 +751,23 @@ func parseBundleEndpointProfile(config federatesWithConfig) (trustDomainConfig * | |||
| }, nil | |||
| } | |||
|
|
|||
| func parseSingleAstNode(node ast.Node) (string, error) { | |||
There was a problem hiding this comment.
We may consider renaming this function since it has logic specifically for bundle endpoint profile handling e.g. parseBundleEndpointProfileASTNode or maybe something shorter 😂
doc/spire_server.md
Outdated
| |-----------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | address | IP address where this server will listen for HTTP requests | | ||
| | port | TCP port number where this server will listen for HTTP requests | | ||
| | acme | Automated Certificate Management Environment configuration section (see below) | |
There was a problem hiding this comment.
Should we remove this from the docs since we're trying to deprecate it?
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
cmd/spire-server/cli/run/run.go
Outdated
| if acme != nil { | ||
| acmeConfig = configToACMEConfig(acme, dataDir) | ||
| if acme == nil { | ||
| return nil, fmt.Errorf("falformed https_web: ACME is required") |
There was a problem hiding this comment.
minor typo
| return nil, fmt.Errorf("falformed https_web: ACME is required") | |
| return nil, fmt.Errorf("malformed https_web profile configuration: ACME is required") |
| refresh_hint = "10m" | ||
| profile "https_web" { |
There was a problem hiding this comment.
non-blocking: Sorry I missed this before, but I think we need to document this in ### Configuration options for federation.bundle_endpoint section?
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com>
Add profile configuration to bundle endpoint
Which issue this PR fixes
fixes #2670