Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Pass kubeconfig content around rather than file path #631

Merged
merged 9 commits into from
Aug 3, 2020
Merged

Pass kubeconfig content around rather than file path #631

merged 9 commits into from
Aug 3, 2020

Conversation

invidian
Copy link
Member

This PR includes commits from #628, please review this one first and don't leave commit comments for this PR here.

This PR changes a lot of the code to accept kubeconfig content rather than kubeconfig path, so we will be able to read it from Terraform state/output in the future, which is needed for #608. It also shows, how poorly the code is structured, as such change requite so much code changes. We should make most of this function to execute in the scope/context (so make them have a receiver), which already has access to the kubeconfig content, as the kubeconfig content is almost constant in calls of this functions.

Please review the PR commit by commit, as each of them contains small, logical and independent changes, described a bit in commit message.

Now that reading kubeconfig file is in single place, we can implement reading it from Terraform state.

Closes #623

Part of #608

@invidian invidian requested review from ipochi and surajssd as code owners June 16, 2020 10:02
@invidian invidian force-pushed the invidian/kubeconfig-content-backup branch from 7f82e75 to 57a8710 Compare June 23, 2020 09:18
@invidian invidian mentioned this pull request Jul 1, 2020
Merged
@invidian invidian force-pushed the invidian/kubeconfig-content-backup branch 3 times, most recently from 530b04a to 2dae3ce Compare July 1, 2020 18:36
@invidian
Copy link
Member Author

invidian commented Jul 2, 2020

After this PR, last step is to read kubeconfig file content from Terraform state. This requires:

  • changing getKubeconfig to also read from Terraform output
  • if one runs lokoctl health with cluster configuration, Terraform files needs to be dumped and Terraform binary must be present on the system.

@invidian invidian requested review from iaguis and johananl July 3, 2020 07:19
@invidian invidian mentioned this pull request Jul 3, 2020
Merged
2 tasks
@invidian invidian force-pushed the invidian/kubeconfig-content-backup branch from 2dae3ce to e9b259a Compare July 23, 2020 11:42
invidian added 9 commits July 30, 2020 10:39
@invidian
pkg/k8sutil: add Getter helper struct
c67f23e 
Getter struct and NewGetter function is a replacement for
helm.sh/helm/v3/pkg/kube.GetConfig(), which allows to create Helm action
config from the content of kubeconfig file, instead of from the path of
the file, which is part of #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components/util: make HelmActionConfig use kubeconfig file content
ecfd727 
As part of #608, this commit changes the implementation of
HelmActionConfig function to read the content of given kubeconfig file
and then use the content to construct Helm's actionConfig. This will
make easier changing the function signature to accept content of
kubeconfig file in the future changes.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: move deleteHelmRelease to pkg/components/util
98b1d2f 
As component-oriented functionality should be stored in pkg/components
and cli package should not go too much into helm details etc.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components/util: make HelmActionConfig take kubeconfig content
bf1ce37 
Instead of kubeconfig file path. This simplifies the tests and moves us
closer towards resolving #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: don't read kubeconfig file twice when running cluster apply
4d59cbc 
Instead, change verifyCluster function to take content of kubeconfig
file as an argument, which caller should already have.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components/util: make UninstallComponent take kubeconfig content
434b7bb 
So we don't necesserily need a file on file system to perfrom this
action, which is needed to resolve #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
pkg/components/util: make InstallComponent take kubeconfig content
88d87d3 
This commit changes InstallComponent and InstallAsRelease methods to
accept kubeconfig content, rather than the file path, to allow loading
it from places other than local filesystem.

Part of #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: remove check if kubeconfig exists from health command
8a5c4d8 
The subcommand function will fail anyway while trying to read kubeconfig
file, so there is no need to check it twice.

Also part of #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: refactor getKubeconfig() to return kubeconfig file content
b65cf5b 
Rather than kubeconfig file path. This will allow changing this
function to pull kubeconfig content from Terraform output rather than
from assets directory, which will resolve #608.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian invidian force-pushed the invidian/kubeconfig-content-backup branch from e9b259a to b65cf5b Compare July 30, 2020 08:39
ipochi
ipochi reviewed Jul 30, 2020
View reviewed changes
Copy link
Member

@ipochi ipochi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commits that move code from from cli to util package such as deleteComponent.
Its great that you have taken this out of cli package but I have reservations about this in this PR.

cli/cmd/component-delete.go Show resolved Hide resolved
@invidian
Copy link
Member Author

Commits that move code from from cli to util package such as deleteComponent.
Its great that you have taken this out of cli package but I have reservations about this in this PR.

Do you mean that I move deleteHelmRelease to pkg/components/util package, rather than to /pkg/components for example? I'd say this is on purpose for now, to keep all Helm-related functionality in one place. Later on, we should move all of that to pkg/components or to pkg/components/helm, but I'd do that separately.

@invidian invidian requested review from ipochi and knrt10 July 31, 2020 08:12
knrt10
knrt10 approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @invidian. And by previous comment for @ipochi I think he meant moving code out of cli package might not be suitable for this PR.

@invidian
Copy link
Member Author

Looks great @invidian. And by previous comment for @ipochi I think he meant moving code out of cli package might not be suitable for this PR.

Ah, that make sense. If you insist, that this shouldn't be part of this PR, I'll remove it, but I don't see it doing much harm in the existing state.

@knrt10
Copy link
Member

knrt10 commented Jul 31, 2020 •
edited
Loading

Yea, I would say no need to remove it now, this looks completely fine

@ipochi
Copy link
Member

ipochi commented Aug 3, 2020 •
edited
Loading

Looks great @invidian. And by previous comment for @ipochi I think he meant moving code out of cli package might not be suitable for this PR.

Ah, that make sense. If you insist, that this shouldn't be part of this PR, I'll remove it, but I don't see it doing much harm in the existing state.

If its a small thing to do, then I'd like that to keep them separate. However if its going to be bit more than tiny overhead, its fine as is.

I'll let you make the call.

@invidian
Copy link
Member Author

invidian commented Aug 3, 2020

If its a small thing to do, then I'd like that to keep them separate. However if its going to be bit more than tiny overhead, its fine as is.

I'll let you make the call.

Unfortunately, it won't be a tiny thing, as moved function is changed afterwards, so I'll just keep it as it is and merge. Thanks!

@invidian invidian merged commit 863a7a9 into master Aug 3, 2020
@invidian invidian deleted the invidian/kubeconfig-content-backup branch August 3, 2020 06:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@knrt10 knrt10 knrt10 approved these changes

@surajssd surajssd Awaiting requested review from surajssd

@iaguis iaguis Awaiting requested review from iaguis

@johananl johananl Awaiting requested review from johananl

@ipochi ipochi Awaiting requested review from ipochi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use kubeconfig content rather than kubeconfig path between functions
3 participants
@invidian @knrt10 @ipochi