Skip to content

Conversation

@nitishm
Copy link
Contributor

@nitishm nitishm commented Mar 1, 2022

This change adds the generation of the Server cert and CA cert to the docker image. These certs are available to both the client and the server side to establish a secure gRPC connection between client & server.

Changes include,

  • Adding tls helper functions to transport package
  • Generate script to generate the TLS certificates
  • Dockerfile (Linux) updated to run the generate script at build time.

TODOs,

  • Create a generate script for Windows.

Reason for Change:

Issue Fixed:

Requirements:

Notes:

nitishm added 2 commits March 1, 2022 21:20
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
nitishm added 4 commits March 2, 2022 19:52
- Fixed an issue with the root command not parsing the config
- Removed ca key after signing server cert
- Fixed lint errors

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
serverCertPEMFilename = "tls.crt"
serverKeyPEMFilename = "tls.key"
caCertPEMFilename = "ca.crt"
path = "/usr/local/npm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this path be defined in azure-npm-config ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The certs are baked in through the Dockerfile. Not sure how to make that dynamic. Hence I made this static :/

Copy link
Collaborator

@rbtr rbtr Mar 7, 2022

Choose a reason for hiding this comment

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

you could make it dynamic using a volume mount at runtime for a BYO cert situation; it would be better for these to be configurable imo

if connectClient == nil {
klog.Info("Reconnecting to gRPC server controller")
opts := []grpc.CallOption{grpc.WaitForReady(true)}
opts := []grpc.CallOption{grpc.WaitForReady(false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

curious what this represents and why we had it true before, false now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True blocks the connection until the server is ready to accept the connection. We don't want to block our connection since the server pod could crash which we are connecting or in the load-balancing case reject the connection in which case the agent would need to block until timeout. Instead we try sending, error out and try reconnecting.

COPY --from=builder /usr/local/bin/azure-npm \
/usr/bin/azure-npm

COPY --from=builder /usr/local/src/npm/npm/scripts \
Copy link
Contributor

Choose a reason for hiding this comment

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

is npm/npm/ intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Since I didnt want to change the WORKDIR in the builder image (here) which copies the host's source to /usr/local/src/npm it has to be this way.

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>
@huntergregory huntergregory added the npm Related to NPM. label Mar 4, 2022
@vakalapa vakalapa enabled auto-merge (squash) March 4, 2022 22:26
@vakalapa vakalapa disabled auto-merge March 4, 2022 22:27
@vakalapa vakalapa merged commit 4971211 into Azure:master Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

npm Related to NPM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants