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

Stub mesh configuration resource controller #3302

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

andrewstucki
Copy link
Contributor

Changes proposed in this PR:

Adds basic stubbing/syncing of "MeshConfiguration" resources to Consul.

How I've tested this PR:

Manually created a CRD and saw it propagate to Consul:

apiVersion: mesh.consul.hashicorp.com/v2beta1
kind: MeshConfiguration
metadata:
  name: configuration

and then with consul resource list mesh.v2beta1.meshconfiguration:

{
    "resources": [
        {
            "data": {},
            "generation": "01HGXDYZTYK20821Q9Z6JMT9YS",
            "id": {
                "name": "configuration",
                "tenancy": {
                    "partition": "default",
                    "peerName": "local"
                },
                "type": {
                    "group": "mesh",
                    "groupVersion": "v2beta1",
                    "kind": "MeshConfiguration"
                },
                "uid": "01HGXDYZTYK20821Q9Z4BJPSHR"
            },
            "metadata": {
                "external-source": "kubernetes"
            },
            "version": "154"
        }
    ]
}

How I expect reviewers to test this PR:

Checklist:

@andrewstucki andrewstucki added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport signals that a PR will not contain a backport label labels Dec 5, 2023
@andrewstucki andrewstucki requested review from a team, NiniOak and t-eckert and removed request for a team December 5, 2023 16:50
@@ -50,13 +50,13 @@ type MeshConfigurationList struct {
Items []*MeshConfiguration `json:"items"`
}

func (in *MeshConfiguration) ResourceID(namespace, partition string) *pbresource.ID {
func (in *MeshConfiguration) ResourceID(_, partition string) *pbresource.ID {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a blank identifier here as opposed to just removing the namespace parameter as it's no longer required?

@@ -65,9 +65,9 @@ func (in *MeshConfiguration) ResourceID(namespace, partition string) *pbresource
}
}

func (in *MeshConfiguration) Resource(namespace, partition string) *pbresource.Resource {
func (in *MeshConfiguration) Resource(_, partition string) *pbresource.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

@andrewstucki andrewstucki Dec 6, 2023

Choose a reason for hiding this comment

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

These are both to implement the ConsulResource interface found here -- it's what allows us to leverage the shared controller code:

ResourceID(namespace, partition string) *pbresource.ID
Resource(namespace, partition string) *pbresource.Resource

So I need to keep the arguments even though they're unused since the resource is purely partition scoped (it doesn't have a namespace). It's a much less invasive change on the shared controller -- alternatively we'd need to make the shared code aware of resources that are partition-scoped and treat them differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation

@andrewstucki andrewstucki merged commit ff66cfe into main Dec 7, 2023
55 checks passed
@andrewstucki andrewstucki deleted the net-6422-mesh-configuration-stub branch December 7, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport signals that a PR will not contain a backport label pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants