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.

Sign up for GitHub

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

Enable reload-interval flag for all TLS server configs #6525

Merged
merged 9 commits into from
Jan 12, 2025
+16 −66
Merged

Enable reload-interval flag for all TLS server configs #6525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
606ad6e
Set EnableCertReloadInterval: true everywhere
ADI-ROXX Jan 11, 2025
a582865
Set EnableCertReloadInterval: true everywhere
ADI-ROXX Jan 11, 2025
99809eb
Removed property
ADI-ROXX Jan 11, 2025
5edf8f0
Removed comments
ADI-ROXX Jan 11, 2025
cd01487
Removed comma
ADI-ROXX Jan 11, 2025
5de5286
Removed TestServerCertReloadInterval
ADI-ROXX Jan 11, 2025
c0e8dce
fix
yurishkuro Jan 12, 2025
a44a401
add-test
yurishkuro Jan 12, 2025
e1d7fe2
Merge branch 'main' into reloadInterval
yurishkuro Jan 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions cmd/collector/app/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,13 @@ var otlpServerFlagsCfg = struct {
GRPC: serverFlagsConfig{
prefix: "collector.otlp.grpc",
tls: tlscfg.ServerFlagsConfig{
Prefix: "collector.otlp.grpc",
EnableCertReloadInterval: true,
Prefix: "collector.otlp.grpc",
},
},
HTTP: serverFlagsConfig{
prefix: "collector.otlp.http",
tls: tlscfg.ServerFlagsConfig{
Prefix: "collector.otlp.http",
EnableCertReloadInterval: true,
Prefix: "collector.otlp.http",
},
corsCfg: corscfg.Flags{
Prefix: "collector.otlp.http",
Expand Down
28 changes: 0 additions & 28 deletions cmd/collector/app/flags/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,6 @@ func TestCollectorOptionsWithFailedTLSFlags(t *testing.T) {
}
}

func TestCollectorOptionsWithFlags_CheckTLSReloadInterval(t *testing.T) {
prefixes := []string{
"--collector.http",
"--collector.grpc",
"--collector.zipkin",
"--collector.otlp.http",
"--collector.otlp.grpc",
}
otlpPrefixes := map[string]struct{}{
"--collector.otlp.http": {},
"--collector.otlp.grpc": {},
}
for _, prefix := range prefixes {
t.Run(prefix, func(t *testing.T) {
_, command := config.Viperize(AddFlags)
err := command.ParseFlags([]string{
prefix + ".tls.enabled=true",
prefix + ".tls.reload-interval=24h",
})
if _, ok := otlpPrefixes[prefix]; !ok {
assert.ErrorContains(t, err, "unknown flag")
} else {
require.NoError(t, err)
}
})
}
}

func TestCollectorOptionsWithFlags_CheckMaxReceiveMessageLength(t *testing.T) {
c := &CollectorOptions{}
v, command := config.Viperize(AddFlags)
Expand Down
7 changes: 2 additions & 5 deletions pkg/config/tlscfg/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ type ClientFlagsConfig struct {

// ServerFlagsConfig describes which CLI flags for TLS server should be generated.
type ServerFlagsConfig struct {
Prefix string
EnableCertReloadInterval bool
Prefix string
}

// AddFlags adds flags for TLS to the FlagSet.
Expand All @@ -58,9 +57,7 @@ func (c ServerFlagsConfig) AddFlags(flags *flag.FlagSet) {
flags.String(c.Prefix+tlsCipherSuites, "", "Comma-separated list of cipher suites for the server, values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants).")
flags.String(c.Prefix+tlsMinVersion, "", "Minimum TLS version supported (Possible values: 1.0, 1.1, 1.2, 1.3)")
flags.String(c.Prefix+tlsMaxVersion, "", "Maximum TLS version supported (Possible values: 1.0, 1.1, 1.2, 1.3)")
if c.EnableCertReloadInterval {
flags.Duration(c.Prefix+tlsReloadInterval, 0, "The duration after which the certificate will be reloaded (0s means will not be reloaded)")
}
flags.Duration(c.Prefix+tlsReloadInterval, 0, "The duration after which the certificate will be reloaded (0s means will not be reloaded)")
}

// InitFromViper creates tls.Config populated with values retrieved from Viper.
Expand Down
41 changes: 12 additions & 29 deletions pkg/config/tlscfg/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package tlscfg
import (
"flag"
"testing"
"time"

"github.com/spf13/cobra"
"github.com/spf13/viper"
Expand Down Expand Up @@ -125,36 +126,18 @@ func TestServerFlags(t *testing.T) {
}

func TestServerCertReloadInterval(t *testing.T) {
tests := []struct {
config ServerFlagsConfig
}{
{
config: ServerFlagsConfig{
Prefix: "enabled",
EnableCertReloadInterval: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this test anymore. We can just move the parsing of this flag to TestServerFlags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So shall I remove this test completely?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ADI-ROXX Yeah - I think we can remove this test but we don't want to lose coverage of this flag. So we can add this flag as part of the test setup in TestServerFlags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we can add this flag as part of the test setup in TestServerFlags

I am not able to understand what is meant by this. What do I have to add in TestServerFlags? Because EnableCertReloadInterval is no longer in ServerFlagsConfig, what is to be done now?

},
},
{
config: ServerFlagsConfig{
Prefix: "disabled",
EnableCertReloadInterval: false,
},
},
}
for _, test := range tests {
t.Run(test.config.Prefix, func(t *testing.T) {
_, command := config.Viperize(test.config.AddFlags)
err := command.ParseFlags([]string{
"--" + test.config.Prefix + ".tls.enabled=true",
"--" + test.config.Prefix + ".tls.reload-interval=24h",
})
if !test.config.EnableCertReloadInterval {
assert.ErrorContains(t, err, "unknown flag")
} else {
require.NoError(t, err)
}
})
cfg := ServerFlagsConfig{
Prefix: "prefix",
}
v, command := config.Viperize(cfg.AddFlags)
err := command.ParseFlags([]string{
"--prefix.tls.enabled=true",
"--prefix.tls.reload-interval=24h",
})
require.NoError(t, err)
tlscfg, err := cfg.InitFromViper(v)
require.NoError(t, err)
assert.Equal(t, 24*time.Hour, tlscfg.ReloadInterval)
}

// TestFailedTLSFlags verifies that TLS options cannot be used when tls.enabled=false
Expand Down
Loading