Can't run operators locally, when using the conditions package

Bug Report

The NewCondition (and other) method, called (indirectlly) to the readSAFile method. This method uses a hard code path for the operator namespace file:

operator-lib/internal/utils/utils.go

Line 29 in c0ba7dc

    
           return ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")

This implementation blocks the ability to run the operator locally for develpment and debug,

Please consider one of the following, or both (pefered):

Replace the hard-code path with a environment variable, with a default of the current value.
Use an environment variable to store the namespace, and use the file only if this variable is not set.

Environment

operator-lib version:

Add option to read the operator namespace from an envirnment variable…

@nunnatsa

Hi @nunnatsa ,
Thanks for looking into this. The PR does solve this issue. But however, the main use case of this library was to provide helpers to interact with the Operator Conditions created by OLM. This means that the operator is being run on cluster which has OLM installed. If not, we would not be able to get the name of the operator conditions CR which is set by OLM. There is a warning regarding this in the GetNamespacedName godoc. Can I know how you are testing this locally ? If you are running this on a dev environment without OLM, there shouldn't be an Operator Conditions CR created.

Relevant PR comment: #42 (comment)

@varshaprasad96

Thank you @varshaprasad96 for the detailed response.

As an operator developer, I need to be able to test my code and to debug it. I have many options to do that. For example, running with a real cluster, or running a unit test. In a cluster I can, for example, to manually deploy the opetator condition CRD and then the CR even without a running OLM.

The thing is that with the current implementation, it is harder to write a testable code.

The enhancement for this lib explicitly says that it assumes, and therefore was developed assuming, that OLM is installed. Additionally the CRD enhancement doesn't make an explicit goal for it to be used without OLM, although this is possible I guess. Regardless the intent of this library is specifically to be used with OLM. Out of curiosity why would you be using the operator condition CRD without OLM?

If you really want to, you still can use GetNamespacedName() for your use case like so:

// Set in test code.
var (
	testNS 		 = "foo"
	testCondName = "bar"
)

func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	...

	condKey, err := GetNamespacedName()
	if err != nil {
		// Errors pertaining to running outside of a cluster.
		condKey = &types.NamespacedName{Name: testCondName, Namespace: testNS}
	}

	...

}

The enhancement for this lib explicitly says that it assumes, and therefore was developed assuming, that OLM is installed.

But the code that uses it still need to be tested and debugging.

Additionally the CRD enhancement doesn't make an explicit goal for it to be used without OLM, although this is possible I guess.

There is no point to use this library without OLM, but still need to test and debug the code that uses it.

Regardless the intent of this library is specifically to be used with OLM. Out of curiosity why would you be using the operator condition CRD without OLM?

Yes! When I run a unit test of the function that called NewCondition. I can't do that using the current implementation.

If you really want to, you still can use GetNamespacedName() for your use case like so:

// Set in test code.
var (
	testNS 		 = "foo"
	testCondName = "bar"
)

func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	...

	condKey, err := GetNamespacedName()
	if err != nil {
		// Errors pertaining to running outside of a cluster.
		condKey = &types.NamespacedName{Name: testCondName, Namespace: testNS}
	}

	...

}

This code is a mix of a test code and a production code. There is no way to add a call to NewCondition and not getting error. So all the tests that get to this point will exit with an expected error, and rhere will no way to test what happends next.

But the code that uses it still need to be tested and debugging.

Yes, and the suggested way to handle running the tests locally is to skip conditions-related code when an error is returned.

Yes! When I run a unit test of the function that called NewCondition. I can't do that using the current implementation.

What I don’t understand is what you’re trying to test when you’re running your operator locally and trying to write to the conditions CR. In this scenario, OLM either isn’t running or doesn’t know about your operator if it is. Therefore you’re only testing if you can write to the condition CR, which isn’t really useful; in fact this test is effectively already being performed in the library’s unit tests.

This code is a mix of a test code and a production code. There is no way to add a call to NewCondition and not getting error. So all the tests that get to this point will exit with an expected error, and rhere will no way to test what happends next.

Yes it is a gross mix, but it does let you set the in-cluster condition CR and namespace if you really want to go down that path. However I don’t actually recommend using the code I suggested for the reason I gave above. You shouldn’t need to test writing operator conditions in non-cluster scenarios.

Let's try to explain again the issue.

The operator we're working on does many things beside setting the the operator-condition. The code that do that is part of much larer code. In order to run unit tests we must mock the cluster. We're doing it to mock reading, writing and updating many types of objects.

But using this package as it implemented now will break any unit test we'll trying to write. We must simulate the running environment for tests, we can't skip it because this operaor need to eventually run in a cluster.

Sorry to write it, but this issue is a real issue that can't be rejected by "it's written in the godoc". You don't have to use the sugested solution, but please understand that this is not something we can skip.

I understand the issue you’re posing, and I think because the solution isn’t clear we need to improve the UX of this library, in particular handling the local/testing case.

I think the right solution is to create a test Condition that you can substitute in unit tests with a function variable, ex:

// controllers/kind_controller.go

var newCondition = conditions.NewCondition

func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	...

	cond, err := newCondition(client, SomeCondType)
	if err != nil {
		// This error will never happen in tests.
	}

	...
}

// controllers/kind_controller_test.go

func TestReconciler(t *testing.T) {
    newCondition = conditions.NewFakeCondition

    // Run reconciler tests.
}

That would solve your problem correct?

How would I run this out-of-cluster (eg. when developing on my machine)?

This is how we can run an operator from the IDE: https://github.com/kubevirt/hyperconverged-cluster-operator/blob/master/docs/run-locally.md

The issue is not only in testing, but also running the operator from a local machine for debugging.
Due to the filesystem lookup an operator has to always run in the container filesystem, so something like Telepresence becomes mandatory for development.

OLM itself is already injecting environment variables for a few things and I don't see a rationale behind reading the namespace from file instead.

Kubernetes even has a downward API to make this task trivial:

        - name: MY_POD_NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace

https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/

I also don't think that having a FakeCondition to substitute would help, it doesn't address the portability issue during development.
There is an argument to be had about not asserting on a change on the OperatorCondition object, but instead just mocking the call and asserting that a call has been made, but I don't think the API is complicated enough to require this library to ship the stub.

@nunnatsa

@nunnatsa From your original comment, the reason the directory is hard-coded is because this is where it will live in a Kubernetes environment. If you are running outside of the cluster that location will not exist. This is true when operator-lib is being unit tested outside the cluster. For example, with the leader code, we made the readNamespace a variable to a function: https://github.com/operator-framework/operator-lib/blob/main/leader/leader.go#L41. This allows it to be set for testing like this https://github.com/operator-framework/operator-lib/blob/main/leader/leader_test.go#L83-L85

I'm open to other solutions but the hardcoded path is where the cluster will put the namespace information. In the next comment I'll outline a few thoughts.

How are you trying to test your operator? unit tests? Or is it trying to run it locally like @thetechnick mentioned.

But there is a catch: With every stubbed/mocked dependency the test case needs to make more and more rigid assumptions about call order, exact arguments and API usage. In my experience this makes test cases very brittle, as implementation changes during refactoring invalidates most of the test case.

In general I agree with you. This particular case is interesting though, because you are effectively writing to a list of objects (a conditions CR's status) that is not written to by any other entity than the operator. The conditions CRD status is that simple. Therefore a mock that has an underlying list of conditions would almost exactly replicate in-cluster behavior. Now you could argue that k8s guarantees don't apply so the mock can introduce bugs, but I counter that with: you should be writing e2e tests too, since unit tests are not intended to extensively test in-cluster scenarios.

As a software maintainer, I would like to have the choice to either mock, stub and fake my way through tests or run the actual code to verify functionality.

Ultimately I agree with this statement, and therefore it is probably ok to expose some way to set a namespace as well as provide a mock Conditions constructor. My main issue with doing both is that providing two very different ways to test something is confusing to people, which can be somewhat remedied by documentation saying "use this namespace override only if you know what you're doing, otherwise use this mocker".

If namespace injection is going to be added, I'd also like to see injection happen only when running locally, i.e.

const OperatorNamespaceLocalEnv = "OPERATOR_NAMESPACE_LOCAL"
const podNamespaceFilePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"

if _, err := os.Stat(podNamespaceFilePath); err == nil || errors.Is(err, os.ErrExist) {
	return readNSFile()
} else if localNSValue, isSet := os.LookupEnv(OperatorNamespaceLocalEnv); isSet {
	return localNSValue, nil
} else {
	return "", ErrNamespaceNotSet
}

This disallows using this environment variable in production, which prevents incorrect and confusion about usage IMO.

@estroz

@estroz That would be all I need :)

I already wanted to submit a PR to add this, but falling back to the env lookup on error looks quite fragile to me. Unfortunately Go is not abstracting errors across environments (Linux, OS X, Windows?) very nicely, also there are probably a few cases that need special handling:

Path does not exist
Lack of permissions
Device, does not exist? (on Windows?)

And any other odd error that could come up in this context...

I completely understand, that you don't want anyone ever to use this in prod, so would you mind making this dead simple?
Ceph's --yes-i-really-mean-it cli flag springs to mind :D

const OperatorNamespaceLocalEnv = "OPERATOR_NAMESPACE_OVERRIDE_NEVER_USE_IN_PROD"
const podNamespaceFilePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace"

if localNSValue, isSet := os.LookupEnv(OperatorNamespaceLocalEnv); isSet && len(localNSValue) > 0 {
	return localNSValue, nil
}
return readNSFile()

@jmrodri

I know I'm really late to this, but I'm the one that had opinions about making this interface less "magic". In pre-1.0 versions of operator-sdk, we supported something similar via environment variables. If I remember correctly, we supported:

OPERATOR_NAMESPACE: to lookup the namespace the operator is running in
OPERATOR_SDK_RUN_LOCAL: to tell us whether we were running locally or not.

Both of these struck me as odd and hard for users to get right.

OPERATOR_NAMESPACE needed to be set explicitly on the operator pod even though it is trivial to read this from the cluster (let me hand wave over use cases like testing for a moment). This is easy to miss or get wrong.
OPERATOR_SDK_RUN_LOCAL just seemed bad all around. What if I set that variable when running in a cluster? It seems incongruous to have an environment variable tell the process something that it could very likely determine on its own.

So with that context in mind, I think our goal for this problem should be:

By default this library should read the namespace from the mounted service account metadata. This is what all end-users expect.
When it is necessary for other use cases (e.g. testing), this library should support an injection point that enables the test to provide a namespace rather than read it from the cluster.
There should be no environment variable supported by this library that conveys where an operator is running.

I think 2) is what we're all discussing. To avoid footguns around the "magic" caused by environment variable side effects we experienced with this in SDK pre-v1, I would propose that we make it possible for library users (not end users) to inject custom namespaces via a code API.

The simplest way to do this would be to make this an exported variable. However, since that's a global variable, it would not be thread safe in concurrent testing scenarios, so I would be against that approach.

A more complex, but improved solution would be for us to add a Factory interface that can build arbitrary Condition objects, and we can provide a InClusterFactory struct that would use the in-cluster implementation.

If code under test is aware only of the Factory abstraction, then authors can swap implementations in and out as necessary to test various aspects of their operator.

These ideas are very similar to the ones proposed by @jmrodri here, and I think something like this is the path we should ultimately take.

Here is a PR I posted with this idea implemented: #58

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@joelanford

@joelanford - this is still a blocker for the implementation of operator condition.

/remove-lifecycle rotten

@nunnatsa

@nunnatsa thanks for staying on top of this. We'll re-prioritize this issue.

Navigation Menu

Repository navigation

Can't run operators locally, when using the conditions package #50

Bug Report

8 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Can't run operators locally, when using the conditions package #50

Description

Bug Report

Activity

varshaprasad96 commented on Jan 25, 2021

estroz commented on Jan 25, 2021

nunnatsa commented on Jan 25, 2021

estroz commented on Jan 25, 2021

nunnatsa commented on Jan 26, 2021

estroz commented on Jan 26, 2021

nunnatsa commented on Jan 26, 2021

estroz commented on Jan 26, 2021

erdii commented on Jan 26, 2021

nunnatsa commented on Jan 26, 2021

thetechnick commented on Jan 26, 2021

jmrodri commented on Jan 26, 2021

8 remaining items

estroz commented on Jan 27, 2021

estroz commented on Jan 27, 2021

thetechnick commented on Jan 28, 2021

joelanford commented on Apr 20, 2021

openshift-bot commented on Jul 20, 2021

openshift-bot commented on Aug 19, 2021

nunnatsa commented on Aug 19, 2021

nunnatsa commented on Aug 19, 2021

jmrodri commented on Sep 8, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions