Skip to content

Commit bbc7185

Browse files
authored
CLOUDP-302068: Add a linter to forbid env var usage (#1690)
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 0e72089 commit bbc7185

File tree

11 files changed

+53
-31
lines changed

11 files changed

+53
-31
lines changed

.golangci.yml

+21
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ issues:
1616
- goconst
1717
- golint
1818
text: "underscore"
19+
- path: ^pkg\/util\/envvar
20+
linters:
21+
- forbidigo
22+
- path: ^cmd\/(readiness|versionhook|manager)\/main\.go$
23+
linters:
24+
- forbidigo
1925
linters:
2026
enable:
2127
- govet
@@ -28,10 +34,25 @@ linters:
2834
- rowserrcheck
2935
- gosec
3036
- unconvert
37+
- forbidigo
3138
linters-settings:
3239
gosec:
3340
excludes:
3441
- G115
42+
forbidigo:
43+
forbid:
44+
- p: os\.(Getenv|LookupEnv|Environ|ExpandEnv)
45+
pkg: os
46+
msg: "Reading environemnt variables here is prohibited. Please read environment variables in the main package."
47+
- p: os\.(Clearenv|Unsetenv|Setenv)
48+
msg: "Modifying environemnt variables is prohibited."
49+
pkg: os
50+
- p: envvar\.(Read.*?|MergeWithOverride|GetEnvOrDefault)
51+
pkg: github.com/mongodb/mongodb-kubernetes-operator/pkg/util/envvar
52+
msg: "Using this envvar package here is prohibited. Please work with environment variables in the main package."
53+
# Rules with the `pkg` depend on it
54+
analyze-types: true
55+
3556
run:
3657
modules-download-mode: mod
3758
# timeout for analysis, e.g. 30s, 5m, default is 1m

controllers/construct/build_statefulset_test.go

+1-1
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

+1-1
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

+5-5
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

+1-1
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

+1-1
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

+1-1
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

+4-4
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

+1-1
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

+3-3
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

+14-13
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)