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

Kustomize files in e2e-tests don't set correct clientConfig.service.namespace in cass-operator ValidatingWebhookConfiguration #397

Closed
burmanm opened this issue Feb 14, 2022 · 7 comments
Labels
duplicate This issue or pull request already exists

Comments

@burmanm
Copy link
Contributor

burmanm commented Feb 14, 2022

What happened?
When building control-plane kustomize template, we create one unnecessary namespace: cass-operator:

k8ssandra-operator git:(validating_webhook) ✗ bin/kustomize build config/deployments/control-plane > control-plane.yamlk8ssandra-operator git:(validating_webhook) ✗ grep 'kind: Namespace' control-plane.yaml | wc -l
2k8ssandra-operator git:(validating_webhook) ✗

There's also incorrect cass-operator webhook service deployment:

kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: k8ssandra-operator/cass-operator-serving-cert
  name: cass-operator-validating-webhook-configuration
webhooks:
- admissionReviewVersions:
  - v1
  - v1beta1
  clientConfig:
    service:
      name: cass-operator-webhook-service
      namespace: cass-operator
      path: /validate-cassandra-datastax-com-v1beta1-cassandradatacenter

The clientConfig/service/namespace should be in k8ssandra-operator namespace since that's where the webhook path is also deployed to.

Did you expect to see something different?

How to reproduce it (as minimally and precisely as possible):
build config/deployments/control-plane

@jsanda
Copy link
Contributor

jsanda commented Feb 14, 2022

#387 should address this. @burmanm can you test and confirm?

@burmanm
Copy link
Contributor Author

burmanm commented Feb 14, 2022

It removes the namespace, but not the webhook deployment issue.

@burmanm burmanm added the duplicate This issue or pull request already exists label Feb 14, 2022
@burmanm burmanm closed this as completed Feb 14, 2022
@Miles-Garnsey Miles-Garnsey reopened this Feb 15, 2022
@Miles-Garnsey
Copy link
Member

Miles-Garnsey commented Feb 15, 2022

This may be a legit issue so I'm reopening it. @jsanda is correct in saying that #387 addresses the additional namespace and just needs to be merged to solve that problem.

The ValidatingWebhookConfiguration is a weird issue and actually appears to be a regression. If you run the following you'll note that the namespace is correct:

git checkout main
kustomize build config/deployments/control-plane | yq e 'select(.kind == "ValidatingWebhookConfiguration")' -
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: k8ssandra-operator/cass-operator-serving-cert
  name: cass-operator-validating-webhook-configuration
webhooks:
  - admissionReviewVersions:
      - v1
      - v1beta1
    clientConfig:
      service:
        name: cass-operator-webhook-service
        namespace: k8ssandra-operator
        path: /validate-cassandra-datastax-com-v1beta1-cassandradatacenter
    failurePolicy: Ignore
    name: vcassandradatacenter.kb.io
    rules:
      - apiGroups:
          - cassandra.datastax.com
        apiVersions:
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        resources:
          - cassandradatacenters
    sideEffects: None

However, if you do the same on (for e.g.) jsanda/schema-agreement you get:

git checkout jsanda/schema-agreement
kustomize build config/deployments/control-plane | yq e 'select(.kind == "ValidatingWebhookConfiguration")' -
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: k8ssandra-operator/cass-operator-serving-cert
  name: cass-operator-validating-webhook-configuration
webhooks:
  - admissionReviewVersions:
      - v1
      - v1beta1
    clientConfig:
      service:
        name: cass-operator-webhook-service
        namespace: cass-operator
        path: /validate-cassandra-datastax-com-v1beta1-cassandradatacenter
    failurePolicy: Ignore
    name: vcassandradatacenter.kb.io
    rules:
      - apiGroups:
          - cassandra.datastax.com
        apiVersions:
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        resources:
          - cassandradatacenters
    sideEffects: None

@burmanm can you confirm whether you were observing the ValidatingWebhookConfiguration issue on main or elsewhere?

@Miles-Garnsey
Copy link
Member

Michael reports that, having rebased his branch, he no longer sees problems with the ValidatingWebhookConfiguration's namespace.

To ensure that these issues do not return, I have added an additional assertion to the kuttl test test-user-defined-ns to ensure that the cass-operator ValidatingWebhookConfiguration's webhooks.0.clientConfig.service.namespace is always k8ssandra-operator NS.

If we start seeing that failing elsewhere then we'll need to investigate further, but for now, it may just be that some branches have not been rebased from the most recent kustomize changes.

@burmanm
Copy link
Contributor Author

burmanm commented Feb 15, 2022

Now I'll genuinely reopen this since actually.. there is an issue. Everything works if the namespace is indeed k8ssandra-operator, but once that's changed, things no longer deploy correctly. If I run bin/kustomize build build/test-config/k8ssandra-operator/control-plane > default-test.yaml and verify the output, I see:

apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: single-dc-6228xl/cass-operator-serving-cert
  name: cass-operator-validating-webhook-configuration
webhooks:
- admissionReviewVersions:
  - v1
  - v1beta1
  clientConfig:
    service:
      name: cass-operator-webhook-service
      namespace: k8ssandra-operator
      path: /validate-cassandra-datastax-com-v1beta1-cassandradatacenter

Here we see the clientConfig/service/name not being the correct one, but pointing to a wrong namespace.

@Miles-Garnsey Miles-Garnsey changed the title control-plane deployment creates additional namespace and deploys stuff to wrong namespace Kustomize files in e2e-tests don't set correct clientConfig.service.namespace in cass-operator ValidatingWebhookConfiguration Feb 16, 2022
@Miles-Garnsey
Copy link
Member

Right, this is specific to the tests because they have an additional kustomize overlay which sets the test namespace.

That overlay probably doesn't contain the required replacements for the ValidatingWebhookConfiguration.

This is a very niche thing, but I have started work to fix it under 415 so that the e2e test kustomizations will be compatible with the cass-operator changes you've made here.

@Miles-Garnsey
Copy link
Member

Miles-Garnsey commented Feb 24, 2022

Closed via #365.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants