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

[release/1.1.x] config entry: add validate_clusters to mesh config entry #4262

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Aug 24, 2024

Changes proposed in this PR

How I've tested this PR

How I expect reviewers to test this PR

Checklist

@ndhanushkodi ndhanushkodi added the pr/no-backport signals that a PR will not contain a backport label label Aug 24, 2024
@ndhanushkodi ndhanushkodi changed the title config entry: add validate_clusters to mesh config entry [release/1.1.x] config entry: add validate_clusters to mesh config entry Aug 25, 2024
labels:
app: {{ template "consul.name" . }}
chart: {{ template "consul.chart" . }}
heritage: {{ .Release.Service }}
release: {{ .Release.Name }}
component: crd
spec:
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is back to below labels in https://github.com/hashicorp/consul-k8s/blob/release/1.5.x/charts/consul/templates/crd-exportedservices-v1.yaml

Is it possible this is wrong? I don't see it in the spec schema for 1.28.14:

    "io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionSpec": {
      "description": "CustomResourceDefinitionSpec describes how a user wants their resource to appear",
      "properties": {
        "conversion": {
          "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceConversion",
          "description": "conversion defines conversion settings for the CRD."
        },
        "group": {
          "description": "group is the API group of the defined custom resource. The custom resources are served under `/apis/<group>/...`. Must match the name of the CustomResourceDefinition (in the form `<names.plural>.<group>`).",
          "type": "string"
        },
        "names": {
          "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionNames",
          "description": "names specify the resource and kind names for the custom resource."
        },
        "preserveUnknownFields": {
          "description": "preserveUnknownFields indicates that object fields which are not specified in the OpenAPI schema should be preserved when persisting to storage. apiVersion, kind, metadata and known fields inside metadata are always preserved. This field is deprecated in favor of setting `x-preserve-unknown-fields` to true in `spec.versions[*].schema.openAPIV3Schema`. See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning for details.",
          "type": "boolean"
        },
        "scope": {
          "description": "scope indicates whether the defined custom resource is cluster- or namespace-scoped. Allowed values are `Cluster` and `Namespaced`.",
          "type": "string"
        },
        "versions": {
          "description": "versions is the list of all API versions of the defined custom resource. Version names are used to compute the order in which served versions are listed in API discovery. If the version string is \"kube-like\", it will sort above non \"kube-like\" version strings, which are ordered lexicographically. \"Kube-like\" versions start with a \"v\", then are followed by a number (the major version), then optionally the string \"alpha\" or \"beta\" and another number (the minor version). These are sorted first by GA > beta > alpha (where GA is a version with no suffix such as beta or alpha), and then by comparing major version, then minor version. An example sorted list of versions: v10, v2, v1, v11beta2, v10beta3, v3beta1, v12alpha1, v11alpha2, foo1, foo10.",
          "items": {
            "$ref": "#/definitions/io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion"
          },
          "type": "array"
        }
      },
      "required": [
        "group",
        "names",
        "scope",
        "versions"
      ],
      "type": "object"
    },

Copy link
Member

Choose a reason for hiding this comment

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

Same question for the other templates below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it! I tried syncing the makefile, and the copy-crds-to-chart hack script from 1.3.x to here, and then realized that the hack script does a very hacky thing and uses line numbers to insert labels into the CRD. I had to tweak the line number a bit (I think the license header or something like that was causing the issue)

Copy link
Member

Choose a reason for hiding this comment

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

Aha!! Nice find, glad we have an answer!

Copy link
Member

@zalimeni zalimeni Aug 28, 2024

Choose a reason for hiding this comment

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

And we can probably come up with a nearly-equivalent func that avoids line number hacking. Will take a look tomorrow.

Copy link
Member

@zalimeni zalimeni Aug 28, 2024

Choose a reason for hiding this comment

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

Tried improving on the blind line match that led to the bug: https://github.com/hashicorp/consul-k8s/pull/4268/files happy to punt to after these PRs close if you'd rather

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!! That's great!! I skimmed and it looks great but will take a deeper look right after the k8s release!

cli/go.mod Show resolved Hide resolved
Copy link
Member

@zalimeni zalimeni left a comment

Choose a reason for hiding this comment

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

Couple of questions about schema and deps updates, but otherwise looking good!

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
labels:
app: {{ template "consul.name" . }}
chart: {{ template "consul.chart" . }}
heritage: {{ .Release.Service }}
release: {{ .Release.Name }}
component: crd
spec:
Copy link
Member

@zalimeni zalimeni Aug 28, 2024

Choose a reason for hiding this comment

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

Tried improving on the blind line match that led to the bug: https://github.com/hashicorp/consul-k8s/pull/4268/files happy to punt to after these PRs close if you'd rather

Copy link
Member

@zalimeni zalimeni left a comment

Choose a reason for hiding this comment

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

Couple of non-blocking comments, but LGTM! Thank you for working through this and all the hairy history 🙏🏻

@ndhanushkodi ndhanushkodi merged commit 5b2215a into release/1.1.x Aug 28, 2024
47 checks passed
@ndhanushkodi ndhanushkodi deleted the nd/net-10435-cluster-validation-1.1.x branch August 28, 2024 19:51
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants