Avoid raft change when no config is provided on persistNewRootAndConfig

[jorgemarey]

I saw in the consul logs that every 10 minutes:

Feb  8 20:07:08 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:07:08.217605Z"}
Feb  8 20:17:14 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:17:14.111028Z"}
Feb  8 20:27:34 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:27:34.153630Z"}
Feb  8 20:37:37 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:37:37.123134Z"}
Feb  8 20:46:14 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:46:14.057424Z"}
Feb  8 20:56:18 ctrl-5c33edfc-2b {"@level":"info","@message":"updated root certificates from primary datacenter","@module":"agent.server.connect","@timestamp":"2022-02-08T20:56:18.133358Z"}

I checked that actually no change was made to the roots or configuration so I started to look at the code and I think I found the problem.

// secondaryCARootWatch maintains a blocking query to the primary datacenter's
// ConnectCA.Roots endpoint to monitor when it needs to request a new signed
// intermediate certificate.
func (c *CAManager) secondaryCARootWatch(ctx context.Context) error {
....
		// Attempt to update the roots using the returned data.
		if err := c.secondaryUpdateRoots(roots); err != nil {
			return err
		}
....

	return nil
}

// secondaryUpdateRoots updates the cached roots from the primary and regenerates the intermediate
// certificate if necessary.
func (c *CAManager) secondaryUpdateRoots(roots structs.IndexedCARoots) error {
	// Update the state first to claim the 'lock'.
	if _, err := c.setState(caStateReconfig, true); err != nil {
		return err
	}
	defer c.setState(caStateInitialized, false)

	// Update the cached primary roots now that the lock is held.
	c.secondarySetPrimaryRoots(roots)

	provider, _ := c.getCAProvider()
	if provider == nil {
		// this happens when leadership is being revoked and this go routine will be stopped
		return nil
	}

	// Run the secondary CA init routine to see if we need to request a new
	// intermediate.
	if c.secondaryHasProviderRoots() {
		if err := c.secondaryInitializeIntermediateCA(provider, nil); err != nil {
			return fmt.Errorf("Failed to initialize the secondary CA: %v", err)
		}
		return nil
	}
...
}


func (c *CAManager) secondaryInitializeIntermediateCA(provider ca.Provider, config *structs.CAConfiguration) error {
....
	// Determine whether a root update is needed, and persist the roots/config accordingly.
	var newRoot *structs.CARoot
	if activeRoot == nil || needsNewIntermediate {
		newRoot = newActiveRoot
	}
	if err := c.persistNewRootAndConfig(provider, newRoot, config); err != nil {
		return err
	}

	c.setCAProvider(provider, newActiveRoot)
	return nil
}

persistNewRootAndConfig is called throught the replication process with nil config and roots when no change needs to be performed.

With this PR, a change to the raft store is avoided when no roots or config are provided to persistNewRootAndConfig

[dnephin]

Thank you for the PR! I think you are correct, we can avoid this unnecessary raft apply, nice find!

This comparison is already pretty challenging to read. I think we'd benefit from extracting this to a function. That allows us to break up the logic a little bit like this:

(writing this quickly, so the naming and logic may not be right)

func shouldPersistRootAndConfig(root *structs.CARoot, prevConfig, nextConfig *structs.CAConfiguration) bool {
    if root != nil {
        return true
    }

    if nextConfig == nil {
        return false
    }

    return nextConfig.Provider != prevConfig.Provider || !reflect.DeepEqual(nextConfig.Config, prevConfig.Config)
}

It would also be great to have a test case that shows this change fixes the problem. I guess that updating the config in the primary in such a way that changes the CARoot struct, but does not create a new root cert would accomplish that. This #11811 (comment) mentions one case I noticed where this can happen.

[jorgemarey]

Hi @dnephin I made the suggested changes and added a test case for this. I also added a chagelog file.

[dnephin]

Thanks for adding the test! Sorry it has taken me a few weeks to respond.

This is looking great. I think the test case is close, but doesn't quite exercise the behaviour we want yet. I left a few suggestions for how to remove some things we no longer need, and to better test the behaviour that was changed.

[dnephin]

Just calling this out for anyone else reviewing or reading over this PR.

This condition is the new one that fixes the problem. Previously we were checking if config != nil to prevent reading a field from a nil. Here we instead say that if the config is nil then we know there is nothing to update, otherwise use the existing logic to see if the config has changed.

[jorgemarey]

Hi @dnephin I pushed a new commit with the suggested changes. I hope I have understood everything correctly.

[dnephin]

Nice! This is exactly what I was thinking. I think with these two small changes this should be ready to merge. I'll commit these suggestions and see if CI is happy.

[dnephin]

Looks like CI is happy with those changes. Thanks for making this improvement!

[hc-github-team-consul-core]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/601036.