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

Ensure ServiceIntentions aren't resynced #416

Merged
merged 1 commit into from
Jan 7, 2021
Merged

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Jan 6, 2021

Consul will internally sort the array of ServiceIntention sources by
precedence. For example, if it receives:

sources:
  - name: "*"
    action: allow
  - name: bar
    action: allow

It will return it in precedence sorted order:

sources:
  - name: bar
    action: allow
  - name: "*"
    action: allow

This sorting breaks our MatchesConsul function because we think the
resource has changed and so we will update it continually since it will
never match.

This change uses gocmp's SortSlices to re-sort the sources array so that
both the Kube resource and the Consul resource can be compared despite
Consul's sorting. We use a JSON encoded representation of the source
element with the fields that Consul set automatically zero'd out.

How I've tested this PR:

  • unit tests

  • tested with resource:

    apiVersion: consul.hashicorp.com/v1alpha1
    kind: ServiceIntentions
    metadata:
      name: foo
    spec:
      destination:
        name: foo
      sources:
        - name: "*"
          action: allow
        - name: bar
          action: allow

    Before:

     2021-01-06T22:04:06.335Z  INFO  webhooks.serviceintentions  validate create {"name": "foo"}
     2021-01-06T22:04:06.347Z  INFO  webhooks.serviceintentions  validate update {"name": "foo"}
     2021-01-06T22:04:06.365Z  INFO  controller.serviceintentions  config entry not found in consul  {"request": "default/foo"}
     2021-01-06T22:04:06.377Z  INFO  controller.serviceintentions  config entry created  {"request": "default/foo", "request-time": "11.594086ms"}
     2021-01-06T22:04:06.390Z  INFO  controller.serviceintentions  config entry does not match consul  {"request": "default/foo", "modify-index": 511}
     2021-01-06T22:04:06.394Z  INFO  controller.serviceintentions  config entry updated  {"request": "default/foo", "request-time": "3.454279ms"}
    

    After:

    2021-01-06T22:07:19.745Z  INFO  webhooks.serviceintentions  validate create {"name": "foo"}
    2021-01-06T22:07:19.751Z  INFO  webhooks.serviceintentions  validate update {"name": "foo"}
    2021-01-06T22:07:19.759Z  INFO  controller.serviceintentions  config entry not found in consul  {"request": "default/foo"}
    2021-01-06T22:07:19.762Z  INFO  controller.serviceintentions  config entry created  {"request": "default/foo", "request-time": "3.048659ms"}
    

How I expect reviewers to test this PR: Code

Checklist:

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

Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This looks great!! Have a minor comment but it isn't blocking.

api/v1alpha1/serviceintentions_types.go Outdated Show resolved Hide resolved
@lkysow lkysow force-pushed the crd-source-ixn-sort branch 2 times, most recently from 5213926 to 1ee535a Compare January 6, 2021 22:22
@lkysow lkysow requested review from a team and ndhanushkodi and removed request for a team January 6, 2021 22:23
Copy link
Contributor

@ndhanushkodi ndhanushkodi 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 🎉 !!

Consul will internally sort the array of ServiceIntention sources by
precedence. For example, if it receives:

```
sources:
  - name: "*"
    action: allow
  - name: bar
    action: allow
```

It will return it in precedence sorted order:

```
sources:
  - name: bar
    action: allow
  - name: "*"
    action: allow
```

This sorting breaks our MatchesConsul function because we think the
resource has changed and so we will update it continually since it will
never match.

This change uses gocmp's SortSlices to re-sort the sources array so that
both the Kube resource and the Consul resource can be compared despite
Consul's sorting. We use a JSON encoded representation of the source
element with the fields that Consul set automatically zero'd out.
@lkysow lkysow merged commit 0b6cf87 into master Jan 7, 2021
@lkysow lkysow deleted the crd-source-ixn-sort branch January 7, 2021 00:53
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants