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

Disconnect cluster #3470

Merged
merged 19 commits into from
Oct 12, 2023
Merged

Disconnect cluster #3470

merged 19 commits into from
Oct 12, 2023

Conversation

ranatrk
Copy link
Contributor

@ranatrk ranatrk commented Oct 11, 2023

Closes #3273

What changed?

  • Add gitops disconnect cluster cmd
  • Add disconnectCluster to connector package

Why 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

    • create 2 clusters, a hub cluster and a spoke cluster
    • create a gitopscluster in the hub cluster with the name of the spoke cluster and the intended secret name (status should be failed to get secret because the secret doesn't exist yet and there is no way for communication to the cluster
    • connect the hub cluster with the spoke cluster (default service account name and cluster role binding name is weave- gitops-enterprise
    gitops connect cluster --namespace default --connect-context <SPOKE_CLUSTER_CONTEXT> 
    <SPOKE_CLUSTER_NAME>
    
    • connectivity when listing gitopscluster should be true
    • disconnect the cluster (service account and cluster role binding names are optional)
    gitops disconnect cluster --namespace default --service-account weave-gitops-enterprise --cluster-role-binding weave-gitops- 
    enterprise --connect-context <SPOKE_CLUSTER_CONTEXT> <SPOKE_CLUSTER_NAME>
    
  • [ 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

@ranatrk ranatrk added the enhancement New feature or request label Oct 11, 2023
@ranatrk ranatrk requested a review from a team October 11, 2023 13:56
@ranatrk ranatrk force-pushed the disconnect-cluster branch 5 times, most recently from ba07c77 to 7be2c39 Compare October 11, 2023 15:24
Copy link
Contributor

@bigkevmcd bigkevmcd 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, I've run it and it seems to work!

Example: `
# Disconnect a cluster
gitops disconnect cluster`,
PreRunE: func(cmd *cobra.Command, args []string) error {
Copy link
Contributor

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 {
Copy link
Contributor

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

dynClient, err := dynamic.NewForConfig(config)
if err != nil {
return "", err
return nil, nil, err
}
scheme, err := NewGitopsClusterScheme()
Copy link
Contributor

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?

}

newLabels := map[string]string{
"clusters.weave.works/connect-cluster-service-account": options.ServiceAccountName,
Copy link
Contributor

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.

pkg/cluster/connector/connector.go Outdated Show resolved Hide resolved
// 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())
Copy link
Contributor

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.

@ranatrk ranatrk force-pushed the disconnect-cluster branch 4 times, most recently from 0cef52d to ff4f328 Compare October 12, 2023 13:01
@ranatrk ranatrk requested a review from bigkevmcd October 12, 2023 13:16
Copy link
Contributor

@bigkevmcd bigkevmcd left a 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
… using helper function in service account tests
Verify not found errors by just comparing the type of error to be not found
@ranatrk ranatrk force-pushed the disconnect-cluster branch from ff4f328 to c74f9c8 Compare October 12, 2023 19:29
@ranatrk ranatrk merged commit 3a2548b into main Oct 12, 2023
10 checks passed
@ranatrk ranatrk deleted the disconnect-cluster branch October 12, 2023 20:33
AsmaaNabilBakr pushed a commit that referenced this pull request Oct 15, 2023
* 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
AsmaaNabilBakr added a commit that referenced this pull request Nov 9, 2023
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitOps command to disconnect a cluster
2 participants