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

Use metadata field from configEntry #343

Merged
merged 7 commits into from
Oct 1, 2020

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Sep 30, 2020

This will determine if the resource is managed from outside the current cluster

Changes proposed in this PR:

  • Add flag datacenter that is a required flag for the controller for startup. This string will be used in the meta() field of the config entries managed by Consul
  • Add datacenter specific metadata on the Config Entry Meta field before creating the resource in Consul.
    This field will be ignore when checking for equality between resources with cmp.Equal().
  • A config entry will only be update/deleted from Consul if the metadata on it indicates that it was created in the datacenter the controller is running in.

How I've tested this PR:

  • Adding additional acceptance tests to validate this behavior.

How I expect reviewers to test this PR:

  • Ideally, one should be able to use the consul CLI to create config entries an installed Consul. These will not have the meta information set on them. Attempting to create a Custom Resource that shares that same name should lead the the resource getting accepted by the cluster, but will fail reconcile and its status should mention that the sync has failed with a reason indicating the Config is managed externally.
  • Deleting this resource will not have the side-effect of deleting the Config Entry from Consul.

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@thisisnotashwin thisisnotashwin requested review from lkysow, a team and kschoche and removed request for a team September 30, 2020 15:52
api/v1alpha1/proxydefaults_types.go Outdated Show resolved Hide resolved
controller/configentry_controller.go Show resolved Hide resolved
controller/configentry_controller.go Outdated Show resolved Hide resolved
controller/configentry_controller_test.go Outdated Show resolved Hide resolved
controller/configentry_controller_test.go Show resolved Hide resolved
controller/configentry_controller_test.go Show resolved Hide resolved
subcommand/controller/command.go Outdated Show resolved Hide resolved
@thisisnotashwin thisisnotashwin force-pushed the refactor-ent-controller-tests branch from bf0c6c8 to a0c74d7 Compare September 30, 2020 18:46
Base automatically changed from refactor-ent-controller-tests to crd-controller-base September 30, 2020 18:51
api/v1alpha1/types.go Outdated Show resolved Hide resolved
controller/configentry_controller.go Outdated Show resolved Hide resolved
controller/configentry_controller.go Outdated Show resolved Hide resolved
controller/configentry_controller_test.go Outdated Show resolved Hide resolved
controller/configentry_controller_test.go Outdated Show resolved Hide resolved
@lkysow
Copy link
Member

lkysow commented Oct 1, 2020

You'll need to rebase once #344 is merged in. I tried it out and looks like there's a bug in the enterprise logic: #345

Comment on lines 114 to 120
entry, _, err := r.ConsulClient.ConfigEntries().Get(configEntry.ConsulKind(), configEntry.Name(), &capi.QueryOptions{
Namespace: r.consulNamespace(req.Namespace, configEntry.ConsulNamespaced()),
})

if err != nil {
return ctrl.Result{}, fmt.Errorf("deleting config entry from consul: %w", err)
return ctrl.Result{}, fmt.Errorf("getting config entry from consul: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

So this shouldn't actually error out since if it is deleted then it's actually good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Updated this check to ignore notFound errors (I'm time traveling. Ignore timestamps.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for helping with finding the bug BTW. I really appreciate it.

ishustava and others added 6 commits September 30, 2020 23:00
- This ensures we dont have to necessarily write failing tests every
  time we add a field
- Add error message in logs if Consul entry isnt deleted.
- Extract private method to share meta across resources.
Co-authored-by: Luke Kysow <1034429+lkysow@users.noreply.github.com>
@thisisnotashwin thisisnotashwin requested a review from lkysow October 1, 2020 03:14
@thisisnotashwin thisisnotashwin force-pushed the add-metadata-to-crds branch 2 times, most recently from 82d8c9a to cefc24e Compare October 1, 2020 14:35
Comment on lines 125 to 126
// Our finalizer is present, so we need to delete the config entry
// from consul.
Copy link
Member

Choose a reason for hiding this comment

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

Can probably drop this comment. Doesn't make much sense here since we're so deep into the deletion process here.

Copy link
Contributor

@kschoche kschoche 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!

@thisisnotashwin thisisnotashwin merged commit 40d03ab into crd-controller-base Oct 1, 2020
@thisisnotashwin thisisnotashwin deleted the add-metadata-to-crds branch October 1, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/crds type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants