Skip to content

Commit ea174d6

Browse files
m1kolam1kola
committed
Add a linter to forbid env var usage
We want to avoid spreading the practice of accessing environment variables deep in the call hierarchy. We only want to access env vars in the `main` package or very close to it. This commit adds a linter to help us with that. It also adds `nolint:forbidigo` exceptions into places: * Where we allow access (`main` package) * Where we still unfortunately have access. These places ideally need to change with time. To find remaining undesired places, from repo root you can `grep` all files for `nolint:forbidigo` excluding files with `main` package. For example: `grep "nolint:forbidigo" --exclude main.go -r .`
1 parent 6e53d3c commit ea174d6

File tree

14 files changed

+62
-43
lines changed

14 files changed

+62
-43
lines changed

‎.golangci.yml

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ issues:
1616
- goconst
1717
- golint
1818
text: "underscore"
19+
- path: pkg/util/envvar/
20+
linters:
21+
- forbidigo
1922
linters:
2023
enable:
2124
- govet
@@ -28,10 +31,25 @@ linters:
2831
- rowserrcheck
2932
- gosec
3033
- unconvert
34+
- forbidigo
3135
linters-settings:
3236
gosec:
3337
excludes:
3438
- G115
39+
forbidigo:
40+
forbid:
41+
- p: os\.(Getenv|LookupEnv|Environ|ExpandEnv)
42+
pkg: os
43+
msg: "Reading environemnt variables here is prohibited. Please read environment variables in the main package."
44+
- p: os\.(Clearenv|Unsetenv|Setenv)
45+
msg: "Modifying environemnt variables is prohibited."
46+
pkg: os
47+
- p: envvar\.(Read.*?|MergeWithOverride|GetEnvOrDefault)
48+
pkg: github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar
49+
msg: "Using this envvar package here is prohibited. Please work with environment variables in the main package."
50+
# Rules with the `pkg` depend on it
51+
analyze-types: true
52+
3553
run:
3654
modules-download-mode: mod
3755
# timeout for analysis, e.g. 30s, 5m, default is 1m

‎cmd/manager/main.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func configureLogger() (*zap.Logger, error) {
4444
func hasRequiredVariables(logger *zap.Logger, envVariables ...string) bool {
4545
allPresent := true
4646
for _, envVariable := range envVariables {
47-
if _, envSpecified := os.LookupEnv(envVariable); !envSpecified {
47+
if _, envSpecified := os.LookupEnv(envVariable); !envSpecified { // nolint:forbidigo
4848
logger.Error(fmt.Sprintf("required environment variable %s not found", envVariable))
4949
allPresent = false
5050
}
@@ -70,7 +70,7 @@ func main() {
7070
}
7171

7272
// Get watch namespace from environment variable.
73-
namespace, nsSpecified := os.LookupEnv(WatchNamespaceEnv)
73+
namespace, nsSpecified := os.LookupEnv(WatchNamespaceEnv) // nolint:forbidigo
7474
if !nsSpecified {
7575
log.Sugar().Fatal("No namespace specified to watch")
7676
}
@@ -110,12 +110,12 @@ func main() {
110110
// Setup Controller.
111111
if err = controllers.NewReconciler(
112112
mgr,
113-
os.Getenv(construct.MongodbRepoUrlEnv),
114-
os.Getenv(construct.MongodbImageEnv),
115-
envvar.GetEnvOrDefault(construct.MongoDBImageTypeEnv, construct.DefaultImageType),
116-
os.Getenv(construct.AgentImageEnv),
117-
os.Getenv(construct.VersionUpgradeHookImageEnv),
118-
os.Getenv(construct.ReadinessProbeImageEnv),
113+
os.Getenv(construct.MongodbRepoUrlEnv), // nolint:forbidigo
114+
os.Getenv(construct.MongodbImageEnv), // nolint:forbidigo
115+
envvar.GetEnvOrDefault(construct.MongoDBImageTypeEnv, construct.DefaultImageType), // nolint:forbidigo
116+
os.Getenv(construct.AgentImageEnv), // nolint:forbidigo
117+
os.Getenv(construct.VersionUpgradeHookImageEnv), // nolint:forbidigo
118+
os.Getenv(construct.ReadinessProbeImageEnv), // nolint:forbidigo
119119
).SetupWithManager(mgr); err != nil {
120120
log.Sugar().Fatalf("Unable to create controller: %v", err)
121121
}

‎cmd/readiness/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ func performCheckOMMode(health health.Status) bool {
180180
}
181181

182182
func isHeadlessMode() bool {
183-
return os.Getenv(headlessAgent) == "true"
183+
return os.Getenv(headlessAgent) == "true" // nolint:forbidigo
184184
}
185185

186186
func kubernetesClientset() (kubernetes.Interface, error) {

‎cmd/versionhook/main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func main() {
3232

3333
logger.Info("Running version change post-start hook")
3434

35-
if statusPath := os.Getenv(agentStatusFilePathEnv); statusPath == "" {
35+
if statusPath := os.Getenv(agentStatusFilePathEnv); statusPath == "" { // nolint:forbidigo
3636
logger.Fatalf(`Required environment variable "%s" not set`, agentStatusFilePathEnv)
3737
return
3838
}
@@ -124,7 +124,7 @@ func waitForAgentHealthStatus() (agent.Health, error) {
124124
// getAgentHealthStatus returns an instance of agent.Health read
125125
// from the health file on disk
126126
func getAgentHealthStatus() (agent.Health, error) {
127-
f, err := os.Open(os.Getenv(agentStatusFilePathEnv))
127+
f, err := os.Open(os.Getenv(agentStatusFilePathEnv)) // nolint:forbidigo
128128
if err != nil {
129129
return agent.Health{}, err
130130
}
@@ -150,7 +150,7 @@ func readAgentHealthStatus(reader io.Reader) (agent.Health, error) {
150150
}
151151

152152
func getHostname() string {
153-
return os.Getenv("HOSTNAME")
153+
return os.Getenv("HOSTNAME") // nolint:forbidigo
154154
}
155155

156156
// shouldDeletePod returns a boolean value indicating if this pod should be deleted

‎controllers/construct/build_statefulset_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func assertStatefulSetIsBuiltCorrectly(t *testing.T, mdb mdbv1.MongoDBCommunity,
109109
assert.Len(t, sts.Spec.Template.Spec.Containers[0].Env, 4)
110110
assert.Len(t, sts.Spec.Template.Spec.Containers[1].Env, 1)
111111

112-
managedSecurityContext := envvar.ReadBool(podtemplatespec.ManagedSecurityContextEnv)
112+
managedSecurityContext := envvar.ReadBool(podtemplatespec.ManagedSecurityContextEnv) // nolint:forbidigo
113113
if !managedSecurityContext {
114114
assert.NotNil(t, sts.Spec.Template.Spec.SecurityContext)
115115
assert.Equal(t, podtemplatespec.DefaultPodSecurityContext(), *sts.Spec.Template.Spec.SecurityContext)

‎controllers/construct/mongodbstatefulset.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ func collectEnvVars() []corev1.EnvVar {
417417
})
418418

419419
addEnvVarIfSet := func(name string) {
420-
value := os.Getenv(name)
420+
value := os.Getenv(name) // nolint:forbidigo
421421
if value != "" {
422422
envVars = append(envVars, corev1.EnvVar{
423423
Name: name,

‎controllers/replica_set_controller.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func (r ReplicaSetReconciler) Reconcile(ctx context.Context, request reconcile.R
219219
}
220220

221221
res, err := status.Update(ctx, r.client.Status(), &mdb, statusOptions().
222-
withMongoURI(mdb.MongoURI(os.Getenv(clusterDomain))).
222+
withMongoURI(mdb.MongoURI(os.Getenv(clusterDomain))). // nolint:forbidigo
223223
withMongoDBMembers(mdb.AutomationConfigMembersThisReconciliation()).
224224
withStatefulSetReplicas(mdb.StatefulSetReplicasThisReconciliation()).
225225
withStatefulSetArbiters(mdb.StatefulSetArbitersThisReconciliation()).
@@ -232,7 +232,7 @@ func (r ReplicaSetReconciler) Reconcile(ctx context.Context, request reconcile.R
232232
return res, err
233233
}
234234

235-
if err := r.updateConnectionStringSecrets(ctx, mdb, os.Getenv(clusterDomain)); err != nil {
235+
if err := r.updateConnectionStringSecrets(ctx, mdb, os.Getenv(clusterDomain)); err != nil { // nolint:forbidigo
236236
r.log.Errorf("Could not update connection string secrets: %s", err)
237237
}
238238

@@ -515,8 +515,8 @@ func (r ReplicaSetReconciler) ensureAutomationConfig(mdb mdbv1.MongoDBCommunity,
515515
}
516516

517517
func buildAutomationConfig(mdb mdbv1.MongoDBCommunity, isEnterprise bool, auth automationconfig.Auth, currentAc automationconfig.AutomationConfig, modifications ...automationconfig.Modification) (automationconfig.AutomationConfig, error) {
518-
domain := getDomain(mdb.ServiceName(), mdb.Namespace, os.Getenv(clusterDomain))
519-
arbiterDomain := getDomain(mdb.ServiceName(), mdb.Namespace, os.Getenv(clusterDomain))
518+
domain := getDomain(mdb.ServiceName(), mdb.Namespace, os.Getenv(clusterDomain)) // nolint:forbidigo
519+
arbiterDomain := getDomain(mdb.ServiceName(), mdb.Namespace, os.Getenv(clusterDomain)) // nolint:forbidigo
520520

521521
zap.S().Debugw("AutomationConfigMembersThisReconciliation", "mdb.AutomationConfigMembersThisReconciliation()", mdb.AutomationConfigMembersThisReconciliation())
522522

@@ -557,7 +557,7 @@ func buildAutomationConfig(mdb mdbv1.MongoDBCommunity, isEnterprise bool, auth a
557557
}
558558

559559
func guessEnterprise(mdb mdbv1.MongoDBCommunity, mongodbImage string) bool {
560-
overrideAssumption, err := strconv.ParseBool(os.Getenv(construct.MongoDBAssumeEnterpriseEnv))
560+
overrideAssumption, err := strconv.ParseBool(os.Getenv(construct.MongoDBAssumeEnterpriseEnv)) // nolint:forbidigo
561561
if err == nil {
562562
return overrideAssumption
563563
}

‎pkg/kube/container/container_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func TestMergeEnvs(t *testing.T) {
146146
},
147147
}
148148

149-
merged := envvar.MergeWithOverride(existing, desired)
149+
merged := envvar.MergeWithOverride(existing, desired) // nolint:forbidigo
150150

151151
t.Run("EnvVars should be sorted", func(t *testing.T) {
152152
assert.Equal(t, "A_env", merged[0].Name)

‎pkg/kube/container/containers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func WithLifecycle(lifeCycleMod lifecycle.Modification) Modification {
129129
// WithEnvs ensures all of the provided envs exist in the container
130130
func WithEnvs(envs ...corev1.EnvVar) Modification {
131131
return func(container *corev1.Container) {
132-
container.Env = envvar.MergeWithOverride(container.Env, envs)
132+
container.Env = envvar.MergeWithOverride(container.Env, envs) // nolint:forbidigo
133133
}
134134
}
135135

‎pkg/kube/podtemplatespec/podspec_template.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ func FindContainerByName(name string, podTemplateSpec *corev1.PodTemplateSpec) *
297297
}
298298

299299
func WithDefaultSecurityContextsModifications() (Modification, container.Modification) {
300-
managedSecurityContext := envvar.ReadBool(ManagedSecurityContextEnv)
300+
managedSecurityContext := envvar.ReadBool(ManagedSecurityContextEnv) // nolint:forbidigo
301301
configureContainerSecurityContext := container.NOOP()
302302
configurePodSpecSecurityContext := NOOP()
303303
if !managedSecurityContext {

‎pkg/readiness/config/config.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,15 @@ func BuildFromEnvVariables(clientSet kubernetes.Interface, isHeadless bool, file
4343
var namespace, automationConfigName, hostname string
4444
if isHeadless {
4545
var ok bool
46-
namespace, ok = os.LookupEnv(podNamespaceEnv)
46+
namespace, ok = os.LookupEnv(podNamespaceEnv) // nolint:forbidigo
4747
if !ok {
4848
return Config{}, fmt.Errorf("the '%s' environment variable must be set", podNamespaceEnv)
4949
}
50-
automationConfigName, ok = os.LookupEnv(automationConfigSecretEnv)
50+
automationConfigName, ok = os.LookupEnv(automationConfigSecretEnv) // nolint:forbidigo
5151
if !ok {
5252
return Config{}, fmt.Errorf("the '%s' environment variable must be set", automationConfigSecretEnv)
5353
}
54-
hostname, ok = os.LookupEnv(hostNameEnv)
54+
hostname, ok = os.LookupEnv(hostNameEnv) // nolint:forbidigo
5555
if !ok {
5656
return Config{}, fmt.Errorf("the '%s' environment variable must be set", hostNameEnv)
5757
}
@@ -85,7 +85,7 @@ func readinessProbeLogFilePath() string {
8585
}
8686

8787
func GetEnvOrDefault(envVar, defaultValue string) string {
88-
value := strings.TrimSpace(os.Getenv(envVar))
88+
value := strings.TrimSpace(os.Getenv(envVar)) // nolint:forbidigo
8989
if value == "" {
9090
return defaultValue
9191
}

‎test/e2e/e2eutil.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestAnnotations() map[string]string {
3636
}
3737

3838
func TestDataDir() string {
39-
return envvar.GetEnvOrDefault(testDataDirEnv, "/workspace/testdata")
39+
return envvar.GetEnvOrDefault(testDataDirEnv, "/workspace/testdata") // nolint:forbidigo
4040
}
4141

4242
func TlsTestDataDir() string {

‎test/e2e/setup/setup.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ const (
4242
)
4343

4444
func Setup(ctx context.Context, t *testing.T) *e2eutil.TestContext {
45-
testCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv))
45+
testCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv)) // nolint:forbidigo
4646

4747
if err != nil {
4848
t.Fatal(err)
@@ -57,7 +57,7 @@ func Setup(ctx context.Context, t *testing.T) *e2eutil.TestContext {
5757
}
5858

5959
func SetupWithTLS(ctx context.Context, t *testing.T, resourceName string, additionalHelmArgs ...HelmArg) (*e2eutil.TestContext, TestConfig) {
60-
textCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv))
60+
textCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv)) // nolint:forbidigo
6161

6262
if err != nil {
6363
t.Fatal(err)
@@ -76,7 +76,7 @@ func SetupWithTLS(ctx context.Context, t *testing.T, resourceName string, additi
7676
}
7777

7878
func SetupWithTestConfig(ctx context.Context, t *testing.T, testConfig TestConfig, withTLS, defaultOperator bool, resourceName string) *e2eutil.TestContext {
79-
testCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv))
79+
testCtx, err := e2eutil.NewContext(ctx, t, envvar.ReadBool(performCleanupEnv)) // nolint:forbidigo
8080

8181
if err != nil {
8282
t.Fatal(err)

‎test/e2e/setup/test_config.go

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,18 +34,19 @@ type TestConfig struct {
3434

3535
func LoadTestConfigFromEnv() TestConfig {
3636
return TestConfig{
37-
Namespace: envvar.GetEnvOrDefault(testNamespaceEnvName, "mongodb"),
38-
CertManagerNamespace: envvar.GetEnvOrDefault(testCertManagerNamespaceEnvName, "cert-manager"),
39-
CertManagerVersion: envvar.GetEnvOrDefault(testCertManagerVersionEnvName, "v1.5.3"),
40-
OperatorImage: envvar.GetEnvOrDefault(operatorImageEnvName, "quay.io/mongodb/community-operator-dev:latest"),
41-
MongoDBImage: envvar.GetEnvOrDefault(construct.MongodbImageEnv, "mongodb-community-server"),
42-
MongoDBRepoUrl: envvar.GetEnvOrDefault(construct.MongodbRepoUrlEnv, "quay.io/mongodb"),
43-
VersionUpgradeHookImage: envvar.GetEnvOrDefault(construct.VersionUpgradeHookImageEnv, "quay.io/mongodb/mongodb-kubernetes-operator-version-upgrade-post-start-hook:1.0.2"),
44-
AgentImage: envvar.GetEnvOrDefault(construct.AgentImageEnv, "quay.io/mongodb/mongodb-agent-ubi:10.29.0.6830-1"), // TODO: better way to decide default agent image.
45-
ClusterWide: envvar.ReadBool(clusterWideEnvName),
46-
PerformCleanup: envvar.ReadBool(performCleanupEnvName),
47-
ReadinessProbeImage: envvar.GetEnvOrDefault(construct.ReadinessProbeImageEnv, "quay.io/mongodb/mongodb-kubernetes-readinessprobe:1.0.3"),
48-
HelmChartPath: envvar.GetEnvOrDefault(helmChartPathEnvName, "/workspace/helm-charts/charts/community-operator"),
49-
LocalOperator: envvar.ReadBool(LocalOperatorEnvName),
37+
Namespace: envvar.GetEnvOrDefault(testNamespaceEnvName, "mongodb"), // nolint:forbidigo
38+
CertManagerNamespace: envvar.GetEnvOrDefault(testCertManagerNamespaceEnvName, "cert-manager"), // nolint:forbidigo
39+
CertManagerVersion: envvar.GetEnvOrDefault(testCertManagerVersionEnvName, "v1.5.3"), // nolint:forbidigo
40+
OperatorImage: envvar.GetEnvOrDefault(operatorImageEnvName, "quay.io/mongodb/community-operator-dev:latest"), // nolint:forbidigo
41+
MongoDBImage: envvar.GetEnvOrDefault(construct.MongodbImageEnv, "mongodb-community-server"), // nolint:forbidigo
42+
MongoDBRepoUrl: envvar.GetEnvOrDefault(construct.MongodbRepoUrlEnv, "quay.io/mongodb"), // nolint:forbidigo
43+
VersionUpgradeHookImage: envvar.GetEnvOrDefault(construct.VersionUpgradeHookImageEnv, "quay.io/mongodb/mongodb-kubernetes-operator-version-upgrade-post-start-hook:1.0.2"), // nolint:forbidigo
44+
// TODO: better way to decide default agent image.
45+
AgentImage: envvar.GetEnvOrDefault(construct.AgentImageEnv, "quay.io/mongodb/mongodb-agent-ubi:10.29.0.6830-1"), // nolint:forbidigo
46+
ClusterWide: envvar.ReadBool(clusterWideEnvName), // nolint:forbidigo
47+
PerformCleanup: envvar.ReadBool(performCleanupEnvName), // nolint:forbidigo
48+
ReadinessProbeImage: envvar.GetEnvOrDefault(construct.ReadinessProbeImageEnv, "quay.io/mongodb/mongodb-kubernetes-readinessprobe:1.0.3"), // nolint:forbidigo
49+
HelmChartPath: envvar.GetEnvOrDefault(helmChartPathEnvName, "/workspace/helm-charts/charts/community-operator"), // nolint:forbidigo
50+
LocalOperator: envvar.ReadBool(LocalOperatorEnvName), // nolint:forbidigo
5051
}
5152
}

0 commit comments

Comments
 (0)