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

[WIP] Added the field of EnableCertReloadInterval: true everywhere it is used and in the function #6525

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+15 −13
Open

[WIP] Added the field of EnableCertReloadInterval: true everywhere it is used and in the function #6525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 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
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
2 changes: 0 additions & 2 deletions cmd/collector/app/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,12 @@ var otlpServerFlagsCfg = struct {
prefix: "collector.otlp.grpc",
tls: tlscfg.ServerFlagsConfig{
Prefix: "collector.otlp.grpc",
EnableCertReloadInterval: true,
},
},
HTTP: serverFlagsConfig{
prefix: "collector.otlp.http",
tls: tlscfg.ServerFlagsConfig{
Prefix: "collector.otlp.http",
EnableCertReloadInterval: true,
},
corsCfg: corscfg.Flags{
Prefix: "collector.otlp.http",
Expand Down
13 changes: 13 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions pkg/config/tlscfg/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ type ClientFlagsConfig struct {
// ServerFlagsConfig describes which CLI flags for TLS server should be generated.
type ServerFlagsConfig struct {
Prefix string
EnableCertReloadInterval bool
}

// 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
8 changes: 1 addition & 7 deletions pkg/config/tlscfg/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,11 @@ func TestServerCertReloadInterval(t *testing.T) {
{
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.

},
},
{
config: ServerFlagsConfig{
Prefix: "disabled",
EnableCertReloadInterval: false,
},
},
}
Expand All @@ -148,11 +146,7 @@ func TestServerCertReloadInterval(t *testing.T) {
"--" + 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)
}
require.NoError(t, err)
})
}
}
Expand Down