Allow setting a preconfigured execRunner for cf authentication

[marcusholl]

This pull request is not ready to merge, but I would like to start a discussion about the issue adressed here

The cf cli can be configured environment variables (e.g. CF_TRACE). The current approach used here is to have a non-public instance of command.ExecRunner. With that approach it is not possible to set additional environment variables from outside controlling the behaviour of cf cli.

We have at least two options for fixing this.

1.) Provide the option to set an execRunner from outside. This is basically the approach sketched here.
2.) Extend the signatures of the functions so that we can hand over environment variables. These environment variables needs to be set on the non-public instance of command.ExecRunner.
3.) Switch to a receiver approach. The instance for command.Command is defined in the receiver.
4.) ...

Reason why this PR is not ready for being merged: When we use the approach sketched here we need to ensure that there is not status change of the exec runner after returning from the functions here. E.g. currently we set the stdout stream in order to be able to scan the output of the command. This needs to be set back via a defer. In order to get this we need to enhance the execRunner so that we can obtain a currently set output stream handle in order to be able to set it back afterwards.

I would like to suggest to add other approaches here in this PR so that we can discuss the pros and the cons.

[fwilhe]

Reason why this PR is not ready for being merged: When we use the approach sketched here we need to ensure that there is not status change of the exec runner after returning from the functions here. E.g. currently we set the stdout stream in order to be able to scan the output of the command. This needs to be set back via a defer. In order to get this we need to enhance the execRunner so that we can obtain a currently set output stream handle in order to be able to set it back afterwards.

Without looking at the code, this sounds like something where errors can be made easily and not spotted without knowing this context.

I would like to suggest to add other approaches here in this PR so that we can discuss the pros and the cons.

Good approach. I wonder why you did not start with the "2.) Extend the signatures of the functions" variant, that sounds less invasive to me.

[marcusholl]

I wonder why you did not start with the "2.) Extend the signatures of the functions" variant,

Well, each approach comes with pros and cons (eg. #1 is more invasive, but you have to do it only once ...). I don't say that approach #1 is better compared to #2 or worth. Basically I more the less thew the coin ... And I think it is good to have all options on the table before making a decission.

[marcusholl]

Other approach sketched with PR #1766

[marcusholl]

@fwilhe @DanielMieg @dominiklendle : also the approach with the environment variables provided via signature (sketched in #1766) does not fully convince since we have to reset the environment in this case (... we can't know who else is using cloudfoundry.Login()). We are in a short-living command line tool here, so the risk seems to be low, but nevertheless - in case we prefer clean coding we have to reset IMO.

Maybe we should think about a third approach. Having Login()/LoginCheck(./.) with a receiver and define a struct holding a reference to an instance of command.Command. That basically means the Login/LoginCheck are not "global" anymore:

type CFAuth struct {
    exec command.Command
}

(a *CFAuth)Login(opts LoginOpts) error {


    // here we work upon
    a.exec
    ...
}

We can discuss if we should default to a new command.Command{} in case there is no instance provided with the receiver - or if we would like to fail in that case.

[marcusholl]

@dominiklendle @DanielMieg : pkg/cloudfoundry/Authentication.go has been provided from your party. As we can see we have an issue with using another instance of command.Command for authenticating compared to e.g. cf deploy. My question here is not directly about approaches how to solve that. Imagine we would support having an instance of command.Command provided from outside. In this case we have to ensure that this instance is not modified. For the AuthCheck we have to manipulate stdout in order to receive the response of a cf api command. Now I'm asking me: what is the reason for having such a login check? Isn't cf login idempotent? Are there some side-effects caused by performing a second login? Are there some known causes where another login causes failures during e.g. subsequent commands? In case there are no such reasons: would it be possible to drop the login check?

[DanielMieg]

Regarding the Login Check - I don't think it is necessary. @dominiklendle What do you think? Was there a specific reason for this check?

[dominiklendle]

The Login Check in pkg/Authentication.go has been implemented for convenience reasons.
It is definitely not mandatory to execute a CF Login but it is much more convenient. If there is no login-check you would need to make sure in each step that the user gets logged in and logged out appropriately without polluting the logs too much.

Just an info: cf login doesn't give any information on whether the user is logged in or not. It just assumes that you want to login or re-login and immediately prompts for the credentials. cf api is (to my knowledge at least) the only command that returns the information "Not logged in." if user is not logged in.

I wouldn't mind changing the implementation if there is any other known way besides polling the login status with cf api command. However, I wouldn't completely drop this function since logs will then explode in more complex steps IMO.

[marcusholl]

Just an info: cf login doesn't give any information on whether the user is logged in or not

Why should it do that?

cf api is (to my knowledge at least) the only command that returns the information "Not logged in." if user is not logged in.

Why is it important for our use cases to know if a user has been logged in already?

However, I wouldn't completely drop this function since logs will then explode in more complex steps IMO.

We have basically two situations (resulting in these calls):

• With the cf api call:
  • when we are already logged in:
    • cf api
  • when we are not already logged in
    • cf api
    • cf login
• When we drop the cf api call
  • when we are already logged in:
    • cf login
  • when we are not already logged in
    • cf login

This means:

• in case we are already logged in the number of calls in the same (but cf api is replaced by cf login),
• when we are not already logged in the number of calls decreases by one. I'm not sure, but I think that case is our main use case.

Are you talking about server side logs (... remote end) or about logs on our side? I wouldn't care about logs on our end since we can control that. Regarding logs on the remote end: is cf api that much cheaper compared to cf login? At least on the level of e.g. the http-access-logs I think there is no difference. Regarding other kind of logs I can't judge.

Beside logging: I don't know how expensive a cf login on server side is. Maybe it is justified to have a prior cf api call in order to decrease the number of logins. But since I never got aware about any complains: what about starting without a prior cf api call and introduce this again in case we get complains?

My point is: the less code we have the easier it is to maintain. Here we have to find a way how we can deal with additional properties set on a command.Command. On the same time we have to ensure that a shared instance of command.Command is not manipulated inside a Login call. This is currently the case inside LoginCheck since we have to get stdout content from the cf apicall. Without that our life would be easier and the probability of making mistakes would be less.

I don't say we have to remove that LoginCheck in any way, but I think we should re-iterate if having that is justified in the light of having a small and easy to maintain code base.

[marcusholl]

For a way without a global instance of command.Command see #1771

[dominiklendle]

Why is it important for our use cases to know if a user has been logged in already?
As mentioned earlier, it is there for the ease of development and to reduce the "login complexity". If there is no such check you have to perform the login every single time when using these helper functions.

The reason behind this is due to the outsourcing of cf helper functions to pkg/cloudfoundry.
If pkg/cloudfoundry gets extended by more Cloud Foundry specific methods that can all be called separately there needs to be some way to conveniently check if the user is already logged in in my eyes. Otherwise you would either need to perform a "hard login" with each method call or leave it all out and leave the "stay logged in" part to the developer. It wouldn't make sense for me to leave all this to the developer using these pkg/cloudfoundry helper methods. And a "hard login" with each method call would be rather log output heavy to me (at least due to the current implementation of Login function). Especially if you call more than one helper methods, the logs will not be polluted that much because of the missing login with each call. With the current implementation, there is a simple check with cf api to reduce the log output and the login complexity.

Your call stack would make perfect sense for a single usage of these helper methods. With more than one call of these helper functions, IMO: logs from cf api login check < logs and load from cf login (both are logs on our side that are currently produced/retrained by cf cli)

I totally get your points, the less code the easier the easier it is to maintain. For me, both options are valid:

• perform a "hard login" with each helper method using cf cli
• having a cf login check function without ability to manipulate shared instance of command.Command

Let's discuss this in todays call!

Either ways, even with leaving the login check function out there should be appropriate logs coming with the Login function that we should also discuss about.

[marcusholl]

@dominiklendle : In case we agree on not having a "global" command.Command instance (see other PR #1771) maybe we can add a boolean flag to our struct (acting a receiver) indicating that there was already a successful login. In that case we can remember the state there and avoid subsequent logins. With that there would be no need to an additional cf api. Just as an idea ...

[dominiklendle]

I think this would be the best option. This even avoids subsequent cf api calls since there is already a boolean flag called cfLogoutOption for the ability to stay logged in. Good idea 👍

To answer your original question: After hearing and discussing this, let's drop the LoginCheck and use your idea mentioned above. You convinced me. Also makes life easier

[dominiklendle]

Changes look good to me, although as discussed yesterday I would propose to rather use the changes from PR #1771 . Sounds more promising to me. Is this PR obsolete then?

[marcusholl]

superseded by #1771