-
Notifications
You must be signed in to change notification settings - Fork 1
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
Disconnect cluster #3470
Disconnect cluster #3470
Conversation
ba07c77
to
7be2c39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I've run it and it seems to work!
cmd/gitops/app/disconnect/cmd.go
Outdated
Example: ` | ||
# Disconnect a cluster | ||
gitops disconnect cluster`, | ||
PreRunE: func(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could extract this to a function and reuse it across both commands that don't talk to the API?
Say something like disinheritAPIFlags
?
"github.com/weaveworks/weave-gitops/cmd/gitops/config" | ||
) | ||
|
||
func Command(opts *config.Options) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exported, so it should have a Doc comment? see above for DisconnectCommand
too :-)
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments
pkg/cluster/connector/connector.go
Outdated
dynClient, err := dynamic.NewForConfig(config) | ||
if err != nil { | ||
return "", err | ||
return nil, nil, err | ||
} | ||
scheme, err := NewGitopsClusterScheme() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewGitopsClusterScheme
isn't used outside the connector package so it doesn't need to be exported?
pkg/cluster/connector/connector.go
Outdated
} | ||
|
||
newLabels := map[string]string{ | ||
"clusters.weave.works/connect-cluster-service-account": options.ServiceAccountName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work.
Flux will reset the labels to whatever is in the Git repository.
// Verify ClusterRoleBinding deleted | ||
_, err = remoteClientSet.RbacV1().ClusterRoleBindings().Get(context.Background(), tt.clusterRoleBindingName, metav1.GetOptions{}) | ||
assert.Error(t, err) | ||
assert.ErrorContains(t, err, apierrors.NewNotFound(rbacv1.Resource("clusterrolebindings"), tt.clusterRoleBindingName).Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.True(apierrors.IsNotFound(err))
https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors#IsNotFound expresses what you want better.
You are explicitly loading a ClusterRoleBinding
so the test can assert that you get a "not found" without caring too much about the detail.
0cef52d
to
ff4f328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
…in cluster connector package Fix DisconnectCluster function to include retrieving service account name and cluster role binding name and delete resources accordingly
…deleting service account results in its deletion
…s disconnect cmd Add check service account name and check cluster role binding name functions to verify their existance with the connect-cluster label
change deleteServiceAccountResources to not be exported
…instead of cluster-controller
… using helper function in service account tests
Verify not found errors by just comparing the type of error to be not found
ff4f328
to
c74f9c8
Compare
* Add delete service account resources * Test pass for delete service account resources * Add label managed-by to resources when being created during reconciliation * Fix DisconnectCluster function to include retrieving service account name and cluster role binding name and delete resources accordingly * Add gitops disconnect cluster cmd * Add service account name and cluster role binding name flags to gitops disconnect cmd Add check service account name and check cluster role binding name functions to verify their existance with the connect-cluster label * Add existing resources to simpleClientSet when creating it instead of using helper function in service account tests * Make newGitopsClusterScheme unexposed * Move prerun disinheritApiFlags to new function in common app pkg in cmd gitops pkg dir * update go.mod
* Fix naming of KubeConfig cluster for empty context. When the context is "" i.e. the default context, this was generating odd names for the KubeConfig elements `-user` etc. This ensures that we put something in for the context. * Add creating a role or binding client object for delete transactions manually in case of an "object not found" error. Add sending delete object transactions for deleted roles and bindings in case of an "object not found" error. Add `IsValidID` method to role to be able to delete roles without policy rules (those, which are manually constructed to pass with delete transactions). Update corresponding tests in `reconciler`, `rolecollector`, and `store` packages. * fix: Minor typo * Bump version of weave-gitops to latest (#3471) Signed-off-by: wge-build-bot <[email protected]> Co-authored-by: wge-build-bot <[email protected]> Co-authored-by: Ahmad Samir <[email protected]> * Add bootstrap command for gitops cli to bootstrap WGE (#3371) * init gitops bootstrap command for wge * add checks for entitlement and flux * Add wge version choise list * add create admin username and password secret * add install wge * fix adding admin password * fix linting * run go mod tidy * enable ingress * add domain type selector * add option to bootstrap flux * adjust bootstraping flux and domain * add install extra controllers * fix lint * rename package checks to commands * refactor packages * early exit * refactor errors and add utils for git repos * refactor wge installation to use files * refactor extra controllers * remove extra unneeded values * Add OIDC * update portforward * add admin password revert * refactor error handeling * fix unhandeled errors * refactor creating helmrepos and helmreleases * refactor styling * go mod tidy * split utils * Add OIDC * refactor creating extra controllers * move install extra controller to commands * add oidc * Add OIDC * install capi controller * add terraform controller * refactor OIDC cli * fix linting * fix lintting * fix lint * fix lint * update OIDC CLI * handle error msgs * move check mark to method * reformat error * cleanup * update CLI OIDC * fix confirm input * type * Add localhost portforward info * add gitopssets controller * enable pipelines controller and cluster-controller and gitopssets by default * remove gitops sets * handle existing secrets * Update onboarding CLI messages * cleanup utils * cleanup utils * cleanup domain * cleanup utils * cleanup input * cleanup input * edit messages and variableNames * update cli messages * cleanup variables * cleanup constants * cleanup constants * add unit test for k8s utils and refactor accordingly * add unit tests for flux * add unit test for admin password * Add unit -test for adding capi & policy-agent * move controllers under gitops add * add unit test for wge version * test oidc get issuer * Move controllers under gitops add * Move controllers under gitops add * pause add controllers tests * restore install controller function after setup * fix controllers * pass opts to controllers * refactor git utilits * refactor git utilities * prepare bootstrap bommand for release1 * address messages and languague * clean extra variables * remove oidc flow to another branch * Update cmd/gitops/app/bootstrap/cmd.go Co-authored-by: Eneko Fernández <[email protected]> * Update cmd/gitops/app/bootstrap/cmd.go Co-authored-by: Eneko Fernández <[email protected]> * refactor git utility & add unit-test * remove out of scope componenets * apply code review comments to refactor file names and error messages * move bootstrap package under pkg * refactor using the k8s client and add silent mode refactor git utils * fix admin password * verify username and password * remove unused methods * fix spelling * remove aws related stuff * use git library add check for previous installation * fix lint * imporve error messages to stage failures * adjust gitopssets values and installation checks * clean silent mode * add checks for kubeconfig * adjust admin password * refactor commands to use config interface struct * improve error messages * remove unused variable * wip adding cli design doc * wip adding cli design doc * add error guidance * rename config to bootstrapper to be more clear * refactor commands to use steps pattern * continue on refactor commands to use steps pattern * fix ssh authenticate by explictly asking for private key file in case the key is not loaded in ssh agent. this usually happens on macos * added integration test, refactored configuration and design (#3458) * Changes after the review to enhance the following aspects Testing: - Added integration test so we could test the functionality e2e: it uses some local configuration that we need to test but already provides the acceptance layer that we were missing: Design: - Bootstrap workflow moved to the domain layer within `pkg` so it could be presented in different forms. - Integrated configuration chain of responsibility into a single a builder pattern, so we have configurability in this layer. As a result: - we dont need to pass the flags to the steps - we config the stepsbefore the workflow is executed which seems the right moment. Other refactors: - Moved steps to package `steps` from `command` for consistency * integrated ssh key management * add events and error messages and fix domain bug * add current context * fix lint * add entitlement expiration message * fix entitlement expiration * fix entitlement expiration * seperate entitlement secret validations * add messages for flux * edit messages to small letter and update success messages * add validation for password and wge version * add test cases for admin password create creds * add test cases for domain type * update entitlement test * add validation on password input * remove aws related message * panic in case of casting error to give more context about the error * handle portforward and error messages * fix external dns spacing * add validation on domain type * Cli eneko review (#3474) * reviewed documentation * updated docs with waleed input * reviewed TBD * latest set of changes * removing commented * remove debugging * removed stale documentation * removed withe space * review * removed unused --------- Co-authored-by: Ahmad Samir <[email protected]> Co-authored-by: Eneko Fernández <[email protected]> Co-authored-by: Eneko Fernandez <[email protected]> * build(deps): Ensure protoc-gen-grpc-gateway-ts is installed when running `make dependencies` * fix: Remove action that configured AWS credentials for use against an EKS test cluster. * fix wge cli bootstrap command description (#3479) * fix wge cli bootstrap command description * capital letter --------- Co-authored-by: Eneko Fernandez <[email protected]> * Add eslintrc file from OSS and fix resulting errors (#3346) * disable all testing rules, tests need to be reworked * spyOn yaml.parse instead of mock * Added react-hooks plugin to eslint * specify groups for import ordering * rebase * uneccessary package * add url to font imports, ts ignore correct parcel imports of oss types in auth buttons * more extra packages * add linting to ui-unit-tests job in github workflow * pesky pipeline details snap * maybe the snap from main will do * re-deleting files that got added in a rebase * fix list events import in gitopssets detail --------- Co-authored-by: Jordan Pellizzari <[email protected]> * Use environment-specific promotion strategies in Pipelines UI (#3446) * different field for target number * add promotion to protobuf for environment * getStrategy func * go? * update strategy text tests * ....sigh....snapshots * fix: Update test * something horrible has happened to the pipeline details snapshot test and i hate it * just as i suspected the snap needed to be updated again * pipeline details snap from main who knows where i went wrong * welp once again i have updated the evil pipeline details snap * import order * okay im deleting the snapshot bc not only is it annoying but the entire ui is changing and i will make a new one later --------- Co-authored-by: Yiannis <[email protected]> * Disconnect cluster in gitops (#3470) * Add delete service account resources * Test pass for delete service account resources * Add label managed-by to resources when being created during reconciliation * Fix DisconnectCluster function to include retrieving service account name and cluster role binding name and delete resources accordingly * Add gitops disconnect cluster cmd * Add service account name and cluster role binding name flags to gitops disconnect cmd Add check service account name and check cluster role binding name functions to verify their existance with the connect-cluster label * Add existing resources to simpleClientSet when creating it instead of using helper function in service account tests * Make newGitopsClusterScheme unexposed * Move prerun disinheritApiFlags to new function in common app pkg in cmd gitops pkg dir * update go.mod * Fixes the prod build of the UI loading (#3486) * Fixes the prod build of the UI loading - We've bumped into the YAML import issues again - Some part of the parcel build pipeline (minify?/pack?) seems to have trouble w/ this particular YAML package. First noticed in the intial migration from webpack to parcel * Downgrade git-url-parse to fix another parcel build issue * Update yaml view props for new oss version (#3489) * update yaml view for new oss version * lint * import notifications from OSS * move AlertListErrors to OSS * update snaps * update snap --------- Signed-off-by: wge-build-bot <[email protected]> Co-authored-by: Kevin McDermott <[email protected]> Co-authored-by: opudrovs <[email protected]> Co-authored-by: Yiannis Triantafyllopoulos <[email protected]> Co-authored-by: wge-build-bot <[email protected]> Co-authored-by: wge-build-bot <[email protected]> Co-authored-by: Ahmad Samir <[email protected]> Co-authored-by: Waleed Hammam <[email protected]> Co-authored-by: Eneko Fernández <[email protected]> Co-authored-by: Eneko Fernandez <[email protected]> Co-authored-by: Yiannis <[email protected]> Co-authored-by: Joshua Israel <[email protected]> Co-authored-by: Jordan Pellizzari <[email protected]> Co-authored-by: Rana Tarek Hassan <[email protected]> Co-authored-by: Simon <[email protected]>
Closes #3273
What changed?
gitops disconnect cluster
cmdWhy was this change made?
To be able to disconnect a cluster and delete associated resources after connecting it
How was this change implemented?
Updating connector package and adding required cmd with flags to disconnect a cluster given its context name and cluster name
How did you validate the change?
[ X] Explain how a reviewer can verify the change themselves
failed to get secret
because the secret doesn't exist yet and there is no way for communication to the clusterweave- gitops-enterprise
[ X] Unit tests -- what is covered, what cannot be covered; are
there tests that fail without the change?
Release notes
Disconnect cluster added to gitops cli
Documentation Changes
Other follow ups