Add enhancement proposal to support Alternator API #1406

tnozicka · 2023-09-19T14:48:28Z

Description of your changes:
This PRs add a proposal to fully enable Alternator API in a secured way.

Which issue is resolved by this Pull Request:
Resolves #1405

tnozicka · 2023-09-19T14:50:30Z

enhancements/proposals/NNNN-alternator/README.md

+
+- Exposing Alternator on every node individually and relying on client side load balancing
+- Supporting host networking tweaks
+- Disabling CQL when Alternator is enabled


@tzach I recall you wanted the reverse but given CQL is needed for scaling procedures and AuthZ I don't see an easy way out

rzetelskik

solid, a couple of nits
/approve

enhancements/proposals/NNNN-alternator/README.md

rzetelskik · 2023-09-19T19:03:36Z

Also please remember to change the path from enhancements/proposals/NNNN-alternator/ to enhancements/proposals/1405-alternator/ (or 1405-alternator-api to better align with the PR's title?)

rzetelskik · 2023-09-19T19:06:20Z

The proposal is also missing the Implementation History section, like most of our proposals. Let's either stick to adding it or remove it from the proposal template (outside of this proposal's scope).

zimnx

first pass

enhancements/proposals/NNNN-alternator/README.md

tnozicka · 2023-09-20T08:41:42Z

I've noticed ScyllaDB sets up alternator HTTPS in tests on 8043 instead of 8001 laid out here before, let me know if have a preference.
https://github.com/scylladb/scylladb/blame/e784930dd7f051949c6e172c593f187ec6f97fbf/docs/dev/protocols.md#L202-L204

tnozicka · 2023-09-20T08:57:31Z

The proposal is also missing the Implementation History section, like most of our proposals. Let's either stick to adding it or remove it from the proposal template (outside of this proposal's scope).

I've found the section present in exactly 50% of the existing proposals and I haven't made up my mind on that yet. Maybe it's more useful to add from a second pass on but added it here.

rzetelskik · 2023-09-20T20:15:21Z

Typo in the path - should be enhancements/proposals/1405-alternator instead of enhancements/proposals/1406-alternator.

tnozicka · 2023-09-21T06:43:41Z

Typo in the path - should be enhancements/proposals/1405-alternator instead of enhancements/proposals/1406-alternator.

renamed, thx

zimnx

/lgtm

scylla-operator-bot · 2023-09-26T09:46:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tnozicka,zimnx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rzetelskik · 2023-09-26T09:47:59Z

/hold
I believe we're waiting for @tzach and @mykaul 's input?

tnozicka · 2023-09-26T11:51:14Z

@tzach @mykaul setting a lazy consensus for Wed 27th EOD.
(Tzach has agreed to having both CQL and Alternator exposed at the same time last week on our meeting so the most pressing question has been sorted out.)

mykaul · 2023-09-28T10:15:04Z

enhancements/proposals/1405-alternator/README.md

+
+## Summary
+
+This proposal aims to introduce proper support for enabling Alternator for ScyllaClusters 


Suggested change

This proposal aims to introduce proper support for enabling Alternator for ScyllaClusters

This proposal aims to introduce proper support for enabling Alternator for Scylla Clusters

we are intentionally putting those together as it's the exact CRD name

mykaul · 2023-09-28T10:17:23Z

enhancements/proposals/1405-alternator/README.md

+### Certificates
+
+Given we aim to always enable HTTPS we need to provision default serving certificate. Operator will create signer Secret `<sc-name>-local-alternator-serving-ca`, save the CA bundle into ConfigMap `<sc-name>-local-alternator-serving-ca` and create serving Secret `<sc-name>-local-alternator-serving-certs`.
+Serving certs will be valid for 30 days and rotated from 20 day mark.


This looks unusual low validity and quick rotation. I'm aware of 90 days or 1 year as more regular defaults.

This is in line with the other certificates we create.

I've seen 1y to be used only with manual renewals, I think Let's Encrypt prod CA is on 90d but mostly because some of the clients are not automated and this was a compromise between that. Security wise the lower the better. 1 month still gives enough time to deal with issues while giving a good security policy.

mykaul · 2023-09-28T10:18:28Z

I'm missing the proposed YAML changes (which is the 'interface' a user would use to enable it)

mykaul · 2023-09-28T10:24:05Z

enhancements/proposals/1405-alternator/README.md

+
+### API changes for ScyllaClusters.scylla.scylladb.com/v1
+
+The following API changes show how we'd approximately extend `scyllaclusters.scylla.scylladb.com/v1.spec.alternator` specification. Only new fields are shown for existing structs.


I'm not sure I'm seeing the port definition (8043 by default, I assume) ?

yep, we chose 8043 - described in https://github.com/scylladb/scylla-operator/pull/1406/files#diff-e7c253eebc3e6c9a4c7923ddf75c5c939cfe058d9cf01f085803b4974c2f378eR38

tnozicka · 2023-10-02T10:51:11Z

I'm missing the proposed YAML changes (which is the 'interface' a user would use to enable it)

yaml file is an instance of a schema, one of many possible instantiations. The various options and the schema is described with the Golang structs and special comments. The name of the yaml field is defined withing the API, say

ServingCertificate *TLSCertificate `json:"servingCertificate,omitempty"`

means there will be a field named servingCertificate at this level. We don't duplicate it here but when this is implemented we have code generators that take these structs and convert them to CRDs and define those fields.

so a simple example instance using the schema from
https://github.com/scylladb/scylla-operator/pull/1406/files#diff-e7c253eebc3e6c9a4c7923ddf75c5c939cfe058d9cf01f085803b4974c2f378eR80-R130
can look like

spec:
  alternator:
    insecureEnableHTTP: false
    servingCertificate:
      type: OperatorManaged

(for illustration, some of them can be defaulted)

tnozicka · 2023-10-03T08:44:07Z

@mykaul Thanks for your feedback, I hope I've resolved all your questions but I also know you are out and we need to branch so I'll unhold this. I am happy to address anything you have when you get back as a follow up, if needed.

/hold cancel

tnozicka · 2023-10-03T10:12:04Z

/retest

tnozicka · 2023-10-03T10:12:35Z

/override ci/prow/e2e-gke-parallel
/override ci/prow/e2e-gke-serial
(not needed for a proposal)

scylla-operator-bot · 2023-10-03T10:12:38Z

@tnozicka: Overrode contexts on behalf of tnozicka: ci/prow/e2e-gke-parallel, ci/prow/e2e-gke-serial

In response to this:

/override ci/prow/e2e-gke-parallel
/override ci/prow/e2e-gke-serial

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

scylla-operator-bot bot requested review from mykaul, rzetelskik, tzach and zimnx September 19, 2023 14:48

scylla-operator-bot bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 19, 2023

tnozicka changed the title ~~Add alternator proposal~~ Add enhancement proposal to support Alternator API Sep 19, 2023

tnozicka commented Sep 19, 2023

View reviewed changes

rzetelskik reviewed Sep 19, 2023

View reviewed changes

enhancements/proposals/NNNN-alternator/README.md Outdated Show resolved Hide resolved

enhancements/proposals/NNNN-alternator/README.md Outdated Show resolved Hide resolved

enhancements/proposals/NNNN-alternator/README.md Outdated Show resolved Hide resolved

zimnx reviewed Sep 19, 2023

View reviewed changes

tnozicka force-pushed the alternator-proposal branch 3 times, most recently from d6f7df9 to ac2ef3b Compare September 20, 2023 08:55

Add alternator proposal

b9585c3

tnozicka force-pushed the alternator-proposal branch from ac2ef3b to b9585c3 Compare September 21, 2023 06:43

tnozicka requested a review from zimnx September 25, 2023 05:44

zimnx approved these changes Sep 26, 2023

View reviewed changes

scylla-operator-bot bot assigned zimnx Sep 26, 2023

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2023

scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2023

mykaul reviewed Sep 28, 2023

View reviewed changes

tnozicka requested a review from mykaul October 2, 2023 10:52

scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2023

scylla-operator-bot bot merged commit 3ce65e6 into scylladb:master Oct 3, 2023
11 checks passed

tnozicka deleted the alternator-proposal branch October 3, 2023 10:13

zimnx added the kind/design Categorizes issue or PR as related to design. label Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enhancement proposal to support Alternator API #1406

Add enhancement proposal to support Alternator API #1406

tnozicka commented Sep 19, 2023

tnozicka Sep 19, 2023

rzetelskik left a comment

rzetelskik commented Sep 19, 2023 •

edited

rzetelskik commented Sep 19, 2023

zimnx left a comment

tnozicka commented Sep 20, 2023

tnozicka commented Sep 20, 2023

rzetelskik commented Sep 20, 2023

tnozicka commented Sep 21, 2023

zimnx left a comment

scylla-operator-bot bot commented Sep 26, 2023

rzetelskik commented Sep 26, 2023

tnozicka commented Sep 26, 2023

mykaul Sep 28, 2023

tnozicka Oct 2, 2023

mykaul Sep 28, 2023

tnozicka Oct 2, 2023

mykaul commented Sep 28, 2023

mykaul Sep 28, 2023

tnozicka Oct 2, 2023 •

edited

tnozicka commented Oct 2, 2023

tnozicka commented Oct 3, 2023

tnozicka commented Oct 3, 2023

tnozicka commented Oct 3, 2023 •

edited

scylla-operator-bot bot commented Oct 3, 2023


		## Summary

		This proposal aims to introduce proper support for enabling Alternator for ScyllaClusters


		### API changes for ScyllaClusters.scylla.scylladb.com/v1

		The following API changes show how we'd approximately extend `scyllaclusters.scylla.scylladb.com/v1.spec.alternator` specification. Only new fields are shown for existing structs.

Add enhancement proposal to support Alternator API #1406

Add enhancement proposal to support Alternator API #1406

Conversation

tnozicka commented Sep 19, 2023

tnozicka Sep 19, 2023

Choose a reason for hiding this comment

rzetelskik left a comment

Choose a reason for hiding this comment

rzetelskik commented Sep 19, 2023 • edited

rzetelskik commented Sep 19, 2023

zimnx left a comment

Choose a reason for hiding this comment

tnozicka commented Sep 20, 2023

tnozicka commented Sep 20, 2023

rzetelskik commented Sep 20, 2023

tnozicka commented Sep 21, 2023

zimnx left a comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Sep 26, 2023

rzetelskik commented Sep 26, 2023

tnozicka commented Sep 26, 2023

mykaul Sep 28, 2023

Choose a reason for hiding this comment

tnozicka Oct 2, 2023

Choose a reason for hiding this comment

mykaul Sep 28, 2023

Choose a reason for hiding this comment

tnozicka Oct 2, 2023

Choose a reason for hiding this comment

mykaul commented Sep 28, 2023

mykaul Sep 28, 2023

Choose a reason for hiding this comment

tnozicka Oct 2, 2023 • edited

Choose a reason for hiding this comment

tnozicka commented Oct 2, 2023

tnozicka commented Oct 3, 2023

tnozicka commented Oct 3, 2023

tnozicka commented Oct 3, 2023 • edited

scylla-operator-bot bot commented Oct 3, 2023

rzetelskik commented Sep 19, 2023 •

edited

tnozicka Oct 2, 2023 •

edited

tnozicka commented Oct 3, 2023 •

edited