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

DB cluster configuration to define if CQL session should enable TLS or not #3711

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

karol-kokoszka
Copy link
Collaborator

@karol-kokoszka karol-kokoszka commented Feb 8, 2024

Closes #3679

This PR introduces an additional flags defining the cluster managed by the Scylla Manager:

If any of these flags is not available in cluster add then both are evaluated to false.
If flag is not available in cluster update then update won't change current state of these flags in DB.

Additional tests are introduced, api-integration-tests that are suppose to call CLI / HTTP api against docker env and assert results.


Please make sure that:

  • Code is split to commits that address a single change
  • Commit messages are informative
  • Commit titles have module prefix
  • Commit titles have issue nr. suffix

@karol-kokoszka karol-kokoszka changed the title Kk/3679 cluster param indicating tls enabled DB cluster configuration to define if CQL session should enable TLS or not Feb 8, 2024
@karol-kokoszka karol-kokoszka force-pushed the kk/3679-cluster-param-indicating-TLS-enabled branch from 04bada9 to 35addef Compare February 9, 2024 10:58
@karol-kokoszka karol-kokoszka force-pushed the kk/3679-cluster-param-indicating-TLS-enabled branch 5 times, most recently from 246f24d to dd0125d Compare February 13, 2024 14:16
@karol-kokoszka karol-kokoszka force-pushed the kk/3679-cluster-param-indicating-TLS-enabled branch from dd0125d to 5381e2d Compare February 13, 2024 16:15
Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski left a comment

Choose a reason for hiding this comment

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

With those changes the cql dialing logic finally looks good! Also really nice idea with testing sctool commands.

I just left some comments about simplifying (if it is possible) the way in which the new flags are handled.

swagger/scylla-manager.json Outdated Show resolved Hide resolved
swagger/scylla-manager.json Outdated Show resolved Hide resolved
pkg/restapi/cluster.go Outdated Show resolved Hide resolved
pkg/command/cluster/clusteradd/cmd.go Outdated Show resolved Hide resolved
pkg/command/cluster/clusterupdate/cmd.go Outdated Show resolved Hide resolved
pkg/service/cluster/service.go Outdated Show resolved Hide resolved
pkg/service/cluster/service.go Show resolved Hide resolved
@Michal-Leszczynski
Copy link
Collaborator

Also, the res.yaml files containing description of sctool flags should be updated (they are used in our docs as well).

@Michal-Leszczynski
Copy link
Collaborator

Also, there are more failures in gh actions then expected, but they look like errors not connected to this PR.

…use non SSL port on cluster

Manager 3.2.6 gives a possibility of explicitly disabling TLS on session even though the
certificate and key are available `force_tls_disabled`.
Besides that, there is an option to force session to always use non-tls port from scylla config `force_non_ssl_session_port`.
@karol-kokoszka karol-kokoszka force-pushed the kk/3679-cluster-param-indicating-TLS-enabled branch from 5381e2d to d4f5fbe Compare February 14, 2024 13:19
@karol-kokoszka karol-kokoszka force-pushed the kk/3679-cluster-param-indicating-TLS-enabled branch from d4f5fbe to bdac817 Compare February 14, 2024 13:22
It adds information about `force-tls-disabled` and `force-non-ssl-session-port` flags.
@karol-kokoszka
Copy link
Collaborator Author

docs are updated with 274388b

Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski left a comment

Choose a reason for hiding this comment

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

One last thing would be to adjust healthcheck's pinging to follow the logic introduced here, but I guess it can be done in a separate PR.

@karol-kokoszka karol-kokoszka merged commit d86640b into master Feb 14, 2024
77 of 81 checks passed
@karol-kokoszka karol-kokoszka deleted the kk/3679-cluster-param-indicating-TLS-enabled branch February 14, 2024 15:17
This was referenced Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manager should not assume CQL-SSL in cluster sessions
2 participants