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

Anonymous Email Sending Failed #1

Closed
jayhding opened this issue Feb 4, 2016 · 17 comments
Closed

Anonymous Email Sending Failed #1

jayhding opened this issue Feb 4, 2016 · 17 comments

Comments

@jayhding
Copy link
Contributor

jayhding commented Feb 4, 2016

Existing email sending schema requires auth method. It is necessary for public SMTP server but corporation internal SMTP can be used for anonymous email sending.

Therefore, default configuration will give following error

rancher-alarms_1 | [INFO]   2016-2-4 4:44:5:975       sending email notification to xxx.xxx@abc.com
rancher-alarms_1 | [ERROR]  2016-2-4 4:44:10:993      { [Error: Invalid login: 504 5.7.4 Unrecognized authentication type]
rancher-alarms_1 |   cause:
rancher-alarms_1 |    { [Error: Invalid login: 504 5.7.4 Unrecognized authentication type]
rancher-alarms_1 |      code: 'EAUTH',
rancher-alarms_1 |      response: '504 5.7.4 Unrecognized authentication type',
rancher-alarms_1 |      responseCode: 504 },
rancher-alarms_1 |   isOperational: true,
rancher-alarms_1 |   code: 'EAUTH',
rancher-alarms_1 |   response: '504 5.7.4 Unrecognized authentication type',
rancher-alarms_1 |   responseCode: 504 } Error: Invalid login: 504 5.7.4 Unrecognized authentication type
rancher-alarms_1 |     at SMTPConnection._formatError (/usr/src/app/node_modules/nodemailer/node_modules/nodemailer-smtp-transport/node_modules/smtp-connection/src/smtp-connection.js:384:15)
rancher-alarms_1 |     at SMTPConnection._actionAUTHComplete (/usr/src/app/node_modules/nodemailer/node_modules/nodemailer-smtp-transport/node_modules/smtp-connection/src/smtp-connection.js:878:30)
rancher-alarms_1 |     at SMTPConnection.<anonymous> (/usr/src/app/node_modules/nodemailer/node_modules/nodemailer-smtp-transport/node_modules/smtp-connection/src/smtp-connection.js:239:22)
rancher-alarms_1 |     at SMTPConnection._processResponse (/usr/src/app/node_modules/nodemailer/node_modules/nodemailer-smtp-transport/node_modules/smtp-connection/src/smtp-connection.js:516:16)
rancher-alarms_1 |     at SMTPConnection._onData (/usr/src/app/node_modules/nodemailer/node_modules/nodemailer-smtp-transport/node_modules/smtp-connection/src/smtp-connection.js:353:10)
rancher-alarms_1 |     at emitOne (events.js:77:13)
rancher-alarms_1 |     at Socket.emit (events.js:169:7)
rancher-alarms_1 |     at readableAddChunk (_stream_readable.js:146:16)
rancher-alarms_1 |     at Socket.Readable.push (_stream_readable.js:110:10)
rancher-alarms_1 |     at TCP.onread (net.js:523:20)

If the auth part is removed in email.es6 then it's fine

this._sender = promisifyAll(nodemailer.createTransport({
      port: smtp.port,
      host: smtp.host,
      from: smtp.from
    }));
}
@flaccid
Copy link
Contributor

flaccid commented Mar 1, 2016

@ndelitski would you entertain a pull request for supporting noauth?

@ndelitski
Copy link
Owner

@flaccid surely! sry didn't see notifications earlier. we will definitely fix it soon!

@flaccid
Copy link
Contributor

flaccid commented Mar 1, 2016

@ndelitski no worries, we also would like templating of the emails including html; I sent you an email, but basically just wondering if pull requests are ok or if you would rather do in your own way - templating for example is not a minor change.

@ndelitski
Copy link
Owner

PR with fixes are totally ok! but for features like email templating project needs significant refactoring as you said, it would be better if I do it by myself. I'll try release templating these week, it depends on my current workload

@ndelitski
Copy link
Owner

Mounting a template file(something like .hbs) into rancher-alarms container sounds good for you?

@flaccid
Copy link
Contributor

flaccid commented Mar 1, 2016

No problem, we'll keep an eye out and then after these 2 are addressed see if we can PR any fixes that might be laying around. I think for feature changes that are not minor, we'll start with an issue first.

Re: templating, handlebars is fine but we do need a basic way to do this in compose directly in rancher without using volumes if possible though it might make sense that the rancher-alarms container can simply take a volumes-from of a data container with custom templates inside.

@ndelitski
Copy link
Owner

@JayHaoDing @flaccid just released a prepatch version image with noauth fix, please check it out with your smtp server docker pull ndelitski/rancher-alarms:0.1.1-0 and then I release 0.1.1 and update latest image

@ndelitski
Copy link
Owner

FYI code changes since 0.1.0 aafeabb

@jayhding
Copy link
Contributor Author

jayhding commented Mar 1, 2016

@ndelitski test result showed the solution did not work. I did not configure ALARM_EMAIL_USER and ALARM_EMAIL_PASS but seems smtp.auth is still not NULL.

npm info it worked if it ends with ok
npm info using npm@2.14.7
npm info using node@v4.2.1
npm info prestart rancher-alarms@0.1.0
npm info start rancher-alarms@0.1.0

> rancher-alarms@0.1.0 start /usr/src/app
> node bin/rancher-alarms.js

[INFO]   2016-3-1 22:45:42:554     trying to compose config from env variables
[INFO]   2016-3-1 22:45:42:572     started with config:
{
    "rancher": {
        "address": "http://rancher.xxx.xxx/v1/projects/***",
        "auth": {
            "accessKey": "***",
            "secretKey": "***"
        },
        "projectId": "***"
    },
    "pollServicesInterval": 60000,
    "filter": [
        "***"
    ],
    "notifications": {
        "*": {
            "targets": {
                "email": {
                    "recipients": [
                        "xxx.xxx@***.***"
                    ]
                }
            },
            "healthcheck": {
                "pollInterval": 15000,
                "healthyThreshold": 3,
                "unhealthyThreshold": 4
            }
        }
    },
    "targets": {
        "email": {
            "smtp": {
                "from": "***@***",
                "auth": {},
                "host": "***",
                "secureConnection": true,
                "port": "25"
            }
        }
    }
}
[ERROR]  2016-3-1 22:45:43:139     { [AssertionError: undefined == true]
  name: 'AssertionError',
  actual: undefined,
  expected: true,
  operator: '==',
  message: 'undefined == true',
  generatedMessage: true } AssertionError: undefined == true
    at new EmailTarget (/usr/src/app/src/notifications/email.es6:18:7)
    at Function.init (/usr/src/app/src/notifications/target.es6:12:12)
    at ServiceStateMonitor.setupNotificationsTargets (/usr/src/app/src/monitor.es6:76:33)
    at new ServiceStateMonitor (/usr/src/app/src/monitor.es6:69:12)
    at initServiceMonitor$ (/usr/src/app/src/server.es6:52:12)
    at tryCatch (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:61:40)
    at GeneratorFunctionPrototype.invoke [as _invoke] (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:328:22)
    at GeneratorFunctionPrototype.prototype.(anonymous function) [as next] (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:94:21)
    at invoke (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:136:37)
    at callInvokeWithMethodAndArg (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:172:16)
    at previousPromise (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:194:19)
    at new Promise (/usr/src/app/node_modules/babel-core/node_modules/core-js/modules/es6.promise.js:197:7)
    at AsyncIterator.enqueue (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:193:13)
    at AsyncIterator.prototype.(anonymous function) [as next] (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:94:21)
    at Object.runtime.async (/usr/src/app/node_modules/babel-core/node_modules/regenerator/runtime.js:215:14)
    at initServiceMonitor (/usr/src/app/src/server.es6:51:22)

npm info rancher-alarms@0.1.0 Failed to exec start script

And just FYI, due to proxy issue we cannot use the rancher-alarms image directly so we have touse own Dockerfile and looks like

FROM node:4.2.1

RUN mkdir -p /usr/src/app
WORKDIR /usr/src/app

COPY package.json /usr/src/app/

RUN npm config set http-proxy http://x.x.x.x:8080
RUN npm config set https-proxy http://x.x.x.x:8080
RUN npm install

COPY . /usr/src/app

CMD [ "npm", "start" ]

@ndelitski
Copy link
Owner

@JayHaoDing no need to pass empty hash in auth parameter, current logic is – if auth present it should have credentials or it is invalid

@jayhding
Copy link
Contributor Author

jayhding commented Mar 2, 2016

But it didn't work according to my test. I simpley did not set any value (not mention at all) for USER and PASS in docker compose file. What else shall I do?
To me it looked like the auth{} did not make the if check be failed. 

Regards,
Jay Ding

_____________________________

From: Nick Delitski notifications@github.com
Sent: Wednesday, March 2, 2016 17:25
Subject: Re: [rancher-alarms] Anonymous Email Sending Failed (#1)
To: ndelitski/rancher-alarms rancher-alarms@noreply.github.com
Cc: Jay (Hao) Ding jay.dinghao@outlook.com

@JayHaoDing no need to pass empty hash in auth parameter, current logic is – if auth present it should have credentials or it is invalid


Reply to this email directly or view it on GitHub.

@flaccid
Copy link
Contributor

flaccid commented Mar 2, 2016

Perhaps support either method and document best method in readme?

@ndelitski
Copy link
Owner

@JayHaoDing you are right, I forgot a part where environment variables are composed into config. In a future refactoring I am planning to move config asserts into one place.
Updated master, this should fix. will update readme later

@jayhding
Copy link
Contributor Author

jayhding commented Mar 2, 2016

@ndelitski new changes have fixed this issue, thanks!

@flaccid
Copy link
Contributor

flaccid commented Mar 3, 2016

Good to close (after merge to master)?

@ndelitski
Copy link
Owner

will close soon after updating docs

@flaccid
Copy link
Contributor

flaccid commented Aug 9, 2016

Possible to wrap this one up soon?

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

No branches or pull requests

3 participants