Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

Forward port RPC credentials rotation on upgrade #2595

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

a-palchikov
Copy link
Contributor

@a-palchikov a-palchikov commented Jul 30, 2021

Description

Forward port of #1749 to bring feature parity with 7.x.
Part of a bigger attempt to forward-port intermediate version support to 9.x.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Internal change (not necessarily a bug fix or a new feature)

Linked tickets and other PRs

  • Closes #, refs #

TODOs

  • Self-review the change
  • Write tests
  • Perform manual testing
  • Address review feedback
  • Update upstream references / tags / versions after upstream PR merges (linked above)

Implementation

Performance/Scaling

Testing done

  1. Install a 3-node cluster
  2. Attempt to upgrade a 3-node cluster from base version 8.0.0-beta.1.5
    2.1. Trigger manual upgrade
    2.2. Step over /init to rotate RPC credentials
    2.3. Roll back the operation (to also restore credenetials)
  3. Upgrade a 3-node cluster from base version 8.0.0-beta.1.5

Additional information

@a-palchikov a-palchikov requested review from a team, wadells and knisbet July 30, 2021 16:04
Copy link
Contributor

@wadells wadells left a comment

Choose a reason for hiding this comment

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

Does this need to go to 8.0.x too? 7.0 -> 9.0 isn't possible with a HA config, based on my understanding.

Code looks reasonable -- but I'm not familiar with the original patches.

Testing?

lib/localenv/clusterenv.go Show resolved Hide resolved
if path == "" {
return nil, trace.BadParameter("missing Path parameter")
// New creates a new instance of the local fs blob service
// rooted as the given path
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// rooted as the given path
// rooted at the given path

return p.HostLocalBackend.SetServiceUser(p.ServiceUser.OSUser())
}

func (p *updatePhaseBootstrap) pullSystemUpdates(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

ctx seems unused?

@a-palchikov
Copy link
Contributor Author

Update the PR description with test steps performed.

Copy link
Contributor

@knisbet knisbet left a comment

Choose a reason for hiding this comment

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

Just looked through quickly.

return b.upsertVal(b.key(systemP, nodeAddrP), addr, forever)
}

// GetServiceUser returns the current serviceo user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// GetServiceUser returns the current serviceo user
// GetServiceUser returns the current service user

@@ -50,3 +50,33 @@ func (b *backend) GetSELinux() (enabled bool, err error) {
func (b *backend) SetSELinux(enabled bool) error {
return b.upsertVal(b.key(systemP, seLinuxP), &enabled, forever)
}

// GetNodeAddr returns the current node advertise IP
func (b *backend) GetNodeAddr() (addr string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is probably just part of the port and already this way on other branches, I imagine it applies to just the local backend. I just find it funny, because the other backend is etcd, in which case each node would clobber others writes.

Not asking for any changes, just something I found interesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree - moved the system metadata APIs to a separate interface in c059b97e

a-palchikov added a commit to a-palchikov/gravity that referenced this pull request Aug 17, 2021
@a-palchikov a-palchikov merged commit 513c7c4 into master Sep 10, 2021
@a-palchikov a-palchikov deleted the dmitri/1623-rotate-rpc-creds branch September 10, 2021 10:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants