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.

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

feat(authentication): support for exec kube config #29

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

portellaa
Copy link
Contributor

@portellaa portellaa commented Mar 24, 2023

Hello,

I saw this issue #5 and i remembered that i had locally a branch with this, so i decide to clean it up a little bit and give it some makeup 💄 and open a PR with it.

I have been using this privately, mostly because i'm not totally proud of it, as long as it works and there isn't much more to do, but please shout your ideas, suggestions, ...

Ideally there are a few validations that need to be done, as which version of the client.authentication.k8s.io resource should be used to decode.
Although the only resources versions that are available for now, are equal, which are client.authentication.k8s.io/v1 and client.authentication.k8s.io/v1beta1, so it works for now.

Not sure what is your preference, but probably this ydataai@c2ded62#diff-ff7d732f3cc6e1355f72ed08c4c685417df831d6e7cfedb605418b494a87711bR361 should be moved into the models package.
I only use this for development, to connect remotely to AWS clusters when i need, in-cluster they use the environment inject into the Pod, so this is not necessary.

I'm more then welcome to change whatever is necessary 😊

Thanks 🍻

PS: @iabudiab and all the involved in it great work on the CRD support and so on 👏
I know i was suppose to help and discuss and suddenly disappeared (sorry 😞), but unfortunately and working in a startup i didn't had time to dedicate to this, i haven't using that much Swift on the core, unfortunately 😢 , neither use your recent work, but i pretend to use this CRD support some day soon. 🙏

@portellaa portellaa force-pushed the feat/support-exec-auth branch from 3443611 to 98e9aae Compare March 24, 2023 12:59
@t089
Copy link
Contributor

t089 commented Mar 24, 2023

I think you mean issue #5.

Nice patch, I've also worked around this. I will pull up my implementation and share some feedback. I think the Process api of foundation has many problems and you really need to be careful not to run into potential dead locks or hangs.

@portellaa
Copy link
Contributor Author

I think you mean issue #5.

oh yes, thanks 🤦

Nice patch, I've also worked around this. I will pull up my implementation and share some feedback. I think the Process api of foundation has many problems and you really need to be careful not to run into potential dead locks or hangs.

Happy to get feedback on that.
Like i said, i don't use this in prod, it's only for development and it's rare when i connect to dev cluster, so i didn't put too much effort or even testing, although never had a problem and have been using this for sometime.

@t089
Copy link
Contributor

t089 commented Mar 25, 2023

So, if I remember correctly, I also tried many different ways implementing Foundation.Process correctly. For this specific use case which is mostly used in local dev mode (eg using aws to obtain an access token for EKS) your implementation should work.

But it is quite fragile.

// this is an async method, the task will run in the background
try task.run()

// this will return the data that is currently available in the read buffer
// of the `FileHandle`. It might be all the data you need, but it might also be less.
return pipe.fileHandleForReading.availableData

It is better to use task.waitUntilExit() this will block the current thread until the sub-process exists.

// this is an async method, the task will run in the background
try task.run()

// Block until the process exits
task.waitUntilExit()

// this will return the data that is currently available in the read buffer
// of the `FileHandle`. It might be all the data you need, but it might also be less.
return pipe.fileHandleForReading.availableData

BUT: Some processes produce lots of output and only exit once somebody actually reads the output. If the output does not fit into the internal buffers, waitUntilExit will block forever and we never get a chance to read the output produced by the sub-process.

So, the safest way to make sure that the process can run and you collect all the output, is to read the output from a separate thread. This is the solution I landed on and which has been working reliably in production (for a different use-case than here). With all other approaches I had occasional hangs of the program which required restarting the program altogether.

import Foundation

enum Process {

    enum ShellError: Error {
        case failure(terminationStatus: Int, errorMessage: String?, arguments: [String])
        case missingExecutable(name: String)
    }

    private  static func getEnvSearchPaths(
        pathString: String?,
        currentWorkingDirectory: URL?
    ) -> [URL] {
        // Compute search paths from PATH variable.
        let pathSeparator: Character = ":"

        return (pathString ?? "").split(separator: pathSeparator).map(String.init).compactMap({ pathString in
            if let cwd = currentWorkingDirectory {
                return URL(fileURLWithPath: pathString, relativeTo: cwd)
            }
            return URL(fileURLWithPath: pathString)
        })
    }


    private static func findExecutable(_ name: String) -> URL? {
        if FileManager.default.isExecutableFile(atPath: name) {
            return URL(fileURLWithPath: name)
        }
        
        let paths = getEnvSearchPaths(pathString: ProcessInfo.processInfo.environment["PATH"], currentWorkingDirectory: nil)
        
        return paths.lazy
            .map { URL(fileURLWithPath: name, relativeTo: $0) }
            .first {
                FileManager.default.isExecutableFile(atPath: $0.path)
        }
    }

    public static func  execute(_ arguments: [String], env: [String: String] = ProcessInfo.processInfo.environment) throws -> Data? {
        guard arguments.count > 0 else {
            fatalError("arguments must not be empty")
        }

        guard let executable = findExecutable(arguments[0]) else {
            throw ShellError.missingExecutable(name: arguments[0])
        }

        var outputData : Data?
        var errorData : Data?

        let group = DispatchGroup()

        let task = Foundation.Process()
        task.executableURL = executable
        task.arguments = Array(arguments.dropFirst())
        let outputPipe = Pipe()
        let errorPipe = Pipe()
        

        task.standardOutput = outputPipe
        task.standardError = errorPipe

        try task.run()
        group.enter()
        Thread { // read full output in a separate thread
            outputData = try? outputPipe.fileHandleForReading.readToEnd()
            group.leave()
        }.start()
        
        group.enter()
        Thread {  // read full error output in a separate thread
            errorData = try? errorPipe.fileHandleForReading.readToEnd()
            group.leave()
        }.start()
        

        task.waitUntilExit()

        // wait until the reader threads complete
        if .timedOut == group.wait(timeout: .now() + .seconds(10)) {
            fatalError("Task exited, but timed out waiting for output.")
        }

        guard task.terminationStatus == 0 else {
            let message = errorData.flatMap { String(data:  $0, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) } ?? ""
            throw ShellError.failure(terminationStatus: Int(task.terminationStatus), errorMessage: message, arguments: arguments)
        }

        return outputData
    }
}

@iabudiab
Copy link
Member

@portellaa

PS: @iabudiab and all the involved in it great work on the CRD support and so on 👏
I know i was suppose to help and discuss and suddenly disappeared (sorry 😞), but unfortunately and working in a startup i didn't had time to dedicate to this, i haven't using that much Swift on the core, unfortunately 😢 , neither use your recent work, but i pretend to use this CRD support some day soon.

Don't worry about it, and you don't have to apologise. We all have the same problem with free time 😉

Speaking of free time, I'll try to take a look at this PR in the next couple of days 😅

And big thanks to you and to @t089 for all the work you've done!

@iabudiab
Copy link
Member

iabudiab commented Jun 4, 2023

@t089 @portellaa

Have you tried to play around with FileHandle/fileHandle.readabilityHandler?

Instead of starting two threads this should create a dispatch source, that listens to the incoming data at the file handle, i.e.:

let task = Process()
task.executableURL = URL(fileURLWithPath: command)
task.arguments = arguments

let pipe = Pipe()
let fileHandle = FileHandle(fileDescriptor: STDOUT_FILENO)
fileHandle.readabilityHandler = { handle in
    let data = handle.availableData
    if data.count > 0 {
       pipe.fileHandleForWriting.write(data)
    }
}
task.standardOutput = pipe

try task.run()
task.waitUntilExit()

@t089
Copy link
Contributor

t089 commented Jun 4, 2023

Yes, I have been playing around with this, but found that this does not work as expected on linux. It can work reliably on macOS but I had all kinds of unexpected behaviour on linux that's why I changed to the "brute force" method of spawning two threads and read until EOF. This has been working reliably.

Relatedly, Swift Tool Support Core funnily completely skips Foundation.Process and implements Process in terms of POSIX API, but in the end also spawns threads to handle the output: https://github.com/apple/swift-tools-support-core/blob/033ab451dcbb525de4dc9eaf506fdbc30812de28/Sources/TSCBasic/Process.swift#L748

@portellaa portellaa force-pushed the feat/support-exec-auth branch from 98e9aae to a442a51 Compare July 6, 2023 10:28
@portellaa
Copy link
Contributor Author

@t089 @portellaa

Have you tried to play around with FileHandle/fileHandle.readabilityHandler?

Instead of starting two threads this should create a dispatch source, that listens to the incoming data at the file handle, i.e.:

let task = Process()
task.executableURL = URL(fileURLWithPath: command)
task.arguments = arguments

let pipe = Pipe()
let fileHandle = FileHandle(fileDescriptor: STDOUT_FILENO)
fileHandle.readabilityHandler = { handle in
    let data = handle.availableData
    if data.count > 0 {
       pipe.fileHandleForWriting.write(data)
    }
}
task.standardOutput = pipe

try task.run()
task.waitUntilExit()

Hi @iabudiab

Sorry for the delay again 😞

No i hadn't.
This has been working for me on Debian based distros, so i'm not giving this too much attention, i'm sorry.
It is indeed mostly for local dev mode use, but even on my remote containers or other linux test/dev environments, we don't have problems with this.
I've been using this for almost a year, which is not much, for sure.

If this is to apply @t089 solution, close this PR and open on your behalf with that approach, fine by me, let's just go ahead with this, may be another person in the world needing this.
Honestly doesn't make sense to me to replace everything with your solution, even though i'm not having issues with mine in two environments (didn't testes in windows though).

Cheers 🍻

@iabudiab iabudiab merged commit 88d3550 into swiftkube:main Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants