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

[etcdbackup&restore] Implement RKE2Config webhook #542

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

furkatgofurov7
Copy link
Contributor

@furkatgofurov7 furkatgofurov7 commented May 20, 2024

What this PR does / why we need it:
Adds RKE2Config webhook implementation

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #498
Part of: #388

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@furkatgofurov7 furkatgofurov7 requested a review from a team as a code owner May 20, 2024 21:24
@furkatgofurov7 furkatgofurov7 force-pushed the add-webhook-logic branch 2 times, most recently from 056468c to a9c1745 Compare May 20, 2024 22:05
@furkatgofurov7 furkatgofurov7 changed the title Add RKE2Config webhook implementation [etcdbackup&restore] Implement RKE2Config webhook May 20, 2024
@furkatgofurov7
Copy link
Contributor Author

@alexander-demicev PTAL

Copy link
Member

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

Thank for working on it, I have a couple of comments

Makefile Show resolved Hide resolved
@@ -0,0 +1,6 @@
package webhooks

var installsh = `
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to embed library here, we can update it later when actual script is implemented

Copy link
Contributor Author

@furkatgofurov7 furkatgofurov7 Jun 3, 2024

Choose a reason for hiding this comment

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

This just a skeleton for installsh.go file and will be changed when actually introducing installsh script


logger.Info("Service account secret is populated")

serverUrlSetting := &unstructured.Unstructured{}
Copy link
Member

Choose a reason for hiding this comment

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

We can use this library instead of unstructured https://github.com/rancher/turtles/tree/main/internal/rancher, we just to make it importable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it importable we need a new go.mod file and dedicated file for this kind (=Setting)? I am not sure if that's worth the effort. I noticed the unstructured usage in few places, i.e https://github.com/rancher/turtles/blob/main/test/framework/rancher_helpers.go#L219-L221

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need to move the webhook pathing code to the internal package of its own, and if possible, move out the internal/rancher to and API level, because it is a public API. Then the use of the generated Setting as a typed resource will be easy and correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, that change seems to be out of scope of this PR and can be discussed separately IMO.

var errs []error

// Create the ServiceAccount first to later pass to the RoleBinding creation
sa := r.createServiceAccount(planSecretName, rke2Config)
Copy link
Member

Choose a reason for hiding this comment

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

We can create it later as we already know the namespace and name to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't know it beforehand. I had to create it first and later refer to it.

Signed-off-by: Furkat Gofurov <furkat.gofurov@suse.com>
Signed-off-by: Furkat Gofurov <furkat.gofurov@suse.com>
…efile target

Signed-off-by: Furkat Gofurov <furkat.gofurov@suse.com>
@furkatgofurov7 furkatgofurov7 enabled auto-merge (rebase) June 11, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: PR to be reviewed
Development

Successfully merging this pull request may close these issues.

[etcdbackup&restore] Implement Rke2Config webhook for installing and configuring system-agent
3 participants