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.

Sign up for GitHub

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

Jump to bottom

Default namespaces for CRDs that support namespace when enabled #413

Merged
merged 5 commits into from
Jan 19, 2021
Merged

Default namespaces for CRDs that support namespace when enabled #413

merged 5 commits into from
Jan 19, 2021

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Dec 18, 2020 •
edited
Loading

Changes proposed in this PR:

  • Default the namespace fields of resources when namespaces are enabled so that the controller doesnt continuously attempt to reconcile them as the value presented by consul does not match the spec (because Consul defaults the namespace field where we don't).

How to test this PR:

  • Code Review
  • Deploy consul-enterprise with the consul-k8s image ashwinvenkatesh/consul-k8s:default-ns. Allow namespaces on the consul installation.

Verify that creating an ingress-gateway, terminating-gateway and service-router without namespace fields reconciles just once and does not lead to a reconcile loop.

Checklist:

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

lkysow
lkysow reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looks good, I like the approach.

Can we remove the special case for serviceintentions now?

I checked out which places default, it's inconsistent. I've made comments where the defaulting doesn't happen.

api/common/configentry_webhook.go Outdated Show resolved Hide resolved
api/v1alpha1/serviceresolver_types.go Outdated Show resolved Hide resolved
api/v1alpha1/serviceresolver_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicesplitter_types.go Outdated Show resolved Hide resolved
api/common/configentry.go Outdated Show resolved Hide resolved
@thisisnotashwin
Copy link
Contributor Author

Can we remove the special case for serviceintentions now?

We might have to keep that around because the validation behavior for Service Intentions are different from the other CRDs (we compare the name and namespace of the destination from the spec which is unique to the CRD)

@thisisnotashwin thisisnotashwin marked this pull request as ready for review January 12, 2021 21:05
@thisisnotashwin thisisnotashwin requested review from a team, ishustava and lkysow and removed request for a team January 12, 2021 21:22
lkysow
lkysow reviewed Jan 12, 2021
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

This is looking good! I haven't tested it out yet. I think we might be able to reduce the code duplication between serviceintentions_webhook.

CHANGELOG.md Outdated Show resolved Hide resolved
api/common/configentry.go Outdated Show resolved Hide resolved
api/v1alpha1/ingressgateway_types.go Outdated Show resolved Hide resolved
api/v1alpha1/ingressgateway_types.go Outdated Show resolved Hide resolved
api/v1alpha1/ingressgateway_types_test.go Outdated Show resolved Hide resolved
api/v1alpha1/proxydefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicerouter_types.go Outdated Show resolved Hide resolved
api/v1alpha1/serviceresolver_types.go Show resolved Hide resolved
api/common/configentry_webhook.go Outdated Show resolved Hide resolved
lkysow
lkysow approved these changes Jan 13, 2021
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

I tested this all out and it works great.

CHANGELOG.md Outdated Show resolved Hide resolved
api/v1alpha1/ingressgateway_types_test.go Outdated Show resolved Hide resolved
ishustava
ishustava reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looking pretty good, Ashwin! I only had one suggestion that I think is holding the approval (to add a test case checking that the DefaultNamespaceFields implementations won't overwrite a field that's already set); the rest of my comments are pretty minor. Great work!

api/common/configentry_webhook_test.go
c.nsMirroring)
c.nsMirroring,
c.consulDestinationNS,
c.nsMirroringPrefix)
require.Equal(t, c.expAllow, response.Allowed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add an assertion here that the defaulting has worked (maybe check that the DefaultNamespaceFields method was called on the mock config entry)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for the same. It is fairly primitive but let me know if that works of if you'd like something a little more fleshed out.

Copy link
Contributor

Choose a reason for hiding this comment

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

that works, thanks!

api/v1alpha1/ingressgateway_types.go Show resolved Hide resolved
api/v1alpha1/serviceresolver_types.go Show resolved Hide resolved
api/v1alpha1/terminatinggateway_types_test.go Show resolved Hide resolved
@thisisnotashwin
Validate namespace field do not get over-written if already set.
642d79b 
- Add test to verify DefaultingPatches invokes DefaultNamespaceFields.
- Add return statement to DefaultNamespaceFields with no behavior.
ishustava
ishustava approved these changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@ishustava ishustava 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! Thanks for addressing the comments.

api/common/configentry_webhook_test.go
c.nsMirroring)
c.nsMirroring,
c.consulDestinationNS,
c.nsMirroringPrefix)
require.Equal(t, c.expAllow, response.Allowed)
Copy link
Contributor

Choose a reason for hiding this comment

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

that works, thanks!

@thisisnotashwin thisisnotashwin merged commit 79885bf into master Jan 19, 2021
@thisisnotashwin thisisnotashwin deleted the crd-namespaces branch January 19, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkysow lkysow lkysow approved these changes

@ishustava ishustava ishustava approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thisisnotashwin @lkysow @ishustava