swiftkube / client

Swift client for Kubernetes
Apache License 2.0
129 stars 20 forks source link

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

Closed portellaa closed 1 year ago

portellaa commented 1 year ago

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 https://github.com/ydataai/SwiftKubeClient/commit/c2ded6245b156ca8ac6436ffa4fe62bda25c2f4e#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. πŸ™

t089 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 🍻